Skip to content

Commit

Permalink
V4L/DVB (10811): videodev: only copy needed part of RW ioctl's parameter
Browse files Browse the repository at this point in the history 
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>
  • Loading branch information
Trent Piepho authored and Mauro Carvalho Chehab committed Mar 30, 2009
1 parent 55d5834 commit 19c96e4
Showing 1 changed file with 52 additions and 55 deletions.
107 changes: 52 additions & 55 deletions drivers/media/video/v4l2-ioctl.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down Expand Up @@ -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));

Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -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, "
Expand Down Expand Up @@ -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)
Expand All @@ -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) {
Expand Down Expand Up @@ -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);
Expand All @@ -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);
Expand All @@ -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);
Expand Down Expand Up @@ -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, "
Expand Down Expand Up @@ -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",
Expand All @@ -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)
Expand Down Expand Up @@ -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 ",
Expand Down Expand Up @@ -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 ",
Expand Down Expand Up @@ -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)
{
Expand Down Expand Up @@ -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;
}

Expand Down

0 comments on commit 19c96e4

Please sign in to comment.