Donate to e Foundation | Murena handsets with /e/OS | Own a part of Murena! Learn more

Commit e1a7bfe3 authored by Takashi Iwai's avatar Takashi Iwai
Browse files

ALSA: control: Fix race between adding and removing a user element 



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: default avatar <syzbot+dc09047bce3820621ba2@syzkaller.appspotmail.com>
Cc: <stable@vger.kernel.org>
Signed-off-by: default avatarTakashi Iwai <tiwai@suse.de>
parent 9a20332a

sound/core/control.c

+45 −35
Original line number Diff line number Diff line
@@ -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
@@ -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) {

	err = __snd_ctl_add(card, kcontrol);

	up_write(&card->controls_rwsem);

		err = -ENOMEM;

	if (err < 0)

		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;

	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);

	return 0;



 error:
@@ -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);

	/*
@@ -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;

}