Re: [patch] ALSA: compress_core: integer overflow in snd_compr_allocate_buffer()

[Takashi Iwai <tiwai@suse.de>]

At Thu, 6 Sep 2012 07:59:13 -0700,
Dan Carpenter wrote:
> 
> On Wed, Sep 05, 2012 at 03:40:06PM +0200, Takashi Iwai wrote:
> > At Wed, 5 Sep 2012 15:32:18 +0300,
> > Dan Carpenter wrote:
> > > 
> > > These are 32 bit values that come from the user, we need to check for
> > > integer overflows or we could end up allocating a smaller buffer than
> > > expected.
> > 
> > The buffer size here is supposed to be fairly small that kmalloc can
> > handle.  So, the overflow check is good, but in practice it'd return
> > -ENOMEM.
> 
> My concern was the security implications from an integer wrap.  If
> we chose ->fragment_size = 256 and ->fragments = 0x80000001 then the
> size of the final buffer would only be 256 bytes.  The allocation
> would succeed and it might lead to memory corruption later on.  I
> haven't followed it through to verify but adding a sanity check is a
> good idea.  It should probably be pushed to -stable as well.

Yeah, a fix is really needed.  But, note that this API hasn't been
used by any driver yet in the released upstream kernels, so the impact
to the real world is pretty close to null.  Thus I don't know whether
it's worth for stable kernel, too.

(The real driver implementation appears first in 3.7 kernel, BTW.)

> > Of course, it's fine to put the sanity check, but such
> > checks could be better peformed in snd_compr_set_params() before
> > calling the allocation, I think.
> 
> To me it looks sort of weird to do the checking there.  Also if we
> add more callers we would have to add the checking to all the
> callers as well.  I can do that if you still prefer.

Well, there are two issues here: the integer overflow of buffer size
and the invalid parameters.  I agree that checking the integer
overflow can be there as well in a safer side.  OTOH, the check of
invalid parameters should be added definitely.  There might be more
other places do behave more badly by such parameters even if the
values don't exceed the integer max.

So, we actually should have two distinct fixes.


thanks,

Takashi


> 
> regards,
> dan carpenter
> 
> > 
> > 
> > thanks,
> > 
> > Takashi
> > 
> > > 
> > > Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> > > 
> > > diff --git a/sound/core/compress_offload.c b/sound/core/compress_offload.c
> > > index ec2118d..5a733e7 100644
> > > --- a/sound/core/compress_offload.c
> > > +++ b/sound/core/compress_offload.c
> > > @@ -409,6 +409,10 @@ static int snd_compr_allocate_buffer(struct snd_compr_stream *stream,
> > >  	unsigned int buffer_size;
> > >  	void *buffer;
> > >  
> > > +	if (params->buffer.fragment_size = 0 ||
> > > +	    params->buffer.fragments > SIZE_MAX / params->buffer.fragment_size)
> > > +		return -EINVAL;
> > > +
> > >  	buffer_size = params->buffer.fragment_size * params->buffer.fragments;
> > >  	if (stream->ops->copy) {
> > >  		buffer = NULL;
> > > 
>