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;