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

[Dan Carpenter <dan.carpenter@oracle.com>]

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.

> 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.

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;
> >