linux-kernel - Re: [PATCH] ALSA: control: prevent some integer overflow issues

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <87v7z1yyok.wl-tiwai@suse.de>
Date: Thu, 12 Sep 2024 12:05:31 +0200
From: Takashi Iwai <tiwai@...e.de>
To: Dan Carpenter <dan.carpenter@...aro.org>
Cc: Takashi Sakamoto <o-takashi@...amocchi.jp>,
	Jaroslav Kysela <perex@...ex.cz>,
	Takashi Iwai <tiwai@...e.com>,
	Mark Brown <broonie@...nel.org>,
	linux-sound@...r.kernel.org,
	linux-kernel@...r.kernel.org,
	kernel-janitors@...r.kernel.org
Subject: Re: [PATCH] ALSA: control: prevent some integer overflow issues

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@...aro.org>
> ---
>  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
>