[RFC] VIDIOC_G_EXT_CTRLS does not handle NULL pointer correctly

Commit Message

Laurent Pinchart May 25, 2009, 11:17 a.m. UTC

Hi everybody,

Márton Németh found an integer overflow bug in the extended control ioctl 
handling code. This affects both video_usercopy and video_ioctl2. See 
http://bugzilla.kernel.org/show_bug.cgi?id=13357 for a detailed description of 
the problem.

v4l2_ext_controls::count is not checked explicitly by 
video_usercopy/video_ioctl2. Instead the code tries to allocate 
v4l2_ext_controls::count * sizeof(struct v4l2_ext_control) to copy 
v4l2_ext_controls::controls from userspace to kernelspace, and return an error 
if the memory can't be allocated or if the user pointer is invalid.

The v4l2_ext_controls::count * sizeof(struct v4l2_ext_control) value is stored 
in a 32 bits integer, resulting in an overflow if v4l2_ext_controls::count is 
too high. If the result is smaller than the maximum kmalloc'able size, the 
ioctl call will make it to the device driver, which will likely crash.

The following patch (copied from bugzilla) fixes the problem.


Restricting v4l2_ext_controls::count to values smaller than KMALLOC_MAX_SIZE /
sizeof(struct v4l2_ext_control) should be enough, but we might want to 
restrict the value even further. I'd like opinions on this.

Best regards,

Laurent Pinchart

--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Comments

Mauro Carvalho Chehab May 25, 2009, 2:16 p.m. UTC | #1

Em Mon, 25 May 2009 13:17:02 +0200
Laurent Pinchart <[e-mail]> escreveu:

> Hi everybody,
> 
> Márton Németh found an integer overflow bug in the extended control ioctl 
> handling code. This affects both video_usercopy and video_ioctl2. See 
> http://bugzilla.kernel.org/show_bug.cgi?id=13357 for a detailed description of 
> the problem.
> 

> Restricting v4l2_ext_controls::count to values smaller than KMALLOC_MAX_SIZE /
> sizeof(struct v4l2_ext_control) should be enough, but we might want to 
> restrict the value even further. I'd like opinions on this.

Seems fine to my eyes, but being so close to kmalloc size doesn't seem to be a
good idea. It seems better to choose an arbitrary size big enough to handle all current needs.



Cheers,
Mauro
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Trent Piepho May 25, 2009, 7:22 p.m. UTC | #2

