diff mbox series

ALSA: control: Fix racy management of user ctl memory size account

Message ID 20210415131856.13113-1-tiwai@suse.de (mailing list archive)
State Accepted
Commit 998f26f47e556f14cd124c508d76bceb2c3f6e6a
Headers show
Series ALSA: control: Fix racy management of user ctl memory size account | expand

Commit Message

Takashi Iwai April 15, 2021, 1:18 p.m. UTC
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

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 mbox series


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