| Message ID | 3814fe88-647e-dc2d-2b5f-fcb1c925228b@xs4all.nl (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Em Fri, 11 Aug 2017 15:26:03 +0200 Hans Verkuil <hverkuil@xs4all.nl> escreveu: > The ctrl_is_pointer used a hard-coded list of control IDs that besides being > outdated also wouldn't work for custom driver controls. > > Replaced by calling queryctrl and checking if the V4L2_CTRL_FLAG_HAS_PAYLOAD > flag was set. > > Note that get_v4l2_ext_controls32() will set the v4l2_ext_control 'size' field > to 0 if the control has no payload before passing it to the kernel. This > helps in put_v4l2_ext_controls32() since that function can just look at the > 'size' field instead of having to call queryctrl again. The reason we set > 'size' explicitly for non-pointer controls is that 'size' is ignored by the > kernel in that case. That makes 'size' useless as an indicator of a pointer > type in the put function since it can be any value. But setting it to 0 here > turns it into a useful indicator. > > Also added proper checks for the compat_alloc_user_space return value which > can be NULL, this was never done for some reason. On a quick preview, please split those extra checks you added on a separate patch. The logic for the remaining parts of this patch is not trivial. I'll look into it later. > > Tested with a 32-bit build of v4l2-ctl and the vivid driver. > > Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com> > --- > diff --git a/drivers/media/v4l2-core/v4l2-compat-ioctl32.c b/drivers/media/v4l2-core/v4l2-compat-ioctl32.c > index af8b4c5b0efa..a16338cc216e 100644 > --- a/drivers/media/v4l2-core/v4l2-compat-ioctl32.c > +++ b/drivers/media/v4l2-core/v4l2-compat-ioctl32.c > @@ -17,7 +17,9 @@ > #include <linux/module.h> > #include <linux/videodev2.h> > #include <linux/v4l2-subdev.h> > +#include <media/v4l2-ctrls.h> > #include <media/v4l2-dev.h> > +#include <media/v4l2-fh.h> > #include <media/v4l2-ioctl.h> > > static long native_ioctl(struct file *file, unsigned int cmd, unsigned long arg) > @@ -67,6 +69,8 @@ static int get_v4l2_window32(struct v4l2_window *kp, struct v4l2_window32 __user > return -EFAULT; > uclips = compat_ptr(p); > kclips = compat_alloc_user_space(n * sizeof(struct v4l2_clip)); > + if (kclips == NULL) > + return -EFAULT; > kp->clips = kclips; > while (--n >= 0) { > if (copy_in_user(&kclips->c, &uclips->c, sizeof(uclips->c))) > @@ -473,6 +477,8 @@ static int get_v4l2_buffer32(struct v4l2_buffer *kp, struct v4l2_buffer32 __user > * by passing a very big num_planes value */ > uplane = compat_alloc_user_space(kp->length * > sizeof(struct v4l2_plane)); > + if (uplane == NULL) > + return -EFAULT; > kp->m.planes = (__force struct v4l2_plane *)uplane; > > for (num_planes = 0; num_planes < kp->length; num_planes++) { > @@ -668,24 +674,38 @@ struct v4l2_ext_control32 { > }; > } __attribute__ ((packed)); > > -/* The following function really belong in v4l2-common, but that causes > - a circular dependency between modules. We need to think about this, but > - for now this will do. */ > > -/* Return non-zero if this control is a pointer type. Currently only > - type STRING is a pointer type. */ > -static inline int ctrl_is_pointer(u32 id) > +/* Return non-zero if this control is a pointer type. */ > +static inline int ctrl_is_pointer(struct file *file, u32 id) > { > - switch (id) { > - case V4L2_CID_RDS_TX_PS_NAME: > - case V4L2_CID_RDS_TX_RADIO_TEXT: > - return 1; > - default: > + struct video_device *vfd = video_devdata(file); > + const struct v4l2_ioctl_ops *ops = vfd->ioctl_ops; > + void *fh = file->private_data; > + struct v4l2_fh *vfh = > + test_bit(V4L2_FL_USES_V4L2_FH, &vfd->flags) ? fh : NULL; > + struct v4l2_queryctrl qctrl = { id }; > + int err; > + > + if (!test_bit(_IOC_NR(VIDIOC_QUERYCTRL), vfd->valid_ioctls)) > + err = -ENOTTY; > + else if (vfh && vfh->ctrl_handler) > + err = v4l2_queryctrl(vfh->ctrl_handler, &qctrl); > + else if (vfd->ctrl_handler) > + err = v4l2_queryctrl(vfd->ctrl_handler, &qctrl); > + else if (ops->vidioc_queryctrl) > + err = ops->vidioc_queryctrl(file, fh, &qctrl); > + else > + err = -ENOTTY; > + > + if (err) > return 0; > - } > + > + return qctrl.flags & V4L2_CTRL_FLAG_HAS_PAYLOAD; > } > > -static int get_v4l2_ext_controls32(struct v4l2_ext_controls *kp, struct v4l2_ext_controls32 __user *up) > +static int get_v4l2_ext_controls32(struct file *file, > + struct v4l2_ext_controls *kp, > + struct v4l2_ext_controls32 __user *up) > { > struct v4l2_ext_control32 __user *ucontrols; > struct v4l2_ext_control __user *kcontrols; > @@ -713,15 +733,28 @@ static int get_v4l2_ext_controls32(struct v4l2_ext_controls *kp, struct v4l2_ext > return -EFAULT; > kcontrols = compat_alloc_user_space(kp->count * > sizeof(struct v4l2_ext_control)); > + if (kcontrols == NULL) > + return -EFAULT; > kp->controls = (__force struct v4l2_ext_control *)kcontrols; > for (n = 0; n < kp->count; n++) { > u32 id; > + int ret; > > if (copy_in_user(kcontrols, ucontrols, sizeof(*ucontrols))) > return -EFAULT; > if (get_user(id, &kcontrols->id)) > return -EFAULT; > - if (ctrl_is_pointer(id)) { > + ret = ctrl_is_pointer(file, id); > + if (ret < 0) > + return ret; > + /* > + * Convert the pointer if this is a pointer control. > + * Otherwise set kcontrols->size to 0 as an additional > + * safety measure. This also allows us to reliably use > + * kcontrols->size as an indicator of a pointer control > + * in put_v4l2_ext_controls32(). > + */ > + if (ret) { > void __user *s; > > if (get_user(p, &ucontrols->string)) > @@ -729,6 +762,8 @@ static int get_v4l2_ext_controls32(struct v4l2_ext_controls *kp, struct v4l2_ext > s = compat_ptr(p); > if (put_user(s, &kcontrols->string)) > return -EFAULT; > + } else if (put_user(0, &kcontrols->size)) { > + return -EFAULT; > } > ucontrols++; > kcontrols++; > @@ -762,14 +797,12 @@ static int put_v4l2_ext_controls32(struct v4l2_ext_controls *kp, struct v4l2_ext > > while (--n >= 0) { > unsigned size = sizeof(*ucontrols); > - u32 id; > + u32 payload_size; > > - if (get_user(id, &kcontrols->id)) > + if (get_user(payload_size, &kcontrols->size)) > return -EFAULT; > - /* Do not modify the pointer when copying a pointer control. > - The contents of the pointer was changed, not the pointer > - itself. */ > - if (ctrl_is_pointer(id)) > + /* If payload_size != 0, then this is a pointer control */ > + if (payload_size) > size -= sizeof(ucontrols->value64); > if (copy_in_user(ucontrols, kcontrols, size)) > return -EFAULT; > @@ -983,7 +1016,7 @@ static long do_video_ioctl(struct file *file, unsigned int cmd, unsigned long ar > case VIDIOC_G_EXT_CTRLS: > case VIDIOC_S_EXT_CTRLS: > case VIDIOC_TRY_EXT_CTRLS: > - err = get_v4l2_ext_controls32(&karg.v2ecs, up); > + err = get_v4l2_ext_controls32(file, &karg.v2ecs, up); > compatible_arg = 0; > break; > case VIDIOC_DQEVENT: Thanks, Mauro
On 11/08/17 23:08, Mauro Carvalho Chehab wrote: > Em Fri, 11 Aug 2017 15:26:03 +0200 > Hans Verkuil <hverkuil@xs4all.nl> escreveu: > >> The ctrl_is_pointer used a hard-coded list of control IDs that besides being >> outdated also wouldn't work for custom driver controls. >> >> Replaced by calling queryctrl and checking if the V4L2_CTRL_FLAG_HAS_PAYLOAD >> flag was set. >> >> Note that get_v4l2_ext_controls32() will set the v4l2_ext_control 'size' field >> to 0 if the control has no payload before passing it to the kernel. This >> helps in put_v4l2_ext_controls32() since that function can just look at the >> 'size' field instead of having to call queryctrl again. The reason we set >> 'size' explicitly for non-pointer controls is that 'size' is ignored by the >> kernel in that case. That makes 'size' useless as an indicator of a pointer >> type in the put function since it can be any value. But setting it to 0 here >> turns it into a useful indicator. >> >> Also added proper checks for the compat_alloc_user_space return value which >> can be NULL, this was never done for some reason. > > On a quick preview, please split those extra checks you added on > a separate patch. > > The logic for the remaining parts of this patch is not trivial. I'll look > into it later. > >> >> Tested with a 32-bit build of v4l2-ctl and the vivid driver. >> >> Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com> >> --- >> diff --git a/drivers/media/v4l2-core/v4l2-compat-ioctl32.c b/drivers/media/v4l2-core/v4l2-compat-ioctl32.c >> index af8b4c5b0efa..a16338cc216e 100644 >> --- a/drivers/media/v4l2-core/v4l2-compat-ioctl32.c >> +++ b/drivers/media/v4l2-core/v4l2-compat-ioctl32.c <snip> >> -/* The following function really belong in v4l2-common, but that causes >> - a circular dependency between modules. We need to think about this, but >> - for now this will do. */ >> >> -/* Return non-zero if this control is a pointer type. Currently only >> - type STRING is a pointer type. */ >> -static inline int ctrl_is_pointer(u32 id) >> +/* Return non-zero if this control is a pointer type. */ >> +static inline int ctrl_is_pointer(struct file *file, u32 id) >> { >> - switch (id) { >> - case V4L2_CID_RDS_TX_PS_NAME: >> - case V4L2_CID_RDS_TX_RADIO_TEXT: >> - return 1; >> - default: >> + struct video_device *vfd = video_devdata(file); >> + const struct v4l2_ioctl_ops *ops = vfd->ioctl_ops; >> + void *fh = file->private_data; >> + struct v4l2_fh *vfh = >> + test_bit(V4L2_FL_USES_V4L2_FH, &vfd->flags) ? fh : NULL; >> + struct v4l2_queryctrl qctrl = { id }; >> + int err; >> + >> + if (!test_bit(_IOC_NR(VIDIOC_QUERYCTRL), vfd->valid_ioctls)) >> + err = -ENOTTY; >> + else if (vfh && vfh->ctrl_handler) >> + err = v4l2_queryctrl(vfh->ctrl_handler, &qctrl); >> + else if (vfd->ctrl_handler) >> + err = v4l2_queryctrl(vfd->ctrl_handler, &qctrl); >> + else if (ops->vidioc_queryctrl) >> + err = ops->vidioc_queryctrl(file, fh, &qctrl); >> + else >> + err = -ENOTTY; >> + >> + if (err) >> return 0; >> - } >> + >> + return qctrl.flags & V4L2_CTRL_FLAG_HAS_PAYLOAD; >> } Mauro, I'd like your opinion on something: the code to call queryctrl is identical to the v4l_queryctrl() function in v4l2-ioctl.c. I have been debating with myself whether or not to drop the 'static' from that v4l2-ioctl.c function and call it from here. It's a bit unexpected to have this source calling a function in v4l2-ioctl.c, but on the other hand it avoids having a copy of that function. I'm leaning towards calling v4l_queryctrl from here, but I wonder what you think. Regards, Hans
Em Sat, 12 Aug 2017 10:22:07 +0200 Hans Verkuil <hverkuil@xs4all.nl> escreveu: > On 11/08/17 23:08, Mauro Carvalho Chehab wrote: > > Em Fri, 11 Aug 2017 15:26:03 +0200 > > Hans Verkuil <hverkuil@xs4all.nl> escreveu: > > > >> The ctrl_is_pointer used a hard-coded list of control IDs that besides being > >> outdated also wouldn't work for custom driver controls. > >> > >> Replaced by calling queryctrl and checking if the V4L2_CTRL_FLAG_HAS_PAYLOAD > >> flag was set. > >> > >> Note that get_v4l2_ext_controls32() will set the v4l2_ext_control 'size' field > >> to 0 if the control has no payload before passing it to the kernel. This > >> helps in put_v4l2_ext_controls32() since that function can just look at the > >> 'size' field instead of having to call queryctrl again. The reason we set > >> 'size' explicitly for non-pointer controls is that 'size' is ignored by the > >> kernel in that case. That makes 'size' useless as an indicator of a pointer > >> type in the put function since it can be any value. But setting it to 0 here > >> turns it into a useful indicator. > >> > >> Also added proper checks for the compat_alloc_user_space return value which > >> can be NULL, this was never done for some reason. > > > > On a quick preview, please split those extra checks you added on > > a separate patch. > > > > The logic for the remaining parts of this patch is not trivial. I'll look > > into it later. > > > >> > >> Tested with a 32-bit build of v4l2-ctl and the vivid driver. > >> > >> Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com> > >> --- > >> diff --git a/drivers/media/v4l2-core/v4l2-compat-ioctl32.c b/drivers/media/v4l2-core/v4l2-compat-ioctl32.c > >> index af8b4c5b0efa..a16338cc216e 100644 > >> --- a/drivers/media/v4l2-core/v4l2-compat-ioctl32.c > >> +++ b/drivers/media/v4l2-core/v4l2-compat-ioctl32.c > > <snip> > > >> -/* The following function really belong in v4l2-common, but that causes > >> - a circular dependency between modules. We need to think about this, but > >> - for now this will do. */ > >> > >> -/* Return non-zero if this control is a pointer type. Currently only > >> - type STRING is a pointer type. */ > >> -static inline int ctrl_is_pointer(u32 id) > >> +/* Return non-zero if this control is a pointer type. */ > >> +static inline int ctrl_is_pointer(struct file *file, u32 id) > >> { > >> - switch (id) { > >> - case V4L2_CID_RDS_TX_PS_NAME: > >> - case V4L2_CID_RDS_TX_RADIO_TEXT: > >> - return 1; > >> - default: > >> + struct video_device *vfd = video_devdata(file); > >> + const struct v4l2_ioctl_ops *ops = vfd->ioctl_ops; > >> + void *fh = file->private_data; > >> + struct v4l2_fh *vfh = > >> + test_bit(V4L2_FL_USES_V4L2_FH, &vfd->flags) ? fh : NULL; > >> + struct v4l2_queryctrl qctrl = { id }; > >> + int err; > >> + > >> + if (!test_bit(_IOC_NR(VIDIOC_QUERYCTRL), vfd->valid_ioctls)) > >> + err = -ENOTTY; > >> + else if (vfh && vfh->ctrl_handler) > >> + err = v4l2_queryctrl(vfh->ctrl_handler, &qctrl); > >> + else if (vfd->ctrl_handler) > >> + err = v4l2_queryctrl(vfd->ctrl_handler, &qctrl); > >> + else if (ops->vidioc_queryctrl) > >> + err = ops->vidioc_queryctrl(file, fh, &qctrl); > >> + else > >> + err = -ENOTTY; > >> + > >> + if (err) > >> return 0; > >> - } > >> + > >> + return qctrl.flags & V4L2_CTRL_FLAG_HAS_PAYLOAD; > >> } > > Mauro, > > I'd like your opinion on something: the code to call queryctrl is identical to > the v4l_queryctrl() function in v4l2-ioctl.c. I have been debating with myself > whether or not to drop the 'static' from that v4l2-ioctl.c function and call > it from here. It's a bit unexpected to have this source calling a function in > v4l2-ioctl.c, but on the other hand it avoids having a copy of that function. > > I'm leaning towards calling v4l_queryctrl from here, but I wonder what you > think. I would drop "static" from the function and call it directly here. On my quick look on this patch yesterday, it came to my mind that there is a lot of things there that, IMHO, doesn't belong to the compat code (as it shouldn't know the dirty details about control handling). As both v4l2-ioctl and v4l2-compat-ioctl32 belongs to the same module (videodev), you don't even need to make it exportable. IMHO, doing it let the code more clear and easier to review. As a side effect, it prevents us to change both codes if the implementation of v4l_queryctrl() changes. Regards, Mauro
Hi Hans, On Fri, Aug 11, 2017 at 03:26:03PM +0200, Hans Verkuil wrote: > The ctrl_is_pointer used a hard-coded list of control IDs that besides being > outdated also wouldn't work for custom driver controls. > > Replaced by calling queryctrl and checking if the V4L2_CTRL_FLAG_HAS_PAYLOAD > flag was set. I think the approach is good, this scales much better. Considering that running a 32-bit user space on a 64-bit kernel is an entirely possible production setup, I'm worried of the overhead that this brings to accessing controls. How about just looking up the control and then checking ctrl->is_ptr? Wouldn't it yield the same result? > > Note that get_v4l2_ext_controls32() will set the v4l2_ext_control 'size' field > to 0 if the control has no payload before passing it to the kernel. This > helps in put_v4l2_ext_controls32() since that function can just look at the > 'size' field instead of having to call queryctrl again. The reason we set > 'size' explicitly for non-pointer controls is that 'size' is ignored by the > kernel in that case. That makes 'size' useless as an indicator of a pointer > type in the put function since it can be any value. But setting it to 0 here > turns it into a useful indicator. > > Also added proper checks for the compat_alloc_user_space return value which > can be NULL, this was never done for some reason. > > Tested with a 32-bit build of v4l2-ctl and the vivid driver. > > Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com> > --- > diff --git a/drivers/media/v4l2-core/v4l2-compat-ioctl32.c b/drivers/media/v4l2-core/v4l2-compat-ioctl32.c > index af8b4c5b0efa..a16338cc216e 100644 > --- a/drivers/media/v4l2-core/v4l2-compat-ioctl32.c > +++ b/drivers/media/v4l2-core/v4l2-compat-ioctl32.c > @@ -17,7 +17,9 @@ > #include <linux/module.h> > #include <linux/videodev2.h> > #include <linux/v4l2-subdev.h> > +#include <media/v4l2-ctrls.h> > #include <media/v4l2-dev.h> > +#include <media/v4l2-fh.h> > #include <media/v4l2-ioctl.h> > > static long native_ioctl(struct file *file, unsigned int cmd, unsigned long arg) > @@ -67,6 +69,8 @@ static int get_v4l2_window32(struct v4l2_window *kp, struct v4l2_window32 __user > return -EFAULT; > uclips = compat_ptr(p); > kclips = compat_alloc_user_space(n * sizeof(struct v4l2_clip)); > + if (kclips == NULL) > + return -EFAULT; > kp->clips = kclips; > while (--n >= 0) { > if (copy_in_user(&kclips->c, &uclips->c, sizeof(uclips->c))) > @@ -473,6 +477,8 @@ static int get_v4l2_buffer32(struct v4l2_buffer *kp, struct v4l2_buffer32 __user > * by passing a very big num_planes value */ > uplane = compat_alloc_user_space(kp->length * > sizeof(struct v4l2_plane)); > + if (uplane == NULL) > + return -EFAULT; > kp->m.planes = (__force struct v4l2_plane *)uplane; > > for (num_planes = 0; num_planes < kp->length; num_planes++) { > @@ -668,24 +674,38 @@ struct v4l2_ext_control32 { > }; > } __attribute__ ((packed)); > > -/* The following function really belong in v4l2-common, but that causes > - a circular dependency between modules. We need to think about this, but > - for now this will do. */ > > -/* Return non-zero if this control is a pointer type. Currently only > - type STRING is a pointer type. */ > -static inline int ctrl_is_pointer(u32 id) > +/* Return non-zero if this control is a pointer type. */ > +static inline int ctrl_is_pointer(struct file *file, u32 id) > { > - switch (id) { > - case V4L2_CID_RDS_TX_PS_NAME: > - case V4L2_CID_RDS_TX_RADIO_TEXT: > - return 1; > - default: > + struct video_device *vfd = video_devdata(file); > + const struct v4l2_ioctl_ops *ops = vfd->ioctl_ops; > + void *fh = file->private_data; > + struct v4l2_fh *vfh = > + test_bit(V4L2_FL_USES_V4L2_FH, &vfd->flags) ? fh : NULL; > + struct v4l2_queryctrl qctrl = { id }; > + int err; > + > + if (!test_bit(_IOC_NR(VIDIOC_QUERYCTRL), vfd->valid_ioctls)) > + err = -ENOTTY; > + else if (vfh && vfh->ctrl_handler) > + err = v4l2_queryctrl(vfh->ctrl_handler, &qctrl); > + else if (vfd->ctrl_handler) > + err = v4l2_queryctrl(vfd->ctrl_handler, &qctrl); > + else if (ops->vidioc_queryctrl) > + err = ops->vidioc_queryctrl(file, fh, &qctrl); > + else > + err = -ENOTTY; > + > + if (err) > return 0; > - } > + > + return qctrl.flags & V4L2_CTRL_FLAG_HAS_PAYLOAD; > } > > -static int get_v4l2_ext_controls32(struct v4l2_ext_controls *kp, struct v4l2_ext_controls32 __user *up) > +static int get_v4l2_ext_controls32(struct file *file, > + struct v4l2_ext_controls *kp, > + struct v4l2_ext_controls32 __user *up) > { > struct v4l2_ext_control32 __user *ucontrols; > struct v4l2_ext_control __user *kcontrols; > @@ -713,15 +733,28 @@ static int get_v4l2_ext_controls32(struct v4l2_ext_controls *kp, struct v4l2_ext > return -EFAULT; > kcontrols = compat_alloc_user_space(kp->count * > sizeof(struct v4l2_ext_control)); > + if (kcontrols == NULL) > + return -EFAULT; > kp->controls = (__force struct v4l2_ext_control *)kcontrols; > for (n = 0; n < kp->count; n++) { > u32 id; > + int ret; > > if (copy_in_user(kcontrols, ucontrols, sizeof(*ucontrols))) > return -EFAULT; > if (get_user(id, &kcontrols->id)) > return -EFAULT; > - if (ctrl_is_pointer(id)) { > + ret = ctrl_is_pointer(file, id); > + if (ret < 0) > + return ret; > + /* > + * Convert the pointer if this is a pointer control. > + * Otherwise set kcontrols->size to 0 as an additional > + * safety measure. This also allows us to reliably use > + * kcontrols->size as an indicator of a pointer control > + * in put_v4l2_ext_controls32(). > + */ > + if (ret) { > void __user *s; > > if (get_user(p, &ucontrols->string)) > @@ -729,6 +762,8 @@ static int get_v4l2_ext_controls32(struct v4l2_ext_controls *kp, struct v4l2_ext > s = compat_ptr(p); > if (put_user(s, &kcontrols->string)) > return -EFAULT; > + } else if (put_user(0, &kcontrols->size)) { > + return -EFAULT; > } > ucontrols++; > kcontrols++; > @@ -762,14 +797,12 @@ static int put_v4l2_ext_controls32(struct v4l2_ext_controls *kp, struct v4l2_ext > > while (--n >= 0) { > unsigned size = sizeof(*ucontrols); > - u32 id; > + u32 payload_size; > > - if (get_user(id, &kcontrols->id)) > + if (get_user(payload_size, &kcontrols->size)) > return -EFAULT; > - /* Do not modify the pointer when copying a pointer control. > - The contents of the pointer was changed, not the pointer > - itself. */ > - if (ctrl_is_pointer(id)) > + /* If payload_size != 0, then this is a pointer control */ > + if (payload_size) > size -= sizeof(ucontrols->value64); > if (copy_in_user(ucontrols, kcontrols, size)) > return -EFAULT; > @@ -983,7 +1016,7 @@ static long do_video_ioctl(struct file *file, unsigned int cmd, unsigned long ar > case VIDIOC_G_EXT_CTRLS: > case VIDIOC_S_EXT_CTRLS: > case VIDIOC_TRY_EXT_CTRLS: > - err = get_v4l2_ext_controls32(&karg.v2ecs, up); > + err = get_v4l2_ext_controls32(file, &karg.v2ecs, up); > compatible_arg = 0; > break; > case VIDIOC_DQEVENT:
diff --git a/drivers/media/v4l2-core/v4l2-compat-ioctl32.c b/drivers/media/v4l2-core/v4l2-compat-ioctl32.c index af8b4c5b0efa..a16338cc216e 100644 --- a/drivers/media/v4l2-core/v4l2-compat-ioctl32.c +++ b/drivers/media/v4l2-core/v4l2-compat-ioctl32.c @@ -17,7 +17,9 @@ #include <linux/module.h> #include <linux/videodev2.h> #include <linux/v4l2-subdev.h> +#include <media/v4l2-ctrls.h> #include <media/v4l2-dev.h> +#include <media/v4l2-fh.h> #include <media/v4l2-ioctl.h> static long native_ioctl(struct file *file, unsigned int cmd, unsigned long arg) @@ -67,6 +69,8 @@ static int get_v4l2_window32(struct v4l2_window *kp, struct v4l2_window32 __user return -EFAULT; uclips = compat_ptr(p); kclips = compat_alloc_user_space(n * sizeof(struct v4l2_clip)); + if (kclips == NULL) + return -EFAULT; kp->clips = kclips; while (--n >= 0) { if (copy_in_user(&kclips->c, &uclips->c, sizeof(uclips->c))) @@ -473,6 +477,8 @@ static int get_v4l2_buffer32(struct v4l2_buffer *kp, struct v4l2_buffer32 __user * by passing a very big num_planes value */ uplane = compat_alloc_user_space(kp->length * sizeof(struct v4l2_plane)); + if (uplane == NULL) + return -EFAULT; kp->m.planes = (__force struct v4l2_plane *)uplane; for (num_planes = 0; num_planes < kp->length; num_planes++) { @@ -668,24 +674,38 @@ struct v4l2_ext_control32 { }; } __attribute__ ((packed)); -/* The following function really belong in v4l2-common, but that causes - a circular dependency between modules. We need to think about this, but - for now this will do. */ -/* Return non-zero if this control is a pointer type. Currently only - type STRING is a pointer type. */ -static inline int ctrl_is_pointer(u32 id) +/* Return non-zero if this control is a pointer type. */ +static inline int ctrl_is_pointer(struct file *file, u32 id) { - switch (id) { - case V4L2_CID_RDS_TX_PS_NAME: - case V4L2_CID_RDS_TX_RADIO_TEXT: - return 1; - default: + struct video_device *vfd = video_devdata(file); + const struct v4l2_ioctl_ops *ops = vfd->ioctl_ops; + void *fh = file->private_data; + struct v4l2_fh *vfh = + test_bit(V4L2_FL_USES_V4L2_FH, &vfd->flags) ? fh : NULL; + struct v4l2_queryctrl qctrl = { id }; + int err; + + if (!test_bit(_IOC_NR(VIDIOC_QUERYCTRL), vfd->valid_ioctls)) + err = -ENOTTY; + else if (vfh && vfh->ctrl_handler) + err = v4l2_queryctrl(vfh->ctrl_handler, &qctrl); + else if (vfd->ctrl_handler) + err = v4l2_queryctrl(vfd->ctrl_handler, &qctrl); + else if (ops->vidioc_queryctrl) + err = ops->vidioc_queryctrl(file, fh, &qctrl); + else + err = -ENOTTY; + + if (err) return 0; - } + + return qctrl.flags & V4L2_CTRL_FLAG_HAS_PAYLOAD; } -static int get_v4l2_ext_controls32(struct v4l2_ext_controls *kp, struct v4l2_ext_controls32 __user *up) +static int get_v4l2_ext_controls32(struct file *file, + struct v4l2_ext_controls *kp, + struct v4l2_ext_controls32 __user *up) { struct v4l2_ext_control32 __user *ucontrols; struct v4l2_ext_control __user *kcontrols; @@ -713,15 +733,28 @@ static int get_v4l2_ext_controls32(struct v4l2_ext_controls *kp, struct v4l2_ext return -EFAULT; kcontrols = compat_alloc_user_space(kp->count * sizeof(struct v4l2_ext_control)); + if (kcontrols == NULL) + return -EFAULT; kp->controls = (__force struct v4l2_ext_control *)kcontrols; for (n = 0; n < kp->count; n++) { u32 id; + int ret; if (copy_in_user(kcontrols, ucontrols, sizeof(*ucontrols))) return -EFAULT; if (get_user(id, &kcontrols->id)) return -EFAULT; - if (ctrl_is_pointer(id)) { + ret = ctrl_is_pointer(file, id); + if (ret < 0) + return ret; + /* + * Convert the pointer if this is a pointer control. + * Otherwise set kcontrols->size to 0 as an additional + * safety measure. This also allows us to reliably use + * kcontrols->size as an indicator of a pointer control + * in put_v4l2_ext_controls32(). + */ + if (ret) { void __user *s; if (get_user(p, &ucontrols->string)) @@ -729,6 +762,8 @@ static int get_v4l2_ext_controls32(struct v4l2_ext_controls *kp, struct v4l2_ext s = compat_ptr(p); if (put_user(s, &kcontrols->string)) return -EFAULT; + } else if (put_user(0, &kcontrols->size)) { + return -EFAULT; } ucontrols++; kcontrols++; @@ -762,14 +797,12 @@ static int put_v4l2_ext_controls32(struct v4l2_ext_controls *kp, struct v4l2_ext while (--n >= 0) { unsigned size = sizeof(*ucontrols); - u32 id; + u32 payload_size; - if (get_user(id, &kcontrols->id)) + if (get_user(payload_size, &kcontrols->size)) return -EFAULT; - /* Do not modify the pointer when copying a pointer control. - The contents of the pointer was changed, not the pointer - itself. */ - if (ctrl_is_pointer(id)) + /* If payload_size != 0, then this is a pointer control */ + if (payload_size) size -= sizeof(ucontrols->value64); if (copy_in_user(ucontrols, kcontrols, size)) return -EFAULT; @@ -983,7 +1016,7 @@ static long do_video_ioctl(struct file *file, unsigned int cmd, unsigned long ar case VIDIOC_G_EXT_CTRLS: case VIDIOC_S_EXT_CTRLS: case VIDIOC_TRY_EXT_CTRLS: - err = get_v4l2_ext_controls32(&karg.v2ecs, up); + err = get_v4l2_ext_controls32(file, &karg.v2ecs, up); compatible_arg = 0; break; case VIDIOC_DQEVENT:
The ctrl_is_pointer used a hard-coded list of control IDs that besides being outdated also wouldn't work for custom driver controls. Replaced by calling queryctrl and checking if the V4L2_CTRL_FLAG_HAS_PAYLOAD flag was set. Note that get_v4l2_ext_controls32() will set the v4l2_ext_control 'size' field to 0 if the control has no payload before passing it to the kernel. This helps in put_v4l2_ext_controls32() since that function can just look at the 'size' field instead of having to call queryctrl again. The reason we set 'size' explicitly for non-pointer controls is that 'size' is ignored by the kernel in that case. That makes 'size' useless as an indicator of a pointer type in the put function since it can be any value. But setting it to 0 here turns it into a useful indicator. Also added proper checks for the compat_alloc_user_space return value which can be NULL, this was never done for some reason. Tested with a 32-bit build of v4l2-ctl and the vivid driver. Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com> ---