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
