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

commit e1a7bfe3807974e66f971f2589d4e0197ec0fced upstream.

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>
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

diff --git a/sound/core/control.c b/sound/core/control.c
index 995cde48c1bedc1777db1fbbb2e81434c27b5143..511368fe974efa58823a60c23419ce3971f3e971 100644 (file)
--- a/sound/core/control.c
+++ b/sound/core/control.c
@@ -346,6 +346,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
@@ -362,45 +396,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:
@@ -1354,9 +1361,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);
        /*
@@ -1367,10 +1377,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;
 }