Alsa-Devel Archive on lore.kernel.org
 help / color / Atom feed
From: Takashi Iwai <tiwai@suse.de>
To: alsa-devel@alsa-project.org
Cc: "Gong, Sishuai" <sishuai@purdue.edu>
Subject: [PATCH] ALSA: control: Fix racy management of user ctl memory size account
Date: Thu, 15 Apr 2021 15:18:56 +0200
Message-ID: <20210415131856.13113-1-tiwai@suse.de> (raw)

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


                 reply index

Thread overview: [no followups] expand[flat|nested]  mbox.gz  Atom feed

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20210415131856.13113-1-tiwai@suse.de \
    --to=tiwai@suse.de \
    --cc=alsa-devel@alsa-project.org \
    --cc=sishuai@purdue.edu \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Alsa-Devel Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/alsa-devel/0 alsa-devel/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 alsa-devel alsa-devel/ https://lore.kernel.org/alsa-devel \
		alsa-devel@alsa-project.org
	public-inbox-index alsa-devel

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.alsa-project.alsa-devel


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git