Skip to content

Commit

Permalink
ALSA: control: Fix race between adding and removing a user element
Browse files Browse the repository at this point in the history
The procedure for adding a user control element has some window opened
for race against the concurrent removal of a user element.  This was
caught by syzkaller, hitting a KASAN use-after-free error.

This patch addresses the bug by wrapping the whole procedure to add a
user control element with the card->controls_rwsem, instead of only
around the increment of card->user_ctl_count.

This required a slight code refactoring, too.  The function
snd_ctl_add() is split to two parts: a core function to add the
control element and a part calling it.  The former is called from the
function for adding a user control element inside the controls_rwsem.

One change to be noted is that snd_ctl_notify() for adding a control
element gets called inside the controls_rwsem as well while it was
called outside the rwsem.  But this should be OK, as snd_ctl_notify()
takes another (finer) rwlock instead of rwsem, and the call of
snd_ctl_notify() inside rwsem is already done in another code path.

Reported-by: syzbot+dc09047bce3820621ba2@syzkaller.appspotmail.com
Cc: <stable@vger.kernel.org>
Signed-off-by: Takashi Iwai <tiwai@suse.de>
  • Loading branch information
Takashi Iwai committed Nov 24, 2018
1 parent 9a20332 commit e1a7bfe
Showing 1 changed file with 45 additions and 35 deletions.
80 changes: 45 additions & 35 deletions sound/core/control.c
Original file line number Diff line number Diff line change
Expand Up @@ -348,6 +348,40 @@ static int snd_ctl_find_hole(struct snd_card *card, unsigned int count)
return 0;
}

/* add a new kcontrol object; call with card->controls_rwsem locked */
static int __snd_ctl_add(struct snd_card *card, struct snd_kcontrol *kcontrol)
{
struct snd_ctl_elem_id id;
unsigned int idx;
unsigned int count;

id = kcontrol->id;
if (id.index > UINT_MAX - kcontrol->count)
return -EINVAL;

if (snd_ctl_find_id(card, &id)) {
dev_err(card->dev,
"control %i:%i:%i:%s:%i is already present\n",
id.iface, id.device, id.subdevice, id.name, id.index);
return -EBUSY;
}

if (snd_ctl_find_hole(card, kcontrol->count) < 0)
return -ENOMEM;

list_add_tail(&kcontrol->list, &card->controls);
card->controls_count += kcontrol->count;
kcontrol->id.numid = card->last_numid + 1;
card->last_numid += kcontrol->count;

id = kcontrol->id;
count = kcontrol->count;
for (idx = 0; idx < count; idx++, id.index++, id.numid++)
snd_ctl_notify(card, SNDRV_CTL_EVENT_MASK_ADD, &id);

return 0;
}

/**
* snd_ctl_add - add the control instance to the card
* @card: the card instance
Expand All @@ -364,45 +398,18 @@ static int snd_ctl_find_hole(struct snd_card *card, unsigned int count)
*/
int snd_ctl_add(struct snd_card *card, struct snd_kcontrol *kcontrol)
{
struct snd_ctl_elem_id id;
unsigned int idx;
unsigned int count;
int err = -EINVAL;

if (! kcontrol)
return err;
if (snd_BUG_ON(!card || !kcontrol->info))
goto error;
id = kcontrol->id;
if (id.index > UINT_MAX - kcontrol->count)
goto error;

down_write(&card->controls_rwsem);
if (snd_ctl_find_id(card, &id)) {
up_write(&card->controls_rwsem);
dev_err(card->dev, "control %i:%i:%i:%s:%i is already present\n",
id.iface,
id.device,
id.subdevice,
id.name,
id.index);
err = -EBUSY;
goto error;
}
if (snd_ctl_find_hole(card, kcontrol->count) < 0) {
up_write(&card->controls_rwsem);
err = -ENOMEM;
goto error;
}
list_add_tail(&kcontrol->list, &card->controls);
card->controls_count += kcontrol->count;
kcontrol->id.numid = card->last_numid + 1;
card->last_numid += kcontrol->count;
id = kcontrol->id;
count = kcontrol->count;
err = __snd_ctl_add(card, kcontrol);
up_write(&card->controls_rwsem);
for (idx = 0; idx < count; idx++, id.index++, id.numid++)
snd_ctl_notify(card, SNDRV_CTL_EVENT_MASK_ADD, &id);
if (err < 0)
goto error;
return 0;

error:
Expand Down Expand Up @@ -1361,9 +1368,12 @@ static int snd_ctl_elem_add(struct snd_ctl_file *file,
kctl->tlv.c = snd_ctl_elem_user_tlv;

/* This function manage to free the instance on failure. */
err = snd_ctl_add(card, kctl);
if (err < 0)
return err;
down_write(&card->controls_rwsem);
err = __snd_ctl_add(card, kctl);
if (err < 0) {
snd_ctl_free_one(kctl);
goto unlock;
}
offset = snd_ctl_get_ioff(kctl, &info->id);
snd_ctl_build_ioff(&info->id, kctl, offset);
/*
Expand All @@ -1374,10 +1384,10 @@ static int snd_ctl_elem_add(struct snd_ctl_file *file,
* which locks the element.
*/

down_write(&card->controls_rwsem);
card->user_ctl_count++;
up_write(&card->controls_rwsem);

unlock:
up_write(&card->controls_rwsem);
return 0;
}

Expand Down

0 comments on commit e1a7bfe

Please sign in to comment.