On Fri, Dec 16, 2011 at 09:17:13AM -0500, Kevin McKinney wrote:
> On Fri, Dec 16, 2011 at 4:37 AM, Dan Carpenter <dan.carpenter@xxxxxxxxxx> 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@xxxxxxxxx>
> >> ---
> >> 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?
It's not an really array of one. The number of elements depends on
the size of IoBuffer.InputLength. That bit is normal.
The other thing is that longs are 32 bits or 64 bits but
IoBuffer.InputLength is given in chars. It could be that really the
user does pass unsigned longs... You never know.
regards,
dan carpenter
Attachment:
signature.asc
Description: Digital signature
_______________________________________________ devel mailing list devel@xxxxxxxxxxxxxxxxxxxxxx http://driverdev.linuxdriverproject.org/mailman/listinfo/devel
