V4L/DVB (10811): videodev: only copy needed part of RW ioctl's parameter

There are many RW ioctls() in v4l2 where userspace only supplies one or two
of the first fields in the structure passed to the ioctl.  The driver then
fills in the rest of the fields.

Instead of copying the entire structure from userspace to the kernel we
only need to copy those fields that userspace is actually supposed to
supply.

What's more, the fields that are meant to be only be output from the driver
can be zeroed out in the videodev code, in case the driver doesn't fill
them all in.  Many of the ioctl handlers in v4l2_ioctl do this already, but
my patch does this at one common point and so all the memsets for each
ioctl can be deleted.

For VIDIOC_G_SLICED_VBI_CAP, which has one input field ('type') and other
output-only fields, the input field is near the end of the structure
instead of at the beginning.  So there is still a memset in it's ioctl
handler to zero out the beginning of the struct.

There were a couple mistakes with the existing code:
    For VIDIOC_G_AUDIO the index field was preserved, but G_AUDIO is a read
    only ioctl so nothing is copied from userspace to preserve.

    For VIDIOC_G_FREQUENCY the tuner field was not preserved like it should
    have been.  This would be a problem if there was any hardware with more
    than one tuner/modulator.

    For VIDIOC_ENUM_FRAMESIZES and VIDIOC_ENUM_FRAMEINTERVALS, none of the
    fields were preserved even though each ioctl has several field that are
    supposed to be inputs to the driver!  Obviously these ioctls don't get
    used much.  The index field is needed if the driver has multiple
    discrete sizes/rates and other fields can be used too, e.g. if the size
    depends on pixel format or frame rate depends on image size for
    example.

Signed-off-by: Trent Piepho <xyzzy@speakeasy.org>
Signed-off-by: Mauro Carvalho Chehab <mchehab@redhat.com>

diff --git a/drivers/media/video/v4l2-ioctl.c b/drivers/media/video/v4l2-ioctl.c
index 175688e9489f83093d4f37c4682abdf6ff6a153e..00c0f76b9b5df18dd0a4f1c0c571728d586b4fac 100644 (file)
--- a/drivers/media/video/v4l2-ioctl.c
+++ b/drivers/media/video/v4l2-ioctl.c
@@ -726,16 +726,8 @@ static long __video_do_ioctl(struct file *file,
        case VIDIOC_ENUM_FMT:
        {
                struct v4l2_fmtdesc *f = arg;
-               enum v4l2_buf_type type;
-               unsigned int index;
 
-               index = f->index;
-               type  = f->type;
-               memset(f, 0, sizeof(*f));
-               f->index = index;
-               f->type  = type;
-
-               switch (type) {
+               switch (f->type) {
                case V4L2_BUF_TYPE_VIDEO_CAPTURE:
                        if (ops->vidioc_enum_fmt_vid_cap)
                                ret = ops->vidioc_enum_fmt_vid_cap(file, fh, f);
@@ -772,8 +764,6 @@ static long __video_do_ioctl(struct file *file,
        {
                struct v4l2_format *f = (struct v4l2_format *)arg;
 
-               memset(f->fmt.raw_data, 0, sizeof(f->fmt.raw_data));
-
                /* FIXME: Should be one dump per type */
                dbgarg(cmd, "type=%s\n", prt_names(f->type, v4l2_type_names));
 
@@ -969,11 +959,6 @@ static long __video_do_ioctl(struct file *file,
                if (ret)
                        break;
 
-               /* Zero out all fields starting with bytesysed, which is
-                * everything but index and type.  */
-               memset(0, &p->bytesused,
-                      sizeof(*p) - offsetof(typeof(*p), bytesused));
-
                ret = ops->vidioc_querybuf(file, fh, p);
                if (!ret)
                        dbgbuf(cmd, vfd, p);
@@ -1159,12 +1144,9 @@ static long __video_do_ioctl(struct file *file,
        case VIDIOC_ENUMINPUT:
        {
                struct v4l2_input *p = arg;
-               int i = p->index;
 
                if (!ops->vidioc_enum_input)
                        break;
-               memset(p, 0, sizeof(*p));
-               p->index = i;
 
                ret = ops->vidioc_enum_input(file, fh, p);
                if (!ret)
@@ -1203,12 +1185,9 @@ static long __video_do_ioctl(struct file *file,
        case VIDIOC_ENUMOUTPUT:
        {
                struct v4l2_output *p = arg;
-               int i = p->index;
 
                if (!ops->vidioc_enum_output)
                        break;
-               memset(p, 0, sizeof(*p));
-               p->index = i;
 
                ret = ops->vidioc_enum_output(file, fh, p);
                if (!ret)
@@ -1384,13 +1363,11 @@ static long __video_do_ioctl(struct file *file,
        case VIDIOC_G_AUDIO:
        {
                struct v4l2_audio *p = arg;
-               __u32 index = p->index;
 
                if (!ops->vidioc_g_audio)
                        break;
 
                memset(p, 0, sizeof(*p));
-               p->index = index;
                ret = ops->vidioc_g_audio(file, fh, p);
                if (!ret)
                        dbgarg(cmd, "index=%d, name=%s, capability=0x%x, "
@@ -1485,15 +1462,10 @@ static long __video_do_ioctl(struct file *file,
        case VIDIOC_G_CROP:
        {
                struct v4l2_crop *p = arg;
-               __u32 type;
 
                if (!ops->vidioc_g_crop)
                        break;
 
-               type = p->type;
-               memset(p, 0, sizeof(*p));
-               p->type = type;
-
                dbgarg(cmd, "type=%s\n", prt_names(p->type, v4l2_type_names));
                ret = ops->vidioc_g_crop(file, fh, p);
                if (!ret)
@@ -1514,16 +1486,11 @@ static long __video_do_ioctl(struct file *file,
        case VIDIOC_CROPCAP:
        {
                struct v4l2_cropcap *p = arg;
-               __u32 type;
 
                /*FIXME: Should also show v4l2_fract pixelaspect */
                if (!ops->vidioc_cropcap)
                        break;
 
-               type = p->type;
-               memset(p, 0, sizeof(*p));
-               p->type = type;
-
                dbgarg(cmd, "type=%s\n", prt_names(p->type, v4l2_type_names));
                ret = ops->vidioc_cropcap(file, fh, p);
                if (!ret) {
@@ -1581,7 +1548,6 @@ static long __video_do_ioctl(struct file *file,
 
                if (!ops->vidioc_encoder_cmd)
                        break;
-               memset(&p->raw, 0, sizeof(p->raw));
                ret = ops->vidioc_encoder_cmd(file, fh, p);
                if (!ret)
                        dbgarg(cmd, "cmd=%d, flags=%x\n", p->cmd, p->flags);
@@ -1593,7 +1559,6 @@ static long __video_do_ioctl(struct file *file,
 
                if (!ops->vidioc_try_encoder_cmd)
                        break;
-               memset(&p->raw, 0, sizeof(p->raw));
                ret = ops->vidioc_try_encoder_cmd(file, fh, p);
                if (!ret)
                        dbgarg(cmd, "cmd=%d, flags=%x\n", p->cmd, p->flags);
@@ -1602,10 +1567,6 @@ static long __video_do_ioctl(struct file *file,
        case VIDIOC_G_PARM:
        {
                struct v4l2_streamparm *p = arg;
-               __u32 type = p->type;
-
-               memset(p, 0, sizeof(*p));
-               p->type = type;
 
                if (ops->vidioc_g_parm) {
                        ret = ops->vidioc_g_parm(file, fh, p);
@@ -1638,14 +1599,10 @@ static long __video_do_ioctl(struct file *file,
        case VIDIOC_G_TUNER:
        {
                struct v4l2_tuner *p = arg;
-               __u32 index = p->index;
 
                if (!ops->vidioc_g_tuner)
                        break;
 
-               memset(p, 0, sizeof(*p));
-               p->index = index;
-
                ret = ops->vidioc_g_tuner(file, fh, p);
                if (!ret)
                        dbgarg(cmd, "index=%d, name=%s, type=%d, "
@@ -1682,8 +1639,6 @@ static long __video_do_ioctl(struct file *file,
                if (!ops->vidioc_g_frequency)
                        break;
 
-               memset(p->reserved, 0, sizeof(p->reserved));
-
                ret = ops->vidioc_g_frequency(file, fh, p);
                if (!ret)
                        dbgarg(cmd, "tuner=%d, type=%d, frequency=%d\n",
@@ -1704,12 +1659,13 @@ static long __video_do_ioctl(struct file *file,
        case VIDIOC_G_SLICED_VBI_CAP:
        {
                struct v4l2_sliced_vbi_cap *p = arg;
-               __u32 type = p->type;
 
                if (!ops->vidioc_g_sliced_vbi_cap)
                        break;
-               memset(p, 0, sizeof(*p));
-               p->type = type;
+
+               /* Clear up to type, everything after type is zerod already */
+               memset(p, 0, offsetof(struct v4l2_sliced_vbi_cap, type));
+
                dbgarg(cmd, "type=%s\n", prt_names(p->type, v4l2_type_names));
                ret = ops->vidioc_g_sliced_vbi_cap(file, fh, p);
                if (!ret)
@@ -1782,8 +1738,6 @@ static long __video_do_ioctl(struct file *file,
                if (!ops->vidioc_enum_framesizes)
                        break;
 
-               memset(p, 0, sizeof(*p));
-
                ret = ops->vidioc_enum_framesizes(file, fh, p);
                dbgarg(cmd,
                        "index=%d, pixelformat=%d, type=%d ",
@@ -1815,8 +1769,6 @@ static long __video_do_ioctl(struct file *file,
                if (!ops->vidioc_enum_frameintervals)
                        break;
 
-               memset(p, 0, sizeof(*p));
-
                ret = ops->vidioc_enum_frameintervals(file, fh, p);
                dbgarg(cmd,
                        "index=%d, pixelformat=%d, width=%d, height=%d, type=%d ",
@@ -1865,6 +1817,44 @@ static long __video_do_ioctl(struct file *file,
        return ret;
 }
 
+/* In some cases, only a few fields are used as input, i.e. when the app sets
+ * "index" and then the driver fills in the rest of the structure for the thing
+ * with that index.  We only need to copy up the first non-input field.  */
+static unsigned long cmd_input_size(unsigned int cmd)
+{
+       /* Size of structure up to and including 'field' */
+#define CMDINSIZE(cmd, type, field) case _IOC_NR(VIDIOC_##cmd): return \
+               offsetof(struct v4l2_##type, field) + \
+               sizeof(((struct v4l2_##type *)0)->field);
+
+       switch (_IOC_NR(cmd)) {
+               CMDINSIZE(ENUM_FMT,             fmtdesc,        type);
+               CMDINSIZE(G_FMT,                format,         type);
+               CMDINSIZE(QUERYBUF,             buffer,         type);
+               CMDINSIZE(G_PARM,               streamparm,     type);
+               CMDINSIZE(ENUMSTD,              standard,       index);
+               CMDINSIZE(ENUMINPUT,            input,          index);
+               CMDINSIZE(G_CTRL,               control,        id);
+               CMDINSIZE(G_TUNER,              tuner,          index);
+               CMDINSIZE(QUERYCTRL,            queryctrl,      id);
+               CMDINSIZE(QUERYMENU,            querymenu,      index);
+               CMDINSIZE(ENUMOUTPUT,           output,         index);
+               CMDINSIZE(G_MODULATOR,          modulator,      index);
+               CMDINSIZE(G_FREQUENCY,          frequency,      tuner);
+               CMDINSIZE(CROPCAP,              cropcap,        type);
+               CMDINSIZE(G_CROP,               crop,           type);
+               CMDINSIZE(ENUMAUDIO,            audio,          index);
+               CMDINSIZE(ENUMAUDOUT,           audioout,       index);
+               CMDINSIZE(ENCODER_CMD,          encoder_cmd,    flags);
+               CMDINSIZE(TRY_ENCODER_CMD,      encoder_cmd,    flags);
+               CMDINSIZE(G_SLICED_VBI_CAP,     sliced_vbi_cap, type);
+               CMDINSIZE(ENUM_FRAMESIZES,      frmsizeenum,    pixel_format);
+               CMDINSIZE(ENUM_FRAMEINTERVALS,  frmivalenum,    height);
+       default:
+               return _IOC_SIZE(cmd);
+       }
+}
+
 long video_ioctl2(struct file *file,
               unsigned int cmd, unsigned long arg)
 {
@@ -1901,9 +1891,16 @@ long video_ioctl2(struct file *file,
                }
 
                err = -EFAULT;
-               if (_IOC_DIR(cmd) & _IOC_WRITE)
-                       if (copy_from_user(parg, (void __user *)arg, _IOC_SIZE(cmd)))
+               if (_IOC_DIR(cmd) & _IOC_WRITE) {
+                       unsigned long n = cmd_input_size(cmd);
+
+                       if (copy_from_user(parg, (void __user *)arg, n))
                                goto out;
+
+                       /* zero out anything we don't copy from userspace */
+                       if (n < _IOC_SIZE(cmd))
+                               memset((u8 *)parg + n, 0, _IOC_SIZE(cmd) - n);
+               }
                break;
        }