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

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

[Dan Carpenter]

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.

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;

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

[Takashi Iwai]

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


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

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

[Dan Carpenter]

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

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

[Takashi Iwai]

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

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

[Takashi Iwai]

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

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

[Vinod Koul]

On Fri, 2012-09-14 at 11:06 +0200, Takashi Iwai wrote:
> > 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? 
Yes am already on it :)

-- 
~Vinod

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

[Vinod Koul]

On Fri, 2012-09-14 at 14:54 +0530, Vinod Koul wrote:
> On Fri, 2012-09-14 at 11:06 +0200, Takashi Iwai wrote:
> > > 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? 
> Yes am already on it :)
Btw I saw that you took Dan's fix for flags:
ALSA: compress_core: fix open flags test in snd_compr_open()

Did you apply anything for this?
-- 
~Vinod

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

[Dan Carpenter]

On Fri, Sep 14, 2012 at 02:54:42PM +0530, Vinod Koul wrote:
> On Fri, 2012-09-14 at 11:06 +0200, Takashi Iwai wrote:
> > > 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? 
> Yes am already on it :)
> 

Thanks.  I've had my text editor open to the file all week, but it
felt like there was probably a better limit to use than just what
would overflow...  I just don't know the code very well.  I'm glad
that Vinod is handling this.

regards,
dan carpenter

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

[Takashi Iwai]

At Fri, 14 Sep 2012 14:58:54 +0530,
Vinod Koul wrote:
> 
> On Fri, 2012-09-14 at 14:54 +0530, Vinod Koul wrote:
> > On Fri, 2012-09-14 at 11:06 +0200, Takashi Iwai wrote:
> > > > 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? 
> > Yes am already on it :)
> Btw I saw that you took Dan's fix for flags:
> ALSA: compress_core: fix open flags test in snd_compr_open()
> 
> Did you apply anything for this?

You mean anything else?  So far only two patches from Dan.


Takashi

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

[Vinod Koul]

On Fri, 2012-09-14 at 11:45 +0200, Takashi Iwai wrote:
> > Btw I saw that you took Dan's fix for flags:
> > ALSA: compress_core: fix open flags test in snd_compr_open()
> > 
> > Did you apply anything for this?
> 
> You mean anything else?  So far only two patches from Dan.
I meant else :) Your for-next updated 20hours ago doesn't shows only one
patch. Can you please push.

-- 
~Vinod

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

[Takashi Iwai]

At Fri, 14 Sep 2012 15:19:08 +0530,
Vinod Koul wrote:
> 
> On Fri, 2012-09-14 at 11:45 +0200, Takashi Iwai wrote:
> > > Btw I saw that you took Dan's fix for flags:
> > > ALSA: compress_core: fix open flags test in snd_compr_open()
> > > 
> > > Did you apply anything for this?
> > 
> > You mean anything else?  So far only two patches from Dan.
> I meant else :) Your for-next updated 20hours ago doesn't shows only one
> patch. Can you please push.

I see.  Pushed out, so it'll shortly appear.


Takashi