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

[Dan Carpenter]

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.

regards,
dan carpenter