Re: [SECURITY] CAN-2004-0075

[Michal Schmidt]

Greg KH wrote:
> On Wed, Apr 14, 2004 at 10:30:33PM +0200, Marc-Christian Petersen wrote:
> > Okay, now while we are at fixing security holes, is there any chance we 
> > can _finally_ get the attached patch in?
> >
> > The Vicam USB driver in all Linux Kernels 2.6 mainline does not use the 
> > copy_from_user function when copying data from userspace to kernel space, 
> > which crosses security boundaries and allows local users to cause a denial
> > of service.
> >
> > Already ACKed by Greg. Only complaint was inproper coding style which is done 
> > with attached patch ;)
>
>
> Eeek, I thought this one was already in the tree, very sorry about that.
>
> I'm applying it now and will send it to Linus in a bit.

The patch broke compilation with VICAM_DEBUG on.
There is also another copy_from_user missing in case VIDIOCSPICT.
I'm attaching a patch.

Michal Schmidt
--- linux-2.6.6-rc1/drivers/usb/media/vicam.c	2004-04-15 11:18:18.000000000 +0200
+++ linux-2.6.6-rc1-mich/drivers/usb/media/vicam.c	2004-04-15 11:50:02.791604312 +0200
@@ -612,15 +612,20 @@ vicam_ioctl(struct inode *inode, struct 
 
 	case VIDIOCSPICT:
 		{
-			struct video_picture *vp = (struct video_picture *) arg;
-
-			DBG("VIDIOCSPICT depth = %d, pal = %d\n", vp->depth,
-			    vp->palette);
+			struct video_picture vp;
+			
+			if (copy_from_user(&vp, arg, sizeof(vp))) {
+				retval = -EFAULT;
+				break;
+			}
+			
+			DBG("VIDIOCSPICT depth = %d, pal = %d\n", vp.depth,
+			    vp.palette);
 
-			cam->gain = vp->brightness >> 8;
+			cam->gain = vp.brightness >> 8;
 
-			if (vp->depth != 24
-			    || vp->palette != VIDEO_PALETTE_RGB24)
+			if (vp.depth != 24
+			    || vp.palette != VIDEO_PALETTE_RGB24)
 				retval = -EINVAL;
 
 			break;
@@ -660,7 +665,7 @@ vicam_ioctl(struct inode *inode, struct 
 				break;
 			}
 
-			DBG("VIDIOCSWIN %d x %d\n", vw->width, vw->height);
+			DBG("VIDIOCSWIN %d x %d\n", vw.width, vw.height);
 			
 			if ( vw.width != 320 || vw.height != 240 )
 				retval = -EFAULT;