Skip to content

Commit 82262a4

Browse files
larsclausentiwai
authored andcommitted
ALSA: control: Fix replacing user controls
There are two issues with the current implementation for replacing user controls. The first is that the code does not check if the control is actually a user control and neither does it check if the control is owned by the process that tries to remove it. That allows userspace applications to remove arbitrary controls, which can cause a user after free if a for example a driver does not expect a control to be removed from under its feed. The second issue is that on one hand when a control is replaced the user_ctl_count limit is not checked and on the other hand the user_ctl_count is increased (even though the number of user controls does not change). This allows userspace, once the user_ctl_count limit as been reached, to repeatedly replace a control until user_ctl_count overflows. Once that happens new controls can be added effectively bypassing the user_ctl_count limit. Both issues can be fixed by instead of open-coding the removal of the control that is to be replaced to use snd_ctl_remove_user_ctl(). This function does proper permission checks as well as decrements user_ctl_count after the control has been removed. Note that by using snd_ctl_remove_user_ctl() the check which returns -EBUSY at beginning of the function if the control already exists is removed. This is not a problem though since the check is quite useless, because the lock that is protecting the control list is released between the check and before adding the new control to the list, which means that it is possible that a different control with the same settings is added to the list after the check. Luckily there is another check that is done while holding the lock in snd_ctl_add(), so we'll rely on that to make sure that the same control is not added twice. Signed-off-by: Lars-Peter Clausen <lars@metafoo.de> Acked-by: Jaroslav Kysela <perex@perex.cz> Cc: <stable@vger.kernel.org> Signed-off-by: Takashi Iwai <tiwai@suse.de>
1 parent 07f4d9d commit 82262a4

File tree

1 file changed

+9
-16
lines changed

1 file changed

+9
-16
lines changed

Diff for: sound/core/control.c

+9-16
Original file line numberDiff line numberDiff line change
@@ -1154,8 +1154,6 @@ static int snd_ctl_elem_add(struct snd_ctl_file *file,
11541154
struct user_element *ue;
11551155
int idx, err;
11561156

1157-
if (!replace && card->user_ctl_count >= MAX_USER_CONTROLS)
1158-
return -ENOMEM;
11591157
if (info->count < 1)
11601158
return -EINVAL;
11611159
access = info->access == 0 ? SNDRV_CTL_ELEM_ACCESS_READWRITE :
@@ -1164,21 +1162,16 @@ static int snd_ctl_elem_add(struct snd_ctl_file *file,
11641162
SNDRV_CTL_ELEM_ACCESS_TLV_READWRITE));
11651163
info->id.numid = 0;
11661164
memset(&kctl, 0, sizeof(kctl));
1167-
down_write(&card->controls_rwsem);
1168-
_kctl = snd_ctl_find_id(card, &info->id);
1169-
err = 0;
1170-
if (_kctl) {
1171-
if (replace)
1172-
err = snd_ctl_remove(card, _kctl);
1173-
else
1174-
err = -EBUSY;
1175-
} else {
1176-
if (replace)
1177-
err = -ENOENT;
1165+
1166+
if (replace) {
1167+
err = snd_ctl_remove_user_ctl(file, &info->id);
1168+
if (err)
1169+
return err;
11781170
}
1179-
up_write(&card->controls_rwsem);
1180-
if (err < 0)
1181-
return err;
1171+
1172+
if (card->user_ctl_count >= MAX_USER_CONTROLS)
1173+
return -ENOMEM;
1174+
11821175
memcpy(&kctl.id, &info->id, sizeof(info->id));
11831176
kctl.count = info->owner ? info->owner : 1;
11841177
access |= SNDRV_CTL_ELEM_ACCESS_USER;

0 commit comments

Comments
 (0)