V4L/DVB: v4l2-ioctl: integer overflow in

PATCH In review Dan Carpenter 4 participants 10 messages Dec 15, 2011 – Jan 5, 2012

Subsystems (2)

Media (V4L/DVB) Kernel janitors

lore

Share & tools

lore

Dan Carpenter

author

2011-12-15 06:34 UTC #0 lore raw

PATCH 1/1 V4L/DVB: v4l2-ioctl: integer overflow in

On a 32bit system the multiplication here could overflow. p->count is used in some of the V4L drivers.

Signed-off-by Dan Carpenter <[email protected]>

---
This is a patch against the 2.6.32-longterm kernel. In the stock kernel, this code was totally rewritten and fixed in 2010 by d14e6d76ebf "[media] v4l: Add multi-planar ioctl handling code".

Hopefully, someone can Ack this and we merge it into the stable tree.

drivers/media/video/v4l2-ioctl.c +6 -0

index 265bfb5..7196303 100644
@@ -414,6 +414,9 @@ video_usercopy(struct file *file, unsigned int cmd, unsigned long arg,
 		p->error_idx = p->count;
 		user_ptr = (void __user *)p->controls;
 		if (p->count) {
+			err = -EINVAL;
+			if (p->count > ULONG_MAX / sizeof(struct v4l2_ext_control))
+				goto out_ext_ctrl;
 			ctrls_size = sizeof(struct v4l2_ext_control) * p->count;
 			/* Note: v4l2_ext_controls fits in sbuf[] so mbuf is still NULL. */
 			mbuf = kmalloc(ctrls_size, GFP_KERNEL);

@@ -1912,6 +1915,9 @@ long video_ioctl2(struct file *file,
 		p->error_idx = p->count;
 		user_ptr = (void __user *)p->controls;
 		if (p->count) {
+			err = -EINVAL;
+			if (p->count > ULONG_MAX / sizeof(struct v4l2_ext_control))
+				goto out_ext_ctrl;
 			ctrls_size = sizeof(struct v4l2_ext_control) * p->count;
 			/* Note: v4l2_ext_controls fits in sbuf[] so mbuf is still NULL. */
 			mbuf = kmalloc(ctrls_size, GFP_KERNEL);

Mauro Carvalho Chehab

2011-12-15 09:21 UTC #1 lore raw

Re: [patch -longterm] V4L/DVB: v4l2-ioctl: integer overflow in video_usercopy()

Quoted 2 lines · Dan Carpenter

On a 32bit system the multiplication here could overflow. p->count is used in some of the V4L drivers.

ULONG_MAX / sizeof(v4l2_ext_control) is too much. This ioctl is used on things like setting MPEG paramenters, where several parameters need adjustments at the same time. I risk to say that 64 is probably a reasonably safe upper limit.

Btw, the upstream code also seems to have the same issue:

static int check_array_args(unsigned int cmd, void *parg, size_t *array_size,
void * __user *user_ptr, void ***kernel_ptr)
{
...

	if (ctrls->count != 0) {

...
*array_size = sizeof(struct v4l2_ext_control)
* ctrls->count;
ret = 1;
...
}

long
video_usercopy(struct file *file, unsigned int cmd, unsigned long arg,
v4l2_kioctl func)
{
...

        err = check_array_args(cmd, parg, &array_size, &user_ptr, &kernel_ptr);
        if (err < 0)
                goto out;
        has_array_args = err;

        if (has_array_args) {
                mbuf = kmalloc(array_size, GFP_KERNEL);

...

so, if is there any overflow at check_array_args(), instead of returning an error to userspace, it will allocate the array with less space than needed.

On both upstream and longterm, I think that it is more reasonable to state a limit for the maximum number of controls that can be passed at the same time, and live with that.

A dummy check says:

$ more include/linux/videodev2.h |grep V4L2_CID|wc -l
    209

So, an upper limit of 256 is enough to allow userspace to change all existing controls at the same time.

The proper way seems to add a define at include/linux/videodev2.h and enforce it at the usercopy code.

Regards,
Mauro

Quoted 33 lines

Signed-off-by Dan Carpenter <[email protected]>

---
This is a patch against the 2.6.32-longterm kernel. In the stock kernel, this code was totally rewritten and fixed in 2010 by d14e6d76ebf "[media] v4l: Add multi-planar ioctl handling code".

Hopefully, someone can Ack this and we merge it into the stable tree.

drivers/media/video/v4l2-ioctl.c +6 -0

index 265bfb5..7196303 100644
@@ -414,6 +414,9 @@ video_usercopy(struct file *file, unsigned int cmd, unsigned long arg,
 		p->error_idx = p->count;
 		user_ptr = (void __user *)p->controls;
 		if (p->count) {
+			err = -EINVAL;
+			if (p->count > ULONG_MAX / sizeof(struct v4l2_ext_control))
+				goto out_ext_ctrl;
 			ctrls_size = sizeof(struct v4l2_ext_control) * p->count;
 			/* Note: v4l2_ext_controls fits in sbuf[] so mbuf is still NULL. */
 			mbuf = kmalloc(ctrls_size, GFP_KERNEL);

@@ -1912,6 +1915,9 @@ long video_ioctl2(struct file *file,
 		p->error_idx = p->count;
 		user_ptr = (void __user *)p->controls;
 		if (p->count) {
+			err = -EINVAL;
+			if (p->count > ULONG_MAX / sizeof(struct v4l2_ext_control))
+				goto out_ext_ctrl;
 			ctrls_size = sizeof(struct v4l2_ext_control) * p->count;
 			/* Note: v4l2_ext_controls fits in sbuf[] so mbuf is still NULL. */
 			mbuf = kmalloc(ctrls_size, GFP_KERNEL);

Hans Verkuil

2011-12-15 09:33 UTC #2 lore raw

Re: [patch -longterm] V4L/DVB: v4l2-ioctl: integer overflow in video_usercopy()

Quoted 7 lines · On Thursday, December 15, 2011 10:21:41 Mauro Carvalho Chehab wrote:

Quoted 2 lines · Dan Carpenter
On a 32bit system the multiplication here could overflow. p->count is used in some of the V4L drivers.
ULONG_MAX / sizeof(v4l2_ext_control) is too much. This ioctl is used on things like setting MPEG paramenters, where several parameters need adjustments at the same time. I risk to say that 64 is probably a reasonably safe upper limit.

Let's make it 1024. That gives more than enough room for expansion without taking too much memory.

Especially for video encoders a lot of controls are needed, and sensor drivers are also getting more complex, so 64 is a bit too low for my taste.

I agree that limiting this to some sensible value is a good idea.

Quoted 36 lines

Btw, the upstream code also seems to have the same issue:
static int check_array_args(unsigned int cmd, void *parg, size_t *array_size,
void * __user *user_ptr, void ***kernel_ptr)
{
...
	if (ctrls->count != 0) {
...
*array_size = sizeof(struct v4l2_ext_control)
* ctrls->count;
ret = 1;
...
}
long
video_usercopy(struct file *file, unsigned int cmd, unsigned long arg,
v4l2_kioctl func)
{
...
        err = check_array_args(cmd, parg, &array_size, &user_ptr, &kernel_ptr);
        if (err < 0)
                goto out;
        has_array_args = err;

        if (has_array_args) {
                mbuf = kmalloc(array_size, GFP_KERNEL);
...
so, if is there any overflow at check_array_args(), instead of returning an error to userspace, it will allocate the array with less space than needed.
On both upstream and longterm, I think that it is more reasonable to state a limit for the maximum number of controls that can be passed at the same time, and live with that.

Quoted 6 lines

A dummy check says:
$ more include/linux/videodev2.h |grep V4L2_CID|wc -l
    209
So, an upper limit of 256 is enough to allow userspace to change all existing controls at the same time.

I would like to have this set to at least twice the number of existing controls (which 1024 certainly is).

It is possible (and valid) to have the same control any number of times in the control list. The last one will 'win' in that case. I can think of (admittedly contrived) scenarios where that might be useful. The only thing we want to do here is to add a sanity check against insane count values.

Quoted 2 lines

The proper way seems to add a define at include/linux/videodev2.h and enforce it at the usercopy code.

I agree.

Regards,

Hans

Quoted 43 lines

Regards,
Mauro
Quoted 24 lines
Signed-off-by Dan Carpenter <[email protected]>
---
This is a patch against the 2.6.32-longterm kernel. In the stock kernel, this code was totally rewritten and fixed in 2010 by d14e6d76ebf "[media] v4l: Add multi-planar ioctl handling code".
Hopefully, someone can Ack this and we merge it into the stable tree.
drivers/media/video/v4l2-ioctl.c +3 -0
index 265bfb5..7196303 100644
@@ -414,6 +414,9 @@ video_usercopy(struct file *file, unsigned int cmd, unsigned long arg,
 		p->error_idx = p->count;
 		user_ptr = (void __user *)p->controls;
 		if (p->count) {
+			err = -EINVAL;
+			if (p->count > ULONG_MAX / sizeof(struct v4l2_ext_control))
+				goto out_ext_ctrl;
 			ctrls_size = sizeof(struct v4l2_ext_control) * p->count;
 			/* Note: v4l2_ext_controls fits in sbuf[] so mbuf is still NULL. */
 			mbuf = kmalloc(ctrls_size, GFP_KERNEL);
drivers/media/video/v4l2-ioctl.c long video_ioctl2(struct file *file, · line 1923
Quoted 9 lines
drivers/media/video/v4l2-ioctl.c +3 -0
@@ -1912,6 +1915,9 @@ long video_ioctl2(struct file *file,
 		p->error_idx = p->count;
 		user_ptr = (void __user *)p->controls;
 		if (p->count) {
+			err = -EINVAL;
+			if (p->count > ULONG_MAX / sizeof(struct v4l2_ext_control))
+				goto out_ext_ctrl;
 			ctrls_size = sizeof(struct v4l2_ext_control) * p->count;
 			/* Note: v4l2_ext_controls fits in sbuf[] so mbuf is still NULL. */
 			mbuf = kmalloc(ctrls_size, GFP_KERNEL);
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Mauro Carvalho Chehab

2011-12-15 09:50 UTC #3 lore raw

Re: [patch -longterm] V4L/DVB: v4l2-ioctl: integer overflow in video_usercopy()

Quoted 9 lines · Hans Verkuil

Quoted 7 lines · On Thursday, December 15, 2011 10:21:41 Mauro Carvalho Chehab wrote:
Quoted 2 lines · Dan Carpenter
On a 32bit system the multiplication here could overflow. p->count is used in some of the V4L drivers.
ULONG_MAX / sizeof(v4l2_ext_control) is too much. This ioctl is used on things like setting MPEG paramenters, where several parameters need adjustments at the same time. I risk to say that 64 is probably a reasonably safe upper limit.

Quoted 7 lines

Let's make it 1024. That gives more than enough room for expansion without taking too much memory.
Especially for video encoders a lot of controls are needed, and sensor drivers are also getting more complex, so 64 is a bit too low for my taste.
I agree that limiting this to some sensible value is a good idea.

I'm fine with 1024. Yet, this could easily be changed to whatever upper value needed, and still be backward compatible.

Quoted 108 lines

Quoted 37 lines
Btw, the upstream code also seems to have the same issue:
static int check_array_args(unsigned int cmd, void *parg, size_t *array_size,
void * __user *user_ptr, void ***kernel_ptr)
{
...
	if (ctrls->count != 0) {
...
*array_size = sizeof(struct v4l2_ext_control)
* ctrls->count;
ret = 1;
...
}
long
video_usercopy(struct file *file, unsigned int cmd, unsigned long arg,
v4l2_kioctl func)
{
...
        err = check_array_args(cmd, parg, &array_size, &user_ptr, &kernel_ptr);
        if (err < 0)
                goto out;
        has_array_args = err;

        if (has_array_args) {
                mbuf = kmalloc(array_size, GFP_KERNEL);
...
so, if is there any overflow at check_array_args(), instead of returning an error to userspace, it will allocate the array with less space than needed.
On both upstream and longterm, I think that it is more reasonable to state a limit for the maximum number of controls that can be passed at the same time, and live with that.
Quoted 6 lines
A dummy check says:
$ more include/linux/videodev2.h |grep V4L2_CID|wc -l
    209
So, an upper limit of 256 is enough to allow userspace to change all existing controls at the same time.
I would like to have this set to at least twice the number of existing controls (which 1024 certainly is).
It is possible (and valid) to have the same control any number of times in the control list. The last one will 'win' in that case. I can think of (admittedly contrived) scenarios where that might be useful. The only thing we want to do here is to add a sanity check against insane count values.
Quoted 2 lines
The proper way seems to add a define at include/linux/videodev2.h and enforce it at the usercopy code.
I agree.
Regards,
Hans
Quoted 34 lines
Regards,
Mauro
Quoted 30 lines
Signed-off-by Dan Carpenter <[email protected]>
---
This is a patch against the 2.6.32-longterm kernel. In the stock kernel, this code was totally rewritten and fixed in 2010 by d14e6d76ebf "[media] v4l: Add multi-planar ioctl handling code".
Hopefully, someone can Ack this and we merge it into the stable tree.
drivers/media/video/v4l2-ioctl.c +6 -0
index 265bfb5..7196303 100644
@@ -414,6 +414,9 @@ video_usercopy(struct file *file, unsigned int cmd, unsigned long arg,
 		p->error_idx = p->count;
 		user_ptr = (void __user *)p->controls;
 		if (p->count) {
+			err = -EINVAL;
+			if (p->count > ULONG_MAX / sizeof(struct v4l2_ext_control))
+				goto out_ext_ctrl;
 			ctrls_size = sizeof(struct v4l2_ext_control) * p->count;
 			/* Note: v4l2_ext_controls fits in sbuf[] so mbuf is still NULL. */
 			mbuf = kmalloc(ctrls_size, GFP_KERNEL);
@@ -1912,6 +1915,9 @@ long video_ioctl2(struct file *file,
 		p->error_idx = p->count;
 		user_ptr = (void __user *)p->controls;
 		if (p->count) {
+			err = -EINVAL;
+			if (p->count > ULONG_MAX / sizeof(struct v4l2_ext_control))
+				goto out_ext_ctrl;
Quoted 9 lines
drivers/media/video/v4l2-ioctl.c video_usercopy(struct file *file, unsigned int cmd, unsigned long arg, · line 422
Quoted 3 lines
drivers/media/video/v4l2-ioctl.c +0 -0
 			ctrls_size = sizeof(struct v4l2_ext_control) * p->count;
 			/* Note: v4l2_ext_controls fits in sbuf[] so mbuf is still NULL. */
 			mbuf = kmalloc(ctrls_size, GFP_KERNEL);
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Greg KH

2012-01-03 20:55 UTC #4 lore raw

Quoted 13 lines · Mauro Carvalho Chehab

Quoted 12 lines · Hans Verkuil
Quoted 7 lines · On Thursday, December 15, 2011 10:21:41 Mauro Carvalho Chehab wrote:
Quoted 2 lines · Dan Carpenter
On a 32bit system the multiplication here could overflow. p->count is used in some of the V4L drivers.
ULONG_MAX / sizeof(v4l2_ext_control) is too much. This ioctl is used on things like setting MPEG paramenters, where several parameters need adjustments at the same time. I risk to say that 64 is probably a reasonably safe upper limit.
Let's make it 1024. That gives more than enough room for expansion without taking too much memory.

Quoted 7 lines

Quoted 4 lines
Especially for video encoders a lot of controls are needed, and sensor drivers are also getting more complex, so 64 is a bit too low for my taste.
I agree that limiting this to some sensible value is a good idea.
I'm fine with 1024. Yet, this could easily be changed to whatever upper value needed, and still be backward compatible.

Ok, can someone please send me the "accepted" version of this patch for inclusion in the 2.6.32-stable tree?

thanks,

greg k-h

Dan Carpenter

author

2012-01-04 13:34 UTC #5 lore raw

Quoted 3 lines · Greg KH

Ok, can someone please send me the "accepted" version of this patch for inclusion in the 2.6.32-stable tree?

Sorry for that. Holidays and all. I'll send a patch tomorrow.

regards,
dan carpenter

Dan Carpenter

author

2012-01-05 06:27 UTC #6 lore raw

PATCH 1/1 V4L/DVB: v4l2-ioctl: integer overflow in

In reply to Greg KH

If ctrls->count is too high the multiplication could overflow and array_size would be lower than expected. Mauro and Hans Verkuil suggested that we cap it at 1024. That comes from the maximum number of controls with lots of room for expantion.

$ grep V4L2_CID include/linux/videodev2.h | wc -l
211

Cc stable <[email protected]>

Signed-off-by Dan Carpenter <[email protected]>

include/linux/videodev2.h +1 -0

index d2f981a..4942f81 100644
@@ -1133,6 +1133,7 @@ struct v4l2_querymenu {
 #define V4L2_CTRL_FLAG_NEXT_CTRL	0x80000000
 
 /*  User-class control IDs defined by V4L2 */
+#define V4L2_CID_MAX_CTRLS		1024
 #define V4L2_CID_BASE			(V4L2_CTRL_CLASS_USER | 0x900)
 #define V4L2_CID_USER_BASE 		V4L2_CID_BASE
 /*  IDs reserved for driver specific controls */

drivers/media/video/v4l2-ioctl.c +4 -0

index e1da8fc..639abee 100644
@@ -2226,6 +2226,10 @@ static int check_array_args(unsigned int cmd, void *parg, size_t *array_size,
 		struct v4l2_ext_controls *ctrls = parg;
 
 		if (ctrls->count != 0) {
+			if (ctrls->count > V4L2_CID_MAX_CTRLS) {
+				ret = -EINVAL;
+				break;
+			}
 			*user_ptr = (void __user *)ctrls->controls;
 			*kernel_ptr = (void *)&ctrls->controls;
 			*array_size = sizeof(struct v4l2_ext_control)

Dan Carpenter

author

2012-01-05 06:27 UTC #7 lore raw

PATCH v2 1/1 V4L/DVB: v4l2-ioctl: integer overflow in

In reply to Greg KH

If p->count is too high the multiplication could overflow and array_size would be lower than expected. Mauro and Hans Verkuil suggested that we cap it at 1024. That comes from the maximum number of controls with lots of room for expantion.

$ grep V4L2_CID include/linux/videodev2.h | wc -l
211

Signed-off-by Dan Carpenter <[email protected]>

include/linux/videodev2.h +1 -0

index b59e78c..9e2088c 100644
@@ -858,6 +858,7 @@ struct v4l2_querymenu {
 #define V4L2_CTRL_FLAG_NEXT_CTRL	0x80000000
 
 /*  User-class control IDs defined by V4L2 */
+#define V4L2_CID_MAX_CTRLS		1024
 #define V4L2_CID_BASE			(V4L2_CTRL_CLASS_USER | 0x900)
 #define V4L2_CID_USER_BASE 		V4L2_CID_BASE
 /*  IDs reserved for driver specific controls */

drivers/media/video/v4l2-ioctl.c +6 -0

index 265bfb5..d7332c7 100644
@@ -414,6 +414,9 @@ video_usercopy(struct file *file, unsigned int cmd, unsigned long arg,
 		p->error_idx = p->count;
 		user_ptr = (void __user *)p->controls;
 		if (p->count) {
+			err = -EINVAL;
+			if (p->count > V4L2_CID_MAX_CTRLS)
+				goto out_ext_ctrl;
 			ctrls_size = sizeof(struct v4l2_ext_control) * p->count;
 			/* Note: v4l2_ext_controls fits in sbuf[] so mbuf is still NULL. */
 			mbuf = kmalloc(ctrls_size, GFP_KERNEL);

@@ -1912,6 +1915,9 @@ long video_ioctl2(struct file *file,
 		p->error_idx = p->count;
 		user_ptr = (void __user *)p->controls;
 		if (p->count) {
+			err = -EINVAL;
+			if (p->count > V4L2_CID_MAX_CTRLS)
+				goto out_ext_ctrl;
 			ctrls_size = sizeof(struct v4l2_ext_control) * p->count;
 			/* Note: v4l2_ext_controls fits in sbuf[] so mbuf is still NULL. */
 			mbuf = kmalloc(ctrls_size, GFP_KERNEL);

Greg KH

2012-01-05 16:43 UTC #8 lore raw

Quoted 9 lines · Dan Carpenter

If p->count is too high the multiplication could overflow and array_size would be lower than expected. Mauro and Hans Verkuil suggested that we cap it at 1024. That comes from the maximum number of controls with lots of room for expantion.
$ grep V4L2_CID include/linux/videodev2.h | wc -l
211
Signed-off-by Dan Carpenter <[email protected]>

So this patch is only for 2.6.32? But the original needs to get into Linus's tree first (which is what I'm guessing the other patch you sent is, right?)

thanks,

greg k-h

Dan Carpenter

author

2012-01-05 17:55 UTC #9 lore raw

Quoted 6 lines · Greg KH

Quoted 5 lines · Dan Carpenter
If p->count is too high the multiplication could overflow and array_size would be lower than expected. Mauro and Hans Verkuil suggested that we cap it at 1024. That comes from the maximum number of controls with lots of room for expantion.

Quoted 8 lines

Quoted 4 lines
$ grep V4L2_CID include/linux/videodev2.h | wc -l
211
Signed-off-by Dan Carpenter <[email protected]>
So this patch is only for 2.6.32? But the original needs to get into Linus's tree first (which is what I'm guessing the other patch you sent is, right?)

Yep.

regards,
dan carpenter