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@xxxxxxxxxx>
> ---
> 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 kernel-janitors" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
