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;