[alsa-devel] [patch] ALSA: compress_core: integer overflow in snd_compr_allocate_buffer()
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;
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;
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;
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;
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;
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
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
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
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
On Fri, 2012-09-14 at 11:45 +0200, Takashi Iwai wrote: patch. Can you please push. -- ~Vinod
At Fri, 14 Sep 2012 15:19:08 +0530, Vinod Koul 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
On Fri, 2012-09-14 at 11:45 +0200, Takashi Iwai wrote: patch. Can you please push.
I see. Pushed out, so it'll shortly appear. Takashi
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
participants (3)
-
Dan Carpenter -
Takashi Iwai -
Vinod Koul