On Mon, 25 May 2009, Laurent Pinchart wrote:
> diff -r e0d881b21bc9 linux/drivers/media/video/v4l2-ioctl.c
> --- a/linux/drivers/media/video/v4l2-ioctl.c	Tue May 19 15:12:17 2009 +0200
> +++ b/linux/drivers/media/video/v4l2-ioctl.c	Sun May 24 18:26:29 2009 +0200
> @@ -402,6 +402,10 @@
>  		   a specific control that caused it. */
>  		p->error_idx = p->count;
>  		user_ptr = (void __user *)p->controls;
> +		if (p->count > KMALLOC_MAX_SIZE / sizeof(p->controls[0])) {
> +			err = -ENOMEM;
> +			goto out_ext_ctrl;
> +		}
>  		if (p->count) {
>  			ctrls_size = sizeof(struct v4l2_ext_control) * p->count;
>  			/* Note: v4l2_ext_controls fits in sbuf[] so mbuf is still NULL. */
> @@ -1859,6 +1863,10 @@
>  		   a specific control that caused it. */
>  		p->error_idx = p->count;
>  		user_ptr = (void __user *)p->controls;
> +		if (p->count > KMALLOC_MAX_SIZE / sizeof(p->controls[0])) {
> +			err = -ENOMEM;
> +			goto out_ext_ctrl;
> +		}
>  		if (p->count) {
>  			ctrls_size = sizeof(struct v4l2_ext_control) * p->count;
>  			/* Note: v4l2_ext_controls fits in sbuf[] so mbuf is still NULL. */
>
> Restricting v4l2_ext_controls::count to values smaller than KMALLOC_MAX_SIZE /
> sizeof(struct v4l2_ext_control) should be enough, but we might want to
> restrict the value even further. I'd like opinions on this.

One thing that could be done is to call access_ok() on the range before
kmalloc'ing a buffer.  If p->count is too high, then it's possible that the
copy_from_user will fail because the process does not have the address
space to copy.
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Laurent Pinchart May 27, 2009, 3:27 p.m. UTC | #3

On Monday 25 May 2009 21:22:06 Trent Piepho wrote:
> On Mon, 25 May 2009, Laurent Pinchart wrote:
> > diff -r e0d881b21bc9 linux/drivers/media/video/v4l2-ioctl.c
> > --- a/linux/drivers/media/video/v4l2-ioctl.c	Tue May 19 15:12:17 2009
> > +0200 +++ b/linux/drivers/media/video/v4l2-ioctl.c	Sun May 24 18:26:29
> > 2009 +0200 @@ -402,6 +402,10 @@
> >  		   a specific control that caused it. */
> >  		p->error_idx = p->count;
> >  		user_ptr = (void __user *)p->controls;
> > +		if (p->count > KMALLOC_MAX_SIZE / sizeof(p->controls[0])) {
> > +			err = -ENOMEM;
> > +			goto out_ext_ctrl;
> > +		}
> >  		if (p->count) {
> >  			ctrls_size = sizeof(struct v4l2_ext_control) * p->count;
> >  			/* Note: v4l2_ext_controls fits in sbuf[] so mbuf is still NULL. */
> > @@ -1859,6 +1863,10 @@
> >  		   a specific control that caused it. */
> >  		p->error_idx = p->count;
> >  		user_ptr = (void __user *)p->controls;
> > +		if (p->count > KMALLOC_MAX_SIZE / sizeof(p->controls[0])) {
> > +			err = -ENOMEM;
> > +			goto out_ext_ctrl;
> > +		}
> >  		if (p->count) {
> >  			ctrls_size = sizeof(struct v4l2_ext_control) * p->count;
> >  			/* Note: v4l2_ext_controls fits in sbuf[] so mbuf is still NULL. */
> >
> > Restricting v4l2_ext_controls::count to values smaller than
> > KMALLOC_MAX_SIZE / sizeof(struct v4l2_ext_control) should be enough, but
> > we might want to restrict the value even further. I'd like opinions on
> > this.
>
> One thing that could be done is to call access_ok() on the range before
> kmalloc'ing a buffer.  If p->count is too high, then it's possible that the
> copy_from_user will fail because the process does not have the address
> space to copy.

arch/x86/include/asm/uaccess.h, about access_ok():

 * Note that, depending on architecture, this function probably just
 * checks that the pointer is in the user space range - after calling
 * this function, memory access functions may still return -EFAULT.

I don't think it's worth it. Let's just kmalloc (or kzalloc) and 
copy_from_user. If one of them fails we'll return an error.

Could a very large number of control requests be used as a DoS attack vector ? 
A userspace application could kmalloc large amounts of memory without any 
restriction. Memory would be reclaimed eventually, but after performing a 
large number of USB requests, which could take quite a long time.

Best regards,

Laurent Pinchart

--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Németh Márton May 28, 2009, 6:44 a.m. UTC | #4

Laurent Pinchart worte:
> Could a very large number of control requests be used as a DoS attack vector ? 
> A userspace application could kmalloc large amounts of memory without any 
> restriction. Memory would be reclaimed eventually, but after performing a 
> large number of USB requests, which could take quite a long time.

A DoS attacker could open the /dev/video0 several times even from one single
process (from different threads) and could kmalloc() as much memory as the
attacker wants. Maybe even one file descriptor would be enough using it from
different threads. This could force the system to swap out pages to get the
necessary memory.

I don't know if more than one instance of the VIDIOC_G_EXT_CTRLS requests can
actively keep memory allocated or only one can run at a time forcing the other
requests to sleep until the previous one hadn't been finished. This is also
true for VIDIOC_S_EXT_CTRLS and VIDIOC_TRY_EXT_CTRLS.

Regards,

	Márton Németh
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Mauro Carvalho Chehab June 10, 2009, 1:52 p.m. UTC | #5

Em Mon, 25 May 2009 11:16:34 -0300
Mauro Carvalho Chehab <[e-mail]> escreveu:

> Em Mon, 25 May 2009 13:17:02 +0200
> Laurent Pinchart <[e-mail]> escreveu:
> 
> > Hi everybody,
> > 
> > Márton Németh found an integer overflow bug in the extended control ioctl 
> > handling code. This affects both video_usercopy and video_ioctl2. See 
> > http://bugzilla.kernel.org/show_bug.cgi?id=13357 for a detailed description of 
> > the problem.
> > 
> 
> > Restricting v4l2_ext_controls::count to values smaller than KMALLOC_MAX_SIZE /
> > sizeof(struct v4l2_ext_control) should be enough, but we might want to 
> > restrict the value even further. I'd like opinions on this.
> 
> Seems fine to my eyes, but being so close to kmalloc size doesn't seem to be a
> good idea. It seems better to choose an arbitrary size big enough to handle all current needs.

I'll apply the current version, but I still think we should restrict it to a lower value.



Cheers,
Mauro
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Mauro Carvalho Chehab June 10, 2009, 1:53 p.m. UTC | #6

Em Wed, 10 Jun 2009 10:52:28 -0300
Mauro Carvalho Chehab <[e-mail]> escreveu:

> Em Mon, 25 May 2009 11:16:34 -0300
> Mauro Carvalho Chehab <[e-mail]> escreveu:
> 
> > Em Mon, 25 May 2009 13:17:02 +0200
> > Laurent Pinchart <[e-mail]> escreveu:
> > 
> > > Hi everybody,
> > > 
> > > Márton Németh found an integer overflow bug in the extended control ioctl 
> > > handling code. This affects both video_usercopy and video_ioctl2. See 
> > > http://bugzilla.kernel.org/show_bug.cgi?id=13357 for a detailed description of 
> > > the problem.
> > > 
> > 
> > > Restricting v4l2_ext_controls::count to values smaller than KMALLOC_MAX_SIZE /
> > > sizeof(struct v4l2_ext_control) should be enough, but we might want to 
> > > restrict the value even further. I'd like opinions on this.
> > 
> > Seems fine to my eyes, but being so close to kmalloc size doesn't seem to be a
> > good idea. It seems better to choose an arbitrary size big enough to handle all current needs.
> 
> I'll apply the current version, but I still think we should restrict it to a lower value.


Hmm... SOB is missing. Márton and Laurent, could you please sign it



Cheers,
Mauro
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Németh Márton June 10, 2009, 3:49 p.m. UTC | #7

Mauro Carvalho Chehab wrote:
> Em Wed, 10 Jun 2009 10:52:28 -0300
> Mauro Carvalho Chehab <[e-mail]> escreveu:
> 
>> Em Mon, 25 May 2009 11:16:34 -0300
>> Mauro Carvalho Chehab <[e-mail]> escreveu:
>>
>>> Em Mon, 25 May 2009 13:17:02 +0200
>>> Laurent Pinchart <[e-mail]> escreveu:
>>>
>>>> Hi everybody,
>>>>
>>>> Márton Németh found an integer overflow bug in the extended control ioctl 
>>>> handling code. This affects both video_usercopy and video_ioctl2. See 
>>>> http://bugzilla.kernel.org/show_bug.cgi?id=13357 for a detailed description of 
>>>> the problem.
>>>>
>>>> Restricting v4l2_ext_controls::count to values smaller than KMALLOC_MAX_SIZE /
>>>> sizeof(struct v4l2_ext_control) should be enough, but we might want to 
>>>> restrict the value even further. I'd like opinions on this.
>>> Seems fine to my eyes, but being so close to kmalloc size doesn't seem to be a
>>> good idea. It seems better to choose an arbitrary size big enough to handle all current needs.
>> I'll apply the current version, but I still think we should restrict it to a lower value.
> 
> 
> Hmm... SOB is missing. Márton and Laurent, could you please sign it

As I wrote at http://bugzilla.kernel.org/show_bug.cgi?id=13357#c6 :

Tested-by: Márton Németh <[e-mail]>

Regards,

	Márton Németh


--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Laurent Pinchart June 10, 2009, 9:58 p.m. UTC | #8

On Wednesday 10 June 2009 15:53:57 Mauro Carvalho Chehab wrote:
> Em Wed, 10 Jun 2009 10:52:28 -0300
>
> Mauro Carvalho Chehab <[e-mail]> escreveu:
> > Em Mon, 25 May 2009 11:16:34 -0300
> >
> > Mauro Carvalho Chehab <[e-mail]> escreveu:
> > > Em Mon, 25 May 2009 13:17:02 +0200
> > >
> > > Laurent Pinchart <[e-mail]> escreveu:
> > > > Hi everybody,
> > > >
> > > > Márton Németh found an integer overflow bug in the extended control
> > > > ioctl handling code. This affects both video_usercopy and
> > > > video_ioctl2. See http://bugzilla.kernel.org/show_bug.cgi?id=13357
> > > > for a detailed description of the problem.
> > > >
> > > >
> > > > Restricting v4l2_ext_controls::count to values smaller than
> > > > KMALLOC_MAX_SIZE / sizeof(struct v4l2_ext_control) should be enough,
> > > > but we might want to restrict the value even further. I'd like
> > > > opinions on this.
> > >
> > > Seems fine to my eyes, but being so close to kmalloc size doesn't seem
> > > to be a good idea. It seems better to choose an arbitrary size big
> > > enough to handle all current needs.
> >
> > I'll apply the current version, but I still think we should restrict it
> > to a lower value.
>
> Hmm... SOB is missing. Márton and Laurent, could you please sign it

Signed-off-by: Laurent Pinchart <[e-mail]>

Cheers,

Laurent Pinchart

--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

diff -r e0d881b21bc9 linux/drivers/media/video/v4l2-ioctl.c
--- a/linux/drivers/media/video/v4l2-ioctl.c	Tue May 19 15:12:17 2009 +0200
+++ b/linux/drivers/media/video/v4l2-ioctl.c	Sun May 24 18:26:29 2009 +0200
@@ -402,6 +402,10 @@ 
 		   a specific control that caused it. */
 		p->error_idx = p->count;
 		user_ptr = (void __user *)p->controls;
+		if (p->count > KMALLOC_MAX_SIZE / sizeof(p->controls[0])) {
+			err = -ENOMEM;
+			goto out_ext_ctrl;
+		}
 		if (p->count) {
 			ctrls_size = sizeof(struct v4l2_ext_control) * p->count;
 			/* Note: v4l2_ext_controls fits in sbuf[] so mbuf is still NULL. */
@@ -1859,6 +1863,10 @@ 
 		   a specific control that caused it. */
 		p->error_idx = p->count;
 		user_ptr = (void __user *)p->controls;
+		if (p->count > KMALLOC_MAX_SIZE / sizeof(p->controls[0])) {
+			err = -ENOMEM;
+			goto out_ext_ctrl;
+		}
 		if (p->count) {
 			ctrls_size = sizeof(struct v4l2_ext_control) * p->count;
 			/* Note: v4l2_ext_controls fits in sbuf[] so mbuf is still NULL. */