[PATCH 1/2] Staging: bcm: Fix an invalid dereference to a zero length kmalloc in IOCTL_BCM_BULK_WRM
Kevin McKinney
klmckinney1 at gmail.com
Fri Dec 16 14:17:13 UTC 2011
On Fri, Dec 16, 2011 at 4:37 AM, Dan Carpenter <dan.carpenter at oracle.com> wrote:
> On Thu, Dec 15, 2011 at 11:14:53PM -0500, Kevin McKinney wrote:
>> Variable IoBuffer.InputLength is chosen from userspace,
>> and can therefore be zero. If so, then we will get a kernel
>> Oops. To resolve this issue, this patch checks to confirm
>> IoBuffer.InputLength is not equal to zero before invoking
>> the kmalloc call.
>>
>> Signed-off-by: Kevin McKinney <klmckinney1 at gmail.com>
>> ---
>> drivers/staging/bcm/Bcmchar.c | 4 +++-
>> 1 files changed, 3 insertions(+), 1 deletions(-)
>>
>> diff --git a/drivers/staging/bcm/Bcmchar.c b/drivers/staging/bcm/Bcmchar.c
>> index fa4a854..47d6818 100644
>> --- a/drivers/staging/bcm/Bcmchar.c
>> +++ b/drivers/staging/bcm/Bcmchar.c
>> @@ -1137,7 +1137,9 @@ cntrlEnd:
>> if (copy_from_user(&IoBuffer, argp, sizeof(IOCTL_BUFFER)))
>> return -EFAULT;
>>
>> - /* FIXME: restrict length */
>> + if (IoBuffer.InputLength == 0)
>> + return -EINVAL;
>
> The beceem code is not super pretty. We're copying one of these:
>
> typedef struct bulkwrmbuffer
> {
> ULONG Register;
> ULONG SwapEndian;
> ULONG Values[1];
> }BULKWRM_BUFFER,*PBULKWRM_BUFFER;
Yeah, that ULONG Values[1] threw me for a loop. Not sure why the array
of one position is needed. Why not just ULONG Values?
> Values[] is a variable length array. It's not clear to me that
> Values[1] actually stores whole longs. It could be that it's really
> just a buffer of chars. So probably to be conservative we should
> say that we have to at least have the first two members of the
> struct:
>
> if (IoBuffer.InputLength < sizeof(ULONG) * 2)
> return -EINVAL;
>
Make sense. I will resubmit this patch.
Thanks,
Kevin
More information about the devel
mailing list