Re: [patch -longterm] V4L/DVB: v4l2-ioctl: integer overflow in video_usercopy()
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 signature.asc Description: Digital signature
Re: [patch -longterm] V4L/DVB: v4l2-ioctl: integer overflow in video_usercopy()
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 -- To unsubscribe from this list: send the line unsubscribe linux-media in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [patch -longterm] V4L/DVB: v4l2-ioctl: integer overflow in video_usercopy()
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.carpen...@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 majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [patch -longterm] V4L/DVB: v4l2-ioctl: integer overflow in video_usercopy()
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.carpen...@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 majord...@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 majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [patch -longterm] V4L/DVB: v4l2-ioctl: integer overflow in video_usercopy()
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.carpen...@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 majord...@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 majord...@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
[patch -longterm] V4L/DVB: v4l2-ioctl: integer overflow in video_usercopy()
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.carpen...@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 majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
