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 <[email protected]>

sound/core/compress_offload.c +4 -0

index ec2118d..5a733e7 100644
@@ -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,

Quoted 4 lines · 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.

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

Quoted 19 lines

Signed-off-by Dan Carpenter <[email protected]>

sound/core/compress_offload.c +4 -0

index ec2118d..5a733e7 100644
@@ -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;

Quoted 10 lines · Takashi Iwai

At Wed, 5 Sep 2012 15:32:18 +0300,

Quoted 4 lines · 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.

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.

Quoted 3 lines

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

Quoted 25 lines

thanks,

Takashi

Quoted 19 lines

Signed-off-by Dan Carpenter <[email protected]>

sound/core/compress_offload.c +4 -0

index ec2118d..5a733e7 100644
@@ -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,

Quoted 13 lines · Dan Carpenter

Quoted 10 lines · Takashi Iwai

At Wed, 5 Sep 2012 15:32:18 +0300,

Quoted 4 lines · 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.

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.

Quoted 6 lines

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

Quoted 7 lines

Quoted 3 lines

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

Quoted 30 lines

regards,
dan carpenter

Quoted 25 lines

thanks,

Takashi

Quoted 19 lines

Signed-off-by Dan Carpenter <[email protected]>

sound/core/compress_offload.c +4 -0

index ec2118d..5a733e7 100644
@@ -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;

Quoted 1 line

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?

Quoted 6 lines · Takashi Iwai

Quoted 1 line

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 :)

Quoted 6 lines · Vinod Koul

Quoted 5 lines · Takashi Iwai

Quoted 1 line

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.

Quoted 2 lines

Quoted 1 line

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?

Quoted 4 lines

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.

Quoted 6 lines · Takashi Iwai

Quoted 4 lines

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.

Quoted 6 lines · Takashi Iwai

Quoted 1 line

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 :)