[RFC PATCH 2/8] ALSA: control: copy the card bytes outside snd_ioctl_rwsem

[Luca Rodenhäuser]

snd_ctl_card_bytes() holds snd_ioctl_rwsem for reading across its
copy_to_user().  That lock is global to ALSA and is taken for writing from
the card probe path, by snd_component_add().

A user buffer that faults can be made to take arbitrarily long: with
userfaultfd, or with the target buffer mapped from a FUSE file, an
unprivileged process controls when the fault resolves.  While it holds the
reader, a writer from a probing card queues up behind it, and because rwsem
is writer-fair every later reader queues behind the writer.  Card teardown
then waits on the probe path, so an unprivileged reader can stall the
removal of a card it does not own.

Take a copy of the string under the lock, drop the lock, and copy to user
space afterwards.  The extra allocation is a card name sized string on a
path that is not hot.

While at it, report the required length even when the caller's buffer was
too small.  The -ENOMEM return told the caller to try again with a bigger
buffer without telling it how big, and data_len is what the query form of
this ioctl exists for.

Assisted-by: LLM
Signed-off-by: Luca Rodenhäuser <otzelot2021@xxxxxxxxxx>
---
 sound/core/control.c | 46 +++++++++++++++++++++++++++-----------------
 1 file changed, 28 insertions(+), 18 deletions(-)

diff --git a/sound/core/control.c b/sound/core/control.c
index 4199342d4f..e404cb55a5 100644
--- a/sound/core/control.c
+++ b/sound/core/control.c
@@ -14,6 +14,7 @@
 #include <linux/mm.h>
 #include <linux/math64.h>
 #include <linux/sched/signal.h>
+#include <linux/cleanup.h>
 #include <sound/core.h>
 #include <sound/minors.h>
 #include <sound/info.h>
@@ -904,36 +905,45 @@ static int snd_ctl_card_bytes(struct snd_card *card,
 			      struct snd_ctl_card_bytes *info,
 			      unsigned int __user *data_len_out)
 {
+	char *copy __free(kfree) = NULL;
+	bool too_small = false;
 	unsigned int data_len;
 
-	switch (info->type) {
-	case SND_CTL_CARD_BTYPE_COMPONENTS:
-		scoped_guard(rwsem_read, &snd_ioctl_rwsem) {
-			const char *components = card->components;
-
-			if (!components)
-				components = "";
+	if (info->type != SND_CTL_CARD_BTYPE_COMPONENTS)
+		return -EINVAL;
 
-			data_len = strlen(components) + 1;
+	scoped_guard(rwsem_read, &snd_ioctl_rwsem) {
+		const char *components = card->components;
 
-			if (!info->data || info->data_allocated == 0)
-				break;
+		if (!components)
+			components = "";
 
-			if (info->data_allocated < data_len)
-				return -ENOMEM;
+		data_len = strlen(components) + 1;
 
-			if (copy_to_user(u64_to_user_ptr(info->data), components, data_len))
-				return -EFAULT;
+		if (info->data && info->data_allocated != 0) {
+			if (info->data_allocated < data_len) {
+				too_small = true;
+			} else {
+				copy = kmemdup(components, data_len, GFP_KERNEL);
+				if (!copy)
+					return -ENOMEM;
+			}
 		}
-		break;
-	default:
-		return -EINVAL;
 	}
 
+	/* Copy outside the lock.  A user buffer that faults can be made to take
+	 * arbitrarily long (userfaultfd, FUSE), and snd_ioctl_rwsem is global:
+	 * holding it across the copy lets an unprivileged reader block the card
+	 * probe path, and with it a card teardown waiting on that path.
+	 */
+	if (copy && copy_to_user(u64_to_user_ptr(info->data), copy, data_len))
+		return -EFAULT;
+
+	/* report the required size even when the buffer was too small */
 	if (put_user(data_len, data_len_out))
 		return -EFAULT;
 
-	return 0;
+	return too_small ? -ENOMEM : 0;
 }
 
 static int snd_ctl_card_bytes_user(struct snd_card *card,
-- 
2.43.0