At Thu, 06 Sep 2012 17:19:07 +0200,
Takashi Iwai wrote:
>
> 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.
I took Dan's patch now for next branch. But it's still better to
filter weird parameters in the caller side, too.
Vinod, care to write such a patch and submit?
thanks,
Takashi
>
>
> thanks,
>
> Takashi
>
>
> >
> > 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
