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: <87r09pynn5.wl-tiwai@suse.de>
Date: Thu, 12 Sep 2024 16:03:58 +0200
From: Takashi Iwai <tiwai@...e.de>
To: Dan Carpenter <dan.carpenter@...aro.org>
Cc: Takashi Iwai <tiwai@...e.de>,
	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 14:44:30 +0200,
Dan Carpenter wrote:
> 
> On Thu, Sep 12, 2024 at 02:29:58PM +0300, Dan Carpenter wrote:
> > 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().
> > 
> 
> I also considered if I should fix this bug by adding checks to
> snd_ctl_check_elem_info() but I don't think that's the right approach.  I
> couldn't see how it would work at least.

OK, so it doesn't seem affected in the end.
The input values have been checked, and they can't overflow.


thanks,

Takashi

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