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@xxxxxxxxxx>
> >
> > 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;
> >
--
To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
