We've got a report about the possible race in the user control element counts (card->user_ctl_count), and it was confirmed that the race wasn't serious in the old code up to 5.12. There, the value modification itself was exclusive and protected via a write semaphore, hence it's at most concurrent reads and evaluations before the increment. Since it's only about the soft-limit to avoid the exhausting memory usage, one-off isn't a big problem at all.
Meanwhile, the relevant code has been largely modified recently, and now card->user_ctl_count was replaced with card->user_ctl_alloc_size, and a few more places were added to access this field. And, in this new code, it turned out to be more serious: the modifications are scattered in various places, and a few of them are without protection. It implies that it may lead to an inconsistent value by racy accesses.
For addressing it, this patch extends the range covered by the card->controls_rwsem write lock at snd_ctl_elem_add() so that the all code paths that modify and refer to card->user_ctl_alloc_size are protected by the rwsem properly.
The patch adds also comments in a couple of functions to indicate that they are under the rwsem lock.
Fixes: 66c6d1ef86ff ("ALSA: control: Add memory consumption limit to user controls") Link: https://lore.kernel.org/r/FEEBF384-44BE-42CF-8FB3-93470933F64F@purdue.edu Signed-off-by: Takashi Iwai tiwai@suse.de --- sound/core/control.c | 17 +++++++++++------ 1 file changed, 11 insertions(+), 6 deletions(-)
diff --git a/sound/core/control.c b/sound/core/control.c index a076c08c21b6..498e3701514a 100644 --- a/sound/core/control.c +++ b/sound/core/control.c @@ -1337,6 +1337,7 @@ static int snd_ctl_elem_user_put(struct snd_kcontrol *kcontrol, return change; }
+/* called in controls_rwsem write lock */ static int replace_user_tlv(struct snd_kcontrol *kctl, unsigned int __user *buf, unsigned int size) { @@ -1414,6 +1415,7 @@ static int snd_ctl_elem_user_tlv(struct snd_kcontrol *kctl, int op_flag, return read_user_tlv(kctl, buf, size); }
+/* called in controls_rwsem write lock */ static int snd_ctl_elem_init_enum_names(struct user_element *ue) { char *names, *p; @@ -1529,8 +1531,11 @@ static int snd_ctl_elem_add(struct snd_ctl_file *file, private_size = value_sizes[info->type] * info->count; alloc_size = compute_user_elem_size(private_size, count);
- if (check_user_elem_overflow(card, alloc_size)) - return -ENOMEM; + down_write(&card->controls_rwsem); + if (check_user_elem_overflow(card, alloc_size)) { + err = -ENOMEM; + goto unlock; + }
/* * Keep memory object for this userspace control. After passing this @@ -1540,12 +1545,13 @@ static int snd_ctl_elem_add(struct snd_ctl_file *file, */ err = snd_ctl_new(&kctl, count, access, file); if (err < 0) - return err; + goto unlock; memcpy(&kctl->id, &info->id, sizeof(kctl->id)); ue = kzalloc(alloc_size, GFP_KERNEL); if (!ue) { kfree(kctl); - return -ENOMEM; + err = -ENOMEM; + goto unlock; } kctl->private_data = ue; kctl->private_free = snd_ctl_elem_user_free; @@ -1563,7 +1569,7 @@ static int snd_ctl_elem_add(struct snd_ctl_file *file, err = snd_ctl_elem_init_enum_names(ue); if (err < 0) { snd_ctl_free_one(kctl); - return err; + goto unlock; } }
@@ -1580,7 +1586,6 @@ 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. */ - down_write(&card->controls_rwsem); err = __snd_ctl_add_replace(card, kctl, CTL_ADD_EXCLUSIVE); if (err < 0) { snd_ctl_free_one(kctl);