* [patch -longterm] V4L/DVB: v4l2-ioctl: integer overflow in video_usercopy()
@ 2011-12-15 6:34 Dan Carpenter
2011-12-15 9:21 ` Mauro Carvalho Chehab
0 siblings, 1 reply; 10+ messages in thread
From: Dan Carpenter @ 2011-12-15 6:34 UTC (permalink / raw)
To: linux-media, Mauro Carvalho Chehab; +Cc: kernel-janitors, stable
On a 32bit system the multiplication here could overflow. p->count is
used in some of the V4L drivers.
Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
---
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.
diff --git a/drivers/media/video/v4l2-ioctl.c b/drivers/media/video/v4l2-ioctl.c
index 265bfb5..7196303 100644
--- a/drivers/media/video/v4l2-ioctl.c
+++ b/drivers/media/video/v4l2-ioctl.c
@@ -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);
^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [patch -longterm] V4L/DVB: v4l2-ioctl: integer overflow in video_usercopy()
2011-12-15 6:34 [patch -longterm] V4L/DVB: v4l2-ioctl: integer overflow in video_usercopy() Dan Carpenter
@ 2011-12-15 9:21 ` Mauro Carvalho Chehab
2011-12-15 9:33 ` Hans Verkuil
0 siblings, 1 reply; 10+ messages in thread
From: Mauro Carvalho Chehab @ 2011-12-15 9:21 UTC (permalink / raw)
To: Dan Carpenter; +Cc: linux-media, kernel-janitors, stable, Hans Verkuil
On 15-12-2011 04:34, Dan Carpenter wrote:
> 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
>
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> ---
> 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.
>
> diff --git a/drivers/media/video/v4l2-ioctl.c b/drivers/media/video/v4l2-ioctl.c
> index 265bfb5..7196303 100644
> --- a/drivers/media/video/v4l2-ioctl.c
> +++ b/drivers/media/video/v4l2-ioctl.c
> @@ -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);
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [patch -longterm] V4L/DVB: v4l2-ioctl: integer overflow in video_usercopy()
2011-12-15 9:21 ` Mauro Carvalho Chehab
@ 2011-12-15 9:33 ` Hans Verkuil
2011-12-15 9:50 ` Mauro Carvalho Chehab
0 siblings, 1 reply; 10+ messages in thread
From: Hans Verkuil @ 2011-12-15 9:33 UTC (permalink / raw)
To: Mauro Carvalho Chehab; +Cc: Dan Carpenter, linux-media, kernel-janitors, stable
On Thursday, December 15, 2011 10:21:41 Mauro Carvalho Chehab wrote:
> On 15-12-2011 04:34, Dan Carpenter wrote:
> > 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.
> 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.
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.
> The proper way seems to add a define at include/linux/videodev2.h
> and enforce it at the usercopy code.
I agree.
Regards,
Hans
>
> Regards,
> Mauro
>
> >
> > Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> > ---
> > 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.
> >
> > diff --git a/drivers/media/video/v4l2-ioctl.c b/drivers/media/video/v4l2-ioctl.c
> > index 265bfb5..7196303 100644
> > --- a/drivers/media/video/v4l2-ioctl.c
> > +++ b/drivers/media/video/v4l2-ioctl.c
> > @@ -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);
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-media" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [patch -longterm] V4L/DVB: v4l2-ioctl: integer overflow in video_usercopy()
2011-12-15 9:33 ` Hans Verkuil
@ 2011-12-15 9:50 ` Mauro Carvalho Chehab
2012-01-03 20:55 ` Greg KH
0 siblings, 1 reply; 10+ messages in thread
From: Mauro Carvalho Chehab @ 2011-12-15 9:50 UTC (permalink / raw)
To: Hans Verkuil; +Cc: Dan Carpenter, linux-media, kernel-janitors, stable
On 15-12-2011 07:33, Hans Verkuil wrote:
> On Thursday, December 15, 2011 10:21:41 Mauro Carvalho Chehab wrote:
>> On 15-12-2011 04:34, Dan Carpenter wrote:
>>> 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.
I'm fine with 1024. Yet, this could easily be changed to whatever upper value needed,
and still be backward compatible.
>
>> 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.
>
> 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.
>
>> The proper way seems to add a define at include/linux/videodev2.h
>> and enforce it at the usercopy code.
>
> I agree.
>
> Regards,
>
> Hans
>
>>
>> Regards,
>> Mauro
>>
>>>
>>> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
>>> ---
>>> 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.
>>>
>>> diff --git a/drivers/media/video/v4l2-ioctl.c b/drivers/media/video/v4l2-ioctl.c
>>> index 265bfb5..7196303 100644
>>> --- a/drivers/media/video/v4l2-ioctl.c
>>> +++ b/drivers/media/video/v4l2-ioctl.c
>>> @@ -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);
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-media" in
>> the body of a message to majordomo@vger.kernel.org
>> 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 majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [patch -longterm] V4L/DVB: v4l2-ioctl: integer overflow in video_usercopy()
2011-12-15 9:50 ` Mauro Carvalho Chehab
@ 2012-01-03 20:55 ` Greg KH
2012-01-04 13:35 ` Dan Carpenter
` (2 more replies)
0 siblings, 3 replies; 10+ messages in thread
From: Greg KH @ 2012-01-03 20:55 UTC (permalink / raw)
To: Mauro Carvalho Chehab
Cc: Hans Verkuil, Dan Carpenter, linux-media, kernel-janitors, stable
On Thu, Dec 15, 2011 at 07:50:30AM -0200, Mauro Carvalho Chehab wrote:
> On 15-12-2011 07:33, Hans Verkuil wrote:
> > On Thursday, December 15, 2011 10:21:41 Mauro Carvalho Chehab wrote:
> >> On 15-12-2011 04:34, Dan Carpenter wrote:
> >>> 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.
>
> 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
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [patch -longterm] V4L/DVB: v4l2-ioctl: integer overflow in video_usercopy()
2012-01-03 20:55 ` Greg KH
@ 2012-01-04 13:35 ` Dan Carpenter
2012-01-05 6:27 ` [patch -next] " Dan Carpenter
2012-01-05 6:28 ` [patch -longterm v2] " Dan Carpenter
2 siblings, 0 replies; 10+ messages in thread
From: Dan Carpenter @ 2012-01-04 13:35 UTC (permalink / raw)
To: Greg KH
Cc: Mauro Carvalho Chehab, Hans Verkuil, linux-media, kernel-janitors,
stable
[-- Attachment #1: Type: text/plain, Size: 273 bytes --]
On Tue, Jan 03, 2012 at 12:55:39PM -0800, Greg KH wrote:
> 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
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]
^ permalink raw reply [flat|nested] 10+ messages in thread
* [patch -next] V4L/DVB: v4l2-ioctl: integer overflow in video_usercopy()
2012-01-03 20:55 ` Greg KH
2012-01-04 13:35 ` Dan Carpenter
@ 2012-01-05 6:27 ` Dan Carpenter
2012-01-05 6:28 ` [patch -longterm v2] " Dan Carpenter
2 siblings, 0 replies; 10+ messages in thread
From: Dan Carpenter @ 2012-01-05 6:27 UTC (permalink / raw)
To: Greg KH
Cc: Mauro Carvalho Chehab, Hans Verkuil, linux-media, kernel-janitors,
stable
[-- Attachment #1: Type: text/plain, Size: 1506 bytes --]
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 <stable@vger.kernel.org>
Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
diff --git a/include/linux/videodev2.h b/include/linux/videodev2.h
index d2f981a..4942f81 100644
--- a/include/linux/videodev2.h
+++ b/include/linux/videodev2.h
@@ -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 */
diff --git a/drivers/media/video/v4l2-ioctl.c b/drivers/media/video/v4l2-ioctl.c
index e1da8fc..639abee 100644
--- a/drivers/media/video/v4l2-ioctl.c
+++ b/drivers/media/video/v4l2-ioctl.c
@@ -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)
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]
^ permalink raw reply related [flat|nested] 10+ messages in thread
* [patch -longterm v2] V4L/DVB: v4l2-ioctl: integer overflow in video_usercopy()
2012-01-03 20:55 ` Greg KH
2012-01-04 13:35 ` Dan Carpenter
2012-01-05 6:27 ` [patch -next] " Dan Carpenter
@ 2012-01-05 6:28 ` Dan Carpenter
2012-01-05 16:43 ` Greg KH
2 siblings, 1 reply; 10+ messages in thread
From: Dan Carpenter @ 2012-01-05 6:28 UTC (permalink / raw)
To: Greg KH
Cc: Mauro Carvalho Chehab, Hans Verkuil, linux-media, kernel-janitors,
stable
[-- Attachment #1: Type: text/plain, Size: 1919 bytes --]
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 <dan.carpenter@oracle.com>
diff --git a/include/linux/videodev2.h b/include/linux/videodev2.h
index b59e78c..9e2088c 100644
--- a/include/linux/videodev2.h
+++ b/include/linux/videodev2.h
@@ -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 */
diff --git a/drivers/media/video/v4l2-ioctl.c b/drivers/media/video/v4l2-ioctl.c
index 265bfb5..d7332c7 100644
--- a/drivers/media/video/v4l2-ioctl.c
+++ b/drivers/media/video/v4l2-ioctl.c
@@ -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);
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]
^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [patch -longterm v2] V4L/DVB: v4l2-ioctl: integer overflow in video_usercopy()
2012-01-05 6:28 ` [patch -longterm v2] " Dan Carpenter
@ 2012-01-05 16:43 ` Greg KH
2012-01-05 17:56 ` Dan Carpenter
0 siblings, 1 reply; 10+ messages in thread
From: Greg KH @ 2012-01-05 16:43 UTC (permalink / raw)
To: Dan Carpenter
Cc: Mauro Carvalho Chehab, Hans Verkuil, linux-media, kernel-janitors,
stable
On Thu, Jan 05, 2012 at 09:28:22AM +0300, Dan Carpenter wrote:
> 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 <dan.carpenter@oracle.com>
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
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [patch -longterm v2] V4L/DVB: v4l2-ioctl: integer overflow in video_usercopy()
2012-01-05 16:43 ` Greg KH
@ 2012-01-05 17:56 ` Dan Carpenter
0 siblings, 0 replies; 10+ messages in thread
From: Dan Carpenter @ 2012-01-05 17:56 UTC (permalink / raw)
To: Greg KH
Cc: Mauro Carvalho Chehab, Hans Verkuil, linux-media, kernel-janitors,
stable
[-- Attachment #1: Type: text/plain, Size: 727 bytes --]
On Thu, Jan 05, 2012 at 08:43:58AM -0800, Greg KH wrote:
> On Thu, Jan 05, 2012 at 09:28:22AM +0300, Dan Carpenter wrote:
> > 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 <dan.carpenter@oracle.com>
>
> 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
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]
^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2012-01-05 17:55 UTC | newest]
Thread overview: 10+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2011-12-15 6:34 [patch -longterm] V4L/DVB: v4l2-ioctl: integer overflow in video_usercopy() Dan Carpenter
2011-12-15 9:21 ` Mauro Carvalho Chehab
2011-12-15 9:33 ` Hans Verkuil
2011-12-15 9:50 ` Mauro Carvalho Chehab
2012-01-03 20:55 ` Greg KH
2012-01-04 13:35 ` Dan Carpenter
2012-01-05 6:27 ` [patch -next] " Dan Carpenter
2012-01-05 6:28 ` [patch -longterm v2] " Dan Carpenter
2012-01-05 16:43 ` Greg KH
2012-01-05 17:56 ` Dan Carpenter
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).