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: <20c99f50-948e-4076-ba28-9640c3cd982d@stanley.mountain>
Date: Thu, 12 Sep 2024 14:29:58 +0300
From: Dan Carpenter <dan.carpenter@...aro.org>
To: Takashi Iwai <tiwai@...e.de>
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, Sep 12, 2024 at 12:05:31PM +0200, Takashi Iwai wrote:
> 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);
> 

I've reviewed this some more and those changes are harmless but unnecessary.
info->count is checked in snd_ctl_check_elem_info().


regards,
dan carpenter