Created attachment 21471 [details]
test case to trigger VIDIOC_G_EXT_CTRLS bug

The attached test case was developed as a part of v4l-test 0.15 ( http://v4l-test.sourceforge.net/ ). This simple ioctl() call triggers the mentioned behaviour.

The V4L2 specification revision 0.24 on this topic can be found at http://v4l2spec.bytesex.org/spec/r10386.htm .

It seems the problem comes from the v4l core, not the uvcvideo driver. Both video_usercopy and video_ioctl2 fail to check controls.count correctly, resulting in an integer overflow. Can you reproduce the problem if you set controls.control to NULL and controls.count to a sensible value (let's say 10) ?

On a side note, the stack trace seems to be corrupt.

It seems that controls.count=0x80000000 is a special value necessary to trigger this problem. I tested the following values:

controls.  | return  | errno       | expected  | expected    | test
   count   | value   |             | ret. val. | errno       | result
-----------+---------+-------------+-----------+-------------+--------
0          | 0       | (not set)   | 0         | -           | passed
1          | -1      | 14 (EFAULT) | -1        | 14 (EFAULT) | passed
10         | -1      | 14 (EFAULT) | -1        | 14 (EFAULT) | passed
0x7FFFFFFF | -1      | 12 (ENOMEM) | -1        | 14 (EFAULT) | FAILED
0x80000000 | Segmentation fault    | -1        | 14 (EFAULT) | FAILED
0x80000001 | -1      | 14 (EFAULT) | -1        | 14 (EFAULT) | passed
0x8000000A | -1      | 14 (EFAULT) | -1        | 14 (EFAULT) | passed
0xFFFFFFFF | -1      | 12 (ENOMEM) | -1        | 14 (EFAULT) | FAILED

I would expect EFAULT in all cases where controls.count != 0 .

There is a similar problem with VIDIOC_S_EXT_CTRLS and with VIDIOC_TRY_EXT_CTRLS what is mentioned in comment #1.

Created attachment 21519 [details]
Fix integer overflow when copying extended controls from userspace to kernelspace

I suspect that controls.count = 0x40000000 will crash the kernel as well.

Could you please try the attached patch ?

Is it acceptable to return -ENOMEM instead of -EFAULT when controls.controls is an invalid userspace pointer and controls.count is higher than the amount of memory that can be allocated by kmalloc ? The video_ioctl2 and video_usercopy both allocate memory to copy controls.controls from userspace to kernelspace, and only check is the pointer is valid after allocating the memory. If memory can't be allocated (because controls.count is too high) they just return -ENOMEM.

Restricting controls.count to values smaller than KMALLOC_MAX_SIZE / sizeof(struct v4l2_ext_control) will likely fix the problem you noticed. It might make sense to restrict it even further. Any opinion on that ?

(In reply to comment #5)
> Created an attachment (id=21519) [details]
> Fix integer overflow when copying extended controls from userspace to
> kernelspace
> 
> I suspect that controls.count = 0x40000000 will crash the kernel as well.

I tried the value 0x20000000 which also crashes the kernel.

> Could you please try the attached patch ?

I tested your patch with Linux kernel 2.6.30-rc7 with CNF7129 and with v4l-test 0.15 from http://v4l-test.sourceforge.net/ . The patch fixes the problem.

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

> Is it acceptable to return -ENOMEM instead of -EFAULT when controls.controls
> is
> an invalid userspace pointer and controls.count is higher than the amount of
> memory that can be allocated by kmalloc ? The video_ioctl2 and video_usercopy
> both allocate memory to copy controls.controls from userspace to kernelspace,
> and only check is the pointer is valid after allocating the memory. If memory
> can't be allocated (because controls.count is too high) they just return
> -ENOMEM.

The error value shall point out the cause of the error. The return value -ENOMEM would point to the controls.count so the application programmer would check that field hopefully. The second thing is that the code shall be optimized for normal operation and not for error handling. So I think that the -ENOMEM instead of -EFAULT would be acceptable for me. I implemented v4l-test version 0.15 so that it expects -ENOMEM or -EFAULT.

I don't get it.

If p->count is too large then the kmalloc() will fail and the driver
should have detected that, cleaned things up and returned a failure.

So I suspect that there's a different bug, and this p->count check
will just cover it up?

kmalloc is passed p->count * sizeof(struct v4l2_ext_control). The size of the structure might depend on the architecture but it will always be a multiple of 4 bytes. Large p->count values will trigger an integer overflow that will make kmalloc succeed.

For instance, on x86 struct v4l2_ext_control seems to be 20 bytes long. Setting p->count to 0x80000001 will kmalloc 20 bytes of memory.

p->count = 0x80000000 will result in a different issue. ctrls_size will be equal to 0, and kmalloc will return ZERO_SIZE_PTR (which is equal to ((void *)16)). copy_from_user will succeed, as we ask it to copy 0 bytes, and control will be passed to the driver which will receive bogus values.

ah, OK, right you are.

A nice way to fix this is to use kcalloc(), which has the multiplicative
overflow check built-in.

Unfortunately kcalloc() also zeroes the memory, which is often undesired.  I'd
merge a patch which adds a kcalloc_no_zero(), only I can't think of a good
name for it :(

Was this patch merged to any repository? I am facing with the same problem under Ubuntu 9.10 with the following kernel version:

$ cat /proc/version
Linux version 2.6.31-14-generic (buildd@rothera) (gcc version 4.4.1 (Ubuntu 4.4.1-4ubuntu8) ) #48-Ubuntu SMP Fri Oct 16 14:04:26 UTC 2009

The details about that problem can be found at https://bugs.launchpad.net/ubuntu/+source/linux/+bug/493246 .

No it hasn't been merged, thanks for reminding me. I've just pinged Mauro.