[RFC,PATCH] VIDIOC_G_EXT_CTRLS does not handle NULL pointer correctly

[RFC,PATCH] VIDIOC_G_EXT_CTRLS does not handle NULL pointer correctly

[Laurent Pinchart]

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.

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.

Best regards,

Laurent Pinchart

Re: [RFC,PATCH] VIDIOC_G_EXT_CTRLS does not handle NULL pointer correctly

[Mauro Carvalho Chehab]

Em Mon, 25 May 2009 13:17:02 +0200
Laurent Pinchart <laurent.pinchart@skynet.be> 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

Re: [RFC,PATCH] VIDIOC_G_EXT_CTRLS does not handle NULL pointer correctly

[Mauro Carvalho Chehab]

Em Mon, 25 May 2009 11:16:34 -0300
Mauro Carvalho Chehab <mchehab@infradead.org> escreveu:

> Em Mon, 25 May 2009 13:17:02 +0200
> Laurent Pinchart <laurent.pinchart@skynet.be> 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

Re: [RFC,PATCH] VIDIOC_G_EXT_CTRLS does not handle NULL pointer correctly

[Mauro Carvalho Chehab]

Em Wed, 10 Jun 2009 10:52:28 -0300
Mauro Carvalho Chehab <mchehab@infradead.org> escreveu:

> Em Mon, 25 May 2009 11:16:34 -0300
> Mauro Carvalho Chehab <mchehab@infradead.org> escreveu:
> 
> > Em Mon, 25 May 2009 13:17:02 +0200
> > Laurent Pinchart <laurent.pinchart@skynet.be> 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

Re: [RFC,PATCH] VIDIOC_G_EXT_CTRLS does not handle NULL pointer correctly

[Németh Márton]

Mauro Carvalho Chehab wrote:
> Em Wed, 10 Jun 2009 10:52:28 -0300
> Mauro Carvalho Chehab <mchehab@infradead.org> escreveu:
> 
>> Em Mon, 25 May 2009 11:16:34 -0300
>> Mauro Carvalho Chehab <mchehab@infradead.org> escreveu:
>>
>>> Em Mon, 25 May 2009 13:17:02 +0200
>>> Laurent Pinchart <laurent.pinchart@skynet.be> 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 <nm127@freemail.hu>

Regards,

	Márton Németh

Re: [RFC,PATCH] VIDIOC_G_EXT_CTRLS does not handle NULL pointer correctly

[Laurent Pinchart]

On Wednesday 10 June 2009 15:53:57 Mauro Carvalho Chehab wrote:
> Em Wed, 10 Jun 2009 10:52:28 -0300
>
> Mauro Carvalho Chehab <mchehab@infradead.org> escreveu:
> > Em Mon, 25 May 2009 11:16:34 -0300
> >
> > Mauro Carvalho Chehab <mchehab@infradead.org> escreveu:
> > > Em Mon, 25 May 2009 13:17:02 +0200
> > >
> > > Laurent Pinchart <laurent.pinchart@skynet.be> 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 <laurent.pinchart@skynet.be>

Cheers,

Laurent Pinchart

Re: [RFC,PATCH] VIDIOC_G_EXT_CTRLS does not handle NULL pointer correctly

[Laurent Pinchart]

Hi Mauro,

On Wednesday 10 June 2009 23:58:31 Laurent Pinchart wrote:
> On Wednesday 10 June 2009 15:53:57 Mauro Carvalho Chehab wrote:
> > Em Wed, 10 Jun 2009 10:52:28 -0300
> >
> > Mauro Carvalho Chehab <mchehab@infradead.org> escreveu:
> > > Em Mon, 25 May 2009 11:16:34 -0300
> > >
> > > Mauro Carvalho Chehab <mchehab@infradead.org> escreveu:
> > > > Em Mon, 25 May 2009 13:17:02 +0200
> > > >
> > > > Laurent Pinchart <laurent.pinchart@skynet.be> 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 <laurent.pinchart@skynet.be>

Márton reminded me that the patch has still not been applied.

Please replace the above SOB line with

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

-- 
Regards,

Laurent Pinchart

Re: [RFC,PATCH] VIDIOC_G_EXT_CTRLS does not handle NULL pointer correctly

[Németh Márton]

Laurent Pinchart wrote:
> Hi Mauro,
> 
> On Wednesday 10 June 2009 23:58:31 Laurent Pinchart wrote:
>> On Wednesday 10 June 2009 15:53:57 Mauro Carvalho Chehab wrote:
>>> Em Wed, 10 Jun 2009 10:52:28 -0300
>>>
>>> Mauro Carvalho Chehab <mchehab@infradead.org> escreveu:
>>>> Em Mon, 25 May 2009 11:16:34 -0300
>>>>
>>>> Mauro Carvalho Chehab <mchehab@infradead.org> escreveu:
>>>>> Em Mon, 25 May 2009 13:17:02 +0200
>>>>>
>>>>> Laurent Pinchart <laurent.pinchart@skynet.be> 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 <laurent.pinchart@skynet.be>
> 
> Márton reminded me that the patch has still not been applied.
> 
> Please replace the above SOB line with
> 
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

Tested-by: Márton Németh <nm127@freemail.hu>

Re: [RFC,PATCH] VIDIOC_G_EXT_CTRLS does not handle NULL pointer correctly

[Trent Piepho]

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.

Re: [RFC,PATCH] VIDIOC_G_EXT_CTRLS does not handle NULL pointer correctly

[Laurent Pinchart]

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

Re: [RFC,PATCH] VIDIOC_G_EXT_CTRLS does not handle NULL pointer correctly

[Németh Márton]

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

Re: [RFC, PATCH] VIDIOC_G_EXT_CTRLS does not handle NULL pointer correctly

[Hans Verkuil]

> 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.

Perhaps we should limit the number of controls to a maximum of 1024. That
should really be enough :-)

I'm OK with such a limitation.

Regards,

         Hans

-- 
Hans Verkuil - video4linux developer - sponsored by TANDBERG