Re: [PATCH] ALSA: control: expand limitation on the number of user-defined control element set per card

[Takashi Iwai <tiwai@suse.de>]

On Tue, 26 Jan 2021 17:41:36 +0100,
Takashi Iwai wrote:
> 
> On Tue, 26 Jan 2021 17:25:24 +0100,
> Jaroslav Kysela wrote:
> > 
> > Dne 26. 01. 21 v 16:57 Takashi Iwai napsal(a):
> > 
> > > @@ -1226,11 +1227,18 @@ struct user_element {
> > >  	struct snd_card *card;
> > >  	char *elem_data;		/* element data */
> > >  	unsigned long elem_data_size;	/* size of element data in bytes */
> > > +	size_t alloc_size;		/* allocated size */
> > 
> > I think that introduction of this new member is not required. The allocated
> > size can be easily computed at runtime (function).
> 
> True.  OTOH, I thought this addition would be error-prone.
> 
> > > @@ -1397,6 +1414,7 @@ static int snd_ctl_elem_init_enum_names(struct user_element *ue)
> > >  
> > >  	ue->priv_data = names;
> > >  	ue->info.value.enumerated.names_ptr = 0;
> > > +	ue->alloc_size += buf_len;
> > 
> > The buf_len variable is the remaining count. The
> > ue->info.value.enumerated.names_length already contains allocated bytes. This
> > code can be elimited if alloc_size is not introduced.
> 
> Oh yeah, that's a wrong count.

The v2 patch is below.


thanks,

Takashi

-- 8< --
From: Takashi Iwai <tiwai@suse.de>
Subject: [PATCH v2] ALSA: control: Add memory consumption limit to user
 controls

ALSA control interface allows users to add arbitrary control elements
(called "user controls" or "user elements"), and its resource usage is
limited just by the max number of control sets (currently 32).  This
limit, however, is quite loose: each allocation of control set may
have 1028 elements, and each element may have up to 512 bytes of value
data.  Moreover, each control set may contain the enum strings and TLV
data, which can be up to 64kB and 128kB, respectively.  Totally, the
whole memory consumption may go over 38MB -- it's quite large, and
we'd rather like to reduce the size.

OTOH, there have been other requests even to increase the max number
of user elements; e.g. FireWire stack require the more user controls,
hence we want to raise the bar, too.

For satisfying both requirements, this patch changes the management of
user controls: instead of setting the upper limit of the number of
user controls, we check the actual memory allocation size and set the
upper limit of the total allocation in bytes.  As long as the memory
consumption stays below the limit, more user controls are allowed than
the current limit 32.  At the same time, we set the lower limit (8MB)
as default than the current theoretical limit, in order to lower the
risk of DoS.

As a compromise for lowering the default limit, now the actual memory
limit is defined as a module option, max_user_ctl, so that user can
increase/decrease the limit if really needed, too.

Signed-off-by: Takashi Iwai <tiwai@suse.de>
---
v1->v2: Drop alloc_size field from user_element, calculate at private_free

 include/sound/core.h |  2 +-
 sound/core/control.c | 73 ++++++++++++++++++++++++++++++--------------
 2 files changed, 51 insertions(+), 24 deletions(-)

diff --git a/include/sound/core.h b/include/sound/core.h
index 0462c577d7a3..a21b14451810 100644
--- a/include/sound/core.h
+++ b/include/sound/core.h
@@ -100,7 +100,7 @@ struct snd_card {
 	struct rw_semaphore controls_rwsem;	/* controls list lock */
 	rwlock_t ctl_files_rwlock;	/* ctl_files list lock */
 	int controls_count;		/* count of all controls */
-	int user_ctl_count;		/* count of all user controls */
+	size_t user_ctl_alloc_size;	/* current memory allocation by user controls */
 	struct list_head controls;	/* all controls for this card */
 	struct list_head ctl_files;	/* active control files */
 
diff --git a/sound/core/control.c b/sound/core/control.c
index 5165741a8400..1a9a3115e4a3 100644
--- a/sound/core/control.c
+++ b/sound/core/control.c
@@ -7,6 +7,7 @@
 #include <linux/threads.h>
 #include <linux/interrupt.h>
 #include <linux/module.h>
+#include <linux/moduleparam.h>
 #include <linux/slab.h>
 #include <linux/vmalloc.h>
 #include <linux/time.h>
@@ -18,10 +19,13 @@
 #include <sound/info.h>
 #include <sound/control.h>
 
-/* max number of user-defined controls */
-#define MAX_USER_CONTROLS	32
 #define MAX_CONTROL_COUNT	1028
 
+/* Max allocation size for user controls */
+static int max_user_ctl_alloc_size = 8 * 1024 * 1024;
+module_param_named(max_user_ctl, max_user_ctl_alloc_size, int, 0444);
+MODULE_PARM_DESC(max_user_ctl, "Max allocation size for user controls");
+
 struct snd_kctl_ioctl {
 	struct list_head list;		/* list of all ioctls */
 	snd_kctl_ioctl_func_t fioctl;
@@ -537,9 +541,6 @@ static int snd_ctl_remove_user_ctl(struct snd_ctl_file * file,
 			goto error;
 		}
 	ret = snd_ctl_remove(card, kctl);
-	if (ret < 0)
-		goto error;
-	card->user_ctl_count--;
 error:
 	up_write(&card->controls_rwsem);
 	return ret;
@@ -1231,6 +1232,12 @@ struct user_element {
 	void *priv_data;		/* private data (like strings for enumerated type) */
 };
 
+/* check whether the addition (in bytes) of user ctl element may overflow the limit */
+static bool user_elem_overflow(struct snd_card *card, ssize_t add)
+{
+	return (ssize_t)card->user_ctl_alloc_size + add >= max_user_ctl_alloc_size;
+}
+
 static int snd_ctl_elem_user_info(struct snd_kcontrol *kcontrol,
 				  struct snd_ctl_elem_info *uinfo)
 {
@@ -1309,6 +1316,10 @@ static int replace_user_tlv(struct snd_kcontrol *kctl, unsigned int __user *buf,
 	if (size > 1024 * 128)	/* sane value */
 		return -EINVAL;
 
+	/* does the TLV size change cause overflow? */
+	if (user_elem_overflow(ue->card, (ssize_t)(size - ue->tlv_data_size)))
+		return -ENOMEM;
+
 	container = vmemdup_user(buf, size);
 	if (IS_ERR(container))
 		return PTR_ERR(container);
@@ -1326,11 +1337,15 @@ static int replace_user_tlv(struct snd_kcontrol *kctl, unsigned int __user *buf,
 		for (i = 0; i < kctl->count; ++i)
 			kctl->vd[i].access |= SNDRV_CTL_ELEM_ACCESS_TLV_READ;
 		mask = SNDRV_CTL_EVENT_MASK_INFO;
+	} else {
+		ue->card->user_ctl_alloc_size -= ue->tlv_data_size;
+		ue->tlv_data_size = 0;
+		kvfree(ue->tlv_data);
 	}
 
-	kvfree(ue->tlv_data);
 	ue->tlv_data = container;
 	ue->tlv_data_size = size;
+	ue->card->user_ctl_alloc_size += size; /* decremented at private_free */
 
 	mask |= SNDRV_CTL_EVENT_MASK_TLV;
 	for (i = 0; i < kctl->count; ++i) {
@@ -1374,16 +1389,17 @@ static int snd_ctl_elem_init_enum_names(struct user_element *ue)
 	unsigned int i;
 	const uintptr_t user_ptrval = ue->info.value.enumerated.names_ptr;
 
-	if (ue->info.value.enumerated.names_length > 64 * 1024)
+	buf_len = ue->info.value.enumerated.names_length;
+	if (buf_len > 64 * 1024)
 		return -EINVAL;
 
-	names = vmemdup_user((const void __user *)user_ptrval,
-		ue->info.value.enumerated.names_length);
+	if (user_elem_overflow(ue->card, buf_len))
+		return -ENOMEM;
+	names = vmemdup_user((const void __user *)user_ptrval, buf_len);
 	if (IS_ERR(names))
 		return PTR_ERR(names);
 
 	/* check that there are enough valid names */
-	buf_len = ue->info.value.enumerated.names_length;
 	p = names;
 	for (i = 0; i < ue->info.value.enumerated.items; ++i) {
 		name_len = strnlen(p, buf_len);
@@ -1397,14 +1413,26 @@ static int snd_ctl_elem_init_enum_names(struct user_element *ue)
 
 	ue->priv_data = names;
 	ue->info.value.enumerated.names_ptr = 0;
+	/* increment the allocation size; decremented again at private_free */
+	ue->card->user_ctl_alloc_size += ue->info.value.enumerated.names_length;
 
 	return 0;
 }
 
+#define user_elem_size(size, count) \
+	(sizeof(struct user_element) + (size) * count)
+
 static void snd_ctl_elem_user_free(struct snd_kcontrol *kcontrol)
 {
 	struct user_element *ue = kcontrol->private_data;
 
+	/* decrement the allocation size */
+	ue->card->user_ctl_alloc_size -= user_elem_size(ue->elem_data_size,
+							kcontrol->count);
+	ue->card->user_ctl_alloc_size -= ue->tlv_data_size;
+	if (ue->priv_data)
+		ue->card->user_ctl_alloc_size -= ue->info.value.enumerated.names_length;
+
 	kvfree(ue->tlv_data);
 	kvfree(ue->priv_data);
 	kfree(ue);
@@ -1418,6 +1446,7 @@ static int snd_ctl_elem_add(struct snd_ctl_file *file,
 	unsigned int count;
 	unsigned int access;
 	long private_size;
+	size_t alloc_size;
 	struct user_element *ue;
 	unsigned int offset;
 	int err;
@@ -1435,13 +1464,6 @@ static int snd_ctl_elem_add(struct snd_ctl_file *file,
 			return err;
 	}
 
-	/*
-	 * The number of userspace controls are counted control by control,
-	 * not element by element.
-	 */
-	if (card->user_ctl_count + 1 > MAX_USER_CONTROLS)
-		return -ENOMEM;
-
 	/* Check the number of elements for this userspace control. */
 	count = info->owner;
 	if (count == 0)
@@ -1472,6 +1494,11 @@ static int snd_ctl_elem_add(struct snd_ctl_file *file,
 	if (info->count < 1)
 		return -EINVAL;
 	private_size = value_sizes[info->type] * info->count;
+	alloc_size = user_elem_size(private_size, count);
+
+	/* Check the memory allocation limit */
+	if (user_elem_overflow(card, alloc_size))
+		return -ENOMEM;
 
 	/*
 	 * Keep memory object for this userspace control. After passing this
@@ -1483,16 +1510,18 @@ static int snd_ctl_elem_add(struct snd_ctl_file *file,
 	if (err < 0)
 		return err;
 	memcpy(&kctl->id, &info->id, sizeof(kctl->id));
-	kctl->private_data = kzalloc(sizeof(struct user_element) + private_size * count,
-				     GFP_KERNEL);
-	if (kctl->private_data == NULL) {
+	ue = kzalloc(alloc_size, GFP_KERNEL);
+	if (!ue) {
 		kfree(kctl);
 		return -ENOMEM;
 	}
+	kctl->private_data = ue;
 	kctl->private_free = snd_ctl_elem_user_free;
 
+	/* increment the allocated size; decremented again at private_free */
+	card->user_ctl_alloc_size += alloc_size;
+
 	/* Set private data for this userspace control. */
-	ue = (struct user_element *)kctl->private_data;
 	ue->card = card;
 	ue->info = *info;
 	ue->info.access = 0;
@@ -1535,8 +1564,6 @@ static int snd_ctl_elem_add(struct snd_ctl_file *file,
 	 * which locks the element.
 	 */
 
-	card->user_ctl_count++;
-
  unlock:
 	up_write(&card->controls_rwsem);
 	return err;
-- 
2.26.2