On Thu, 12 Sep 2024 10:51:14 +0200,
Dan Carpenter wrote:
>
> I believe the this bug affects 64bit systems as well, but analyzing this
> code is easier if we assume that we're on a 32bit system. The problem is
> in snd_ctl_elem_add() where we do:
>
> sound/core/control.c
> 1669 private_size = value_sizes[info->type] * info->count;
> 1670 alloc_size = compute_user_elem_size(private_size, count);
> ^^^^^
> count is info->owner. It's a non-zero u32 that comes from the user via
> snd_ctl_elem_add_user(). So the math in compute_user_elem_size() could
> have an integer overflow resulting in a smaller than expected size.
So this should also use the overflow macro, too, in addition to your
changes? Something like:
--- a/sound/core/control.c
+++ b/sound/core/control.c
@@ -1618,7 +1618,7 @@ static int snd_ctl_elem_add(struct snd_ctl_file *file,
struct snd_kcontrol *kctl;
unsigned int count;
unsigned int access;
- long private_size;
+ size_t private_size;
size_t alloc_size;
struct user_element *ue;
unsigned int offset;
@@ -1666,7 +1666,7 @@ static int snd_ctl_elem_add(struct snd_ctl_file *file,
/* user-space control doesn't allow zero-size data */
if (info->count < 1)
return -EINVAL;
- private_size = value_sizes[info->type] * info->count;
+ private_size = array_size(value_sizes[info->type], info->count);
alloc_size = compute_user_elem_size(private_size, count);
guard(rwsem_write)(&card->controls_rwsem);
thanks,
Takashi
>
> 1671
> 1672 guard(rwsem_write)(&card->controls_rwsem);
> 1673 if (check_user_elem_overflow(card, alloc_size))
>
> The math is check_user_elem_overflow() can also overflow. Additionally,
> large positive values are cast to negative and thus do not exceed
> max_user_ctl_alloc_size so they are treated as valid. It should be the
> opposite, where negative sizes are invalid.
>
> 1674 return -ENOMEM;
>
> Fixes: 2225e79b9b03 ("ALSA: core: reduce stack usage related to snd_ctl_new()")
> Signed-off-by: Dan Carpenter <dan.carpenter@xxxxxxxxxx>
> ---
> sound/core/control.c | 6 +++---
> 1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/sound/core/control.c b/sound/core/control.c
> index 4f55f64c42e1..f36af27e68d5 100644
> --- a/sound/core/control.c
> +++ b/sound/core/control.c
> @@ -1397,9 +1397,9 @@ struct user_element {
> };
>
> // check whether the addition (in bytes) of user ctl element may overflow the limit.
> -static bool check_user_elem_overflow(struct snd_card *card, ssize_t add)
> +static bool check_user_elem_overflow(struct snd_card *card, size_t add)
> {
> - return (ssize_t)card->user_ctl_alloc_size + add > max_user_ctl_alloc_size;
> + return size_add(card->user_ctl_alloc_size, add) > max_user_ctl_alloc_size;
> }
>
> static int snd_ctl_elem_user_info(struct snd_kcontrol *kcontrol,
> @@ -1593,7 +1593,7 @@ static int snd_ctl_elem_init_enum_names(struct user_element *ue)
>
> static size_t compute_user_elem_size(size_t size, unsigned int count)
> {
> - return sizeof(struct user_element) + size * count;
> + return size_add(sizeof(struct user_element), size_mul(size, count));
> }
>
> static void snd_ctl_elem_user_free(struct snd_kcontrol *kcontrol)
> --
> 2.45.2
>
