Hi Hans,
On Mon, Jan 29, 2018 at 06:41:20PM +0100, Hans Verkuil wrote:
> On 01/29/2018 06:06 PM, Sakari Ailus wrote:
> > Hi Hans,
> >
> > Thanks for your efforts on this patch and the patchset. Please see my
> > comments below.
> >
> > On Fri, Jan 26, 2018 at 01:43:27PM +0100, Hans Verkuil wrote:
> >> From: Daniel Mentz <danielme...@google.com>
> >>
> >> The 32-bit compat v4l2 ioctl is implemented based on its 64-bit
> >
> > s/v4l2 ioctl/V4L2 IOCTL handling/
> >
> > ?
> >
> >> equivalent. It converts 32-bit data structures into its 64-bit
> >> equivalents and needs to provide the data to the 64-bit ioctl in user
> >> space memory which is commonly allocated using
> >> compat_alloc_user_space(). However, due to how that function is
> >> implemented, it can only be called a single time for every syscall
> >> invocation. Supposedly to avoid this limitation, the existing code uses
> >> a mix of memory from the kernel stack and memory allocated through
> >> compat_alloc_user_space(). Under normal circumstances, this would not
> >> work, because the 64-bit ioctl expects all pointers to point to user
> >> space memory. As a workaround, set_fs(KERNEL_DS) is called to
> >> temporarily disable this extra safety check and allow kernel pointers.
> >> However, this might introduce a security vulnerability: The
> >> result of the 32-bit to 64-bit conversion is writeable by user space
> >> because the output buffer has been allocated via
> >> compat_alloc_user_space(). A malicious user space process could then
> >> manipulate pointers inside this output buffer, and due to the previous
> >> set_fs(KERNEL_DS) call, functions like get_user() or put_user() no longer
> >> prevent kernel memory access.
> >>
> >> The new approach is to pre-calculate the total amount of user space
> >> memory that is needed, allocate it using compat_alloc_user_space() and
> >> then divide up the allocated memory to accommodate all data structures
> >> that need to be converted.
> >>
> >> An alternative approach would have been to retain the union type karg
> >> that they allocated on the kernel stack in do_video_ioctl(), copy all
> >> data from user space into karg and then back to user space. However,
> >> we decided against this approach because it does not align with other
> >> compat syscall implementations. Instead, we tried to replicate the
> >> get_user/put_user pairs as found in other places in the kernel:
> >>
> >> if (get_user(clipcount, &up->clipcount) ||
> >> put_user(clipcount, &kp->clipcount)) return -EFAULT;
> >>
> >> Notes from hans.verk...@cisco.com:
> >>
> >> This patch was taken from
> >> https://github.com/LineageOS/android_kernel_samsung_apq8084/commit/97b733953c06e4f0398ade18850f0817778255f7
> >>
> >> Clearly nobody could be bothered to upstream this patch or at minimum
> >> tell us :-( We only heard about this a week ago.
> >
> > This is very sad indeed. I would really hope that we'll never run into
> > something this again!
> >
> >>
> >> This patch was rebased and cleaned up. Compared to the original I
> >> also swapped the order of the convert_in_user arguments so that they
> >> matched copy_in_user. It was hard to review otherwise. I also replaced
> >> the ALLOC_USER_SPACE/ALLOC_AND_GET by a normal function.
> >
> > The result looks more complicated than it should be.
> >
> > I wonder if the result could be made cleaner by separating the argument
> > checking and copying from / to user space in video_usercopy. That
> > separation almost completely exists already. Rather than mangling the
> > compat argument struct to look like a user-provided 64-bit argument struct,
> > copying the arguments from and to the user space would be separately done
> > to 32-bit compat IOCTL arguments.
> >
> > I suppose that wouldn't be a trivial change either, so for now I'd maintain
> > the approach and later consider how to make this more maintainable in the
> > future. Still, that approach is quite different and it'd be easier to
> > switch from status quo rather than this last patch, suggesting to start
> > with a revert, should we embark into this direction.
> >
> > What do you think?
>
> To be honest, I don't really know. Things will get even more complicated
> once we have to deal with the year 2038 problem since that impacts this
> code as well. I'm waiting for the fence code and request API code to
> land before continuing working on that. Perhaps that might be a good
> time to take a step back and see if we can do something smarter.
Sounds good to me.
>
> >
> >>
> >> Signed-off-by: Daniel Mentz <danielme...@google.com>
> >> Signed-off-by: Hans Verkuil <hans.verk...@cisco.com>
> >> ---
> >> drivers/media/v4l2-core/v4l2-compat-ioctl32.c | 700
> >> ++++++++++++++++----------
> >> 1 file changed, 448 insertions(+), 252 deletions(-)
> >>
> >> diff --git a/drivers/media/v4l2-core/v4l2-compat-ioctl32.c
> >> b/drivers/media/v4l2-core/v4l2-compat-ioctl32.c
> >> index 2aa9b43daf60..27a5a0961cbd 100644
> >> --- a/drivers/media/v4l2-core/v4l2-compat-ioctl32.c
> >> +++ b/drivers/media/v4l2-core/v4l2-compat-ioctl32.c
> >> @@ -22,6 +22,14 @@
> >> #include <media/v4l2-ctrls.h>
> >> #include <media/v4l2-ioctl.h>
> >>
> >> +/* Use the same argument order as copy_in_user */
> >> +#define convert_in_user(to, from) \
> >
> > This doesn't really convert anything. How about calling it assign_in_user,
> > for example?
>
> Sounds reasonable.
>
> >
> >> +({ \
> >> + typeof(*from) val; \
> >
> > "val" is very short, easily leading to hard-to-find namespace collisions.
> > At least the versions of GCC I've used happily get this wrong without a
> > warning.
> >
> > How about __convert_in_user_tmp instead?
>
> Hmm. What about __assign_tmp?
That seems a little risky still. Perhaps unlikely; I'd use a longer,
unconceivable name to be safe.
>
> >
> >> + \
> >> + get_user(val, from) || put_user(val, to); \
> >> +})
> >> +
> >> static long native_ioctl(struct file *file, unsigned int cmd, unsigned
> >> long arg)
> >> {
> >> long ret = -ENOIOCTLCMD;
> >> @@ -48,37 +56,41 @@ struct v4l2_window32 {
> >> __u8 global_alpha;
> >> };
> >>
> >> -static int get_v4l2_window32(struct v4l2_window *kp, struct v4l2_window32
> >> __user *up)
> >> +static int get_v4l2_window32(struct v4l2_window __user *kp,
> >> + struct v4l2_window32 __user *up,
> >> + void __user *aux_buf, int aux_space)
> >> {
> >> struct v4l2_clip32 __user *uclips;
> >> struct v4l2_clip __user *kclips;
> >> compat_caddr_t p;
> >> - u32 n;
> >> + u32 clipcount;
> >>
> >> if (!access_ok(VERIFY_READ, up, sizeof(*up)) ||
> >> - copy_from_user(&kp->w, &up->w, sizeof(up->w)) ||
> >> - get_user(kp->field, &up->field) ||
> >> - get_user(kp->chromakey, &up->chromakey) ||
> >> - get_user(kp->clipcount, &up->clipcount) ||
> >> - get_user(kp->global_alpha, &up->global_alpha))
> >> + copy_in_user(&kp->w, &up->w, sizeof(up->w)) ||
> >> + convert_in_user(&kp->field, &up->field) ||
> >> + convert_in_user(&kp->chromakey, &up->chromakey) ||
> >> + convert_in_user(&kp->global_alpha, &up->global_alpha) ||
> >> + get_user(clipcount, &up->clipcount) ||
> >> + put_user(clipcount, &kp->clipcount))
> >> return -EFAULT;
> >> - if (kp->clipcount > 2048)
> >> + if (clipcount > 2048)
> >> return -EINVAL;
> >> - if (!kp->clipcount) {
> >> - kp->clips = NULL;
> >> - return 0;
> >> - }
> >> + if (!clipcount)
> >> + return put_user(NULL, &kp->clips) ? -EFAULT : 0;
> >
> > Isn't this what put_user would return anyway?
>
> True, I'll simplify this.
>
> >>
> >> - n = kp->clipcount;
> >> if (get_user(p, &up->clips))
> >> return -EFAULT;
> >> uclips = compat_ptr(p);
> >> - kclips = compat_alloc_user_space(n * sizeof(*kclips));
> >> - kp->clips = kclips;
> >> - while (n--) {
> >> + if (aux_space < clipcount * sizeof(*kclips))
> >> + return -EFAULT;
> >> + kclips = aux_buf;
> >> + if (put_user(kclips, &kp->clips))
> >> + return -EFAULT;
> >> +
> >> + while (clipcount--) {
> >> if (copy_in_user(&kclips->c, &uclips->c, sizeof(uclips->c)))
> >> return -EFAULT;
> >> - if (put_user(n ? kclips + 1 : NULL, &kclips->next))
> >> + if (put_user(clipcount ? kclips + 1 : NULL, &kclips->next))
> >> return -EFAULT;
> >> uclips++;
> >> kclips++;
> >> @@ -86,26 +98,28 @@ static int get_v4l2_window32(struct v4l2_window *kp,
> >> struct v4l2_window32 __user
> >> return 0;
> >> }
> >>
> >> -static int put_v4l2_window32(struct v4l2_window *kp, struct v4l2_window32
> >> __user *up)
> >> +static int put_v4l2_window32(struct v4l2_window __user *kp,
> >> + struct v4l2_window32 __user *up)
> >> {
> >> struct v4l2_clip __user *kclips = kp->clips;
> >> struct v4l2_clip32 __user *uclips;
> >> - int n = kp->clipcount;
> >> compat_caddr_t p;
> >> -
> >> - if (copy_to_user(&up->w, &kp->w, sizeof(kp->w)) ||
> >> - put_user(kp->field, &up->field) ||
> >> - put_user(kp->chromakey, &up->chromakey) ||
> >> - put_user(kp->clipcount, &up->clipcount) ||
> >> - put_user(kp->global_alpha, &up->global_alpha))
> >> + u32 clipcount;
> >> +
> >> + if (copy_in_user(&up->w, &kp->w, sizeof(kp->w)) ||
> >> + convert_in_user(&up->field, &kp->field) ||
> >> + convert_in_user(&up->chromakey, &kp->chromakey) ||
> >> + convert_in_user(&up->global_alpha, &kp->global_alpha) ||
> >> + get_user(clipcount, &kp->clipcount) ||
> >> + put_user(clipcount, &up->clipcount))
> >> return -EFAULT;
> >> - if (!kp->clipcount)
> >> + if (!clipcount)
> >> return 0;
> >>
> >> if (get_user(p, &up->clips))
> >> return -EFAULT;
> >> uclips = compat_ptr(p);
> >> - while (n--) {
> >> + while (clipcount--) {
> >> if (copy_in_user(&uclips->c, &kclips->c, sizeof(uclips->c)))
> >> return -EFAULT;
> >> uclips++;
> >> @@ -145,107 +159,161 @@ struct v4l2_create_buffers32 {
> >> __u32 reserved[8];
> >> };
> >>
> >> -static int __get_v4l2_format32(struct v4l2_format *kp, struct
> >> v4l2_format32 __user *up)
> >> +static int __bufsize_v4l2_format32(struct v4l2_format32 __user *up)
> >
> > This is effectively returning the size of the non-compat struct, or just
> > the part of which is variable. How about calling it __bufsize_v4l2_format
> > instead? The same for other such functions.
>
> Ack.
>
> > I'd also separate the size calculated by the function and the return value.
>
> That's cleaner, yes.
>
> >
> >> {
> >> - if (get_user(kp->type, &up->type))
> >> + u32 type;
> >> +
> >> + if (get_user(type, &up->type))
> >> return -EFAULT;
> >>
> >> - switch (kp->type) {
> >> + switch (type) {
> >> + case V4L2_BUF_TYPE_VIDEO_OVERLAY:
> >> + case V4L2_BUF_TYPE_VIDEO_OUTPUT_OVERLAY: {
> >> + u32 clipcount;
> >> +
> >> + if (get_user(clipcount, &up->fmt.win.clipcount))
> >> + return -EFAULT;
> >> + if (clipcount > 2048)
> >> + return -EINVAL;
> >> + return clipcount * sizeof(struct v4l2_clip);
> >> + }
> >> + default:
> >> + return 0;
> >> + }
> >> +}
> >> +
> >> +static int bufsize_v4l2_format32(struct v4l2_format32 __user *up)
> >> +{
> >> + if (!access_ok(VERIFY_READ, up, sizeof(*up)))
> >> + return -EFAULT;
> >
> > Newline?
> >
> >> + return __bufsize_v4l2_format32(up);
> >> +}
> >> +
> >> +static int __get_v4l2_format32(struct v4l2_format __user *kp,
> >> + struct v4l2_format32 __user *up,
> >> + void __user *aux_buf, int aux_space)
> >> +{
> >> + u32 type;
> >> +
> >> + if (get_user(type, &up->type) || put_user(type, &kp->type))
> >> + return -EFAULT;
> >> +
> >> + switch (type) {
> >> case V4L2_BUF_TYPE_VIDEO_CAPTURE:
> >> case V4L2_BUF_TYPE_VIDEO_OUTPUT:
> >> - return copy_from_user(&kp->fmt.pix, &up->fmt.pix,
> >> - sizeof(kp->fmt.pix)) ? -EFAULT : 0;
> >> + return copy_in_user(&kp->fmt.pix, &up->fmt.pix,
> >> + sizeof(kp->fmt.pix)) ? -EFAULT : 0;
> >> case V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE:
> >> case V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE:
> >> - return copy_from_user(&kp->fmt.pix_mp, &up->fmt.pix_mp,
> >> - sizeof(kp->fmt.pix_mp)) ? -EFAULT : 0;
> >> + return copy_in_user(&kp->fmt.pix_mp, &up->fmt.pix_mp,
> >> + sizeof(kp->fmt.pix_mp)) ? -EFAULT : 0;
> >> case V4L2_BUF_TYPE_VIDEO_OVERLAY:
> >> case V4L2_BUF_TYPE_VIDEO_OUTPUT_OVERLAY:
> >> - return get_v4l2_window32(&kp->fmt.win, &up->fmt.win);
> >> + return get_v4l2_window32(&kp->fmt.win, &up->fmt.win,
> >> + aux_buf, aux_space);
> >> case V4L2_BUF_TYPE_VBI_CAPTURE:
> >> case V4L2_BUF_TYPE_VBI_OUTPUT:
> >> - return copy_from_user(&kp->fmt.vbi, &up->fmt.vbi,
> >> - sizeof(kp->fmt.vbi)) ? -EFAULT : 0;
> >> + return copy_in_user(&kp->fmt.vbi, &up->fmt.vbi,
> >> + sizeof(kp->fmt.vbi)) ? -EFAULT : 0;
> >> case V4L2_BUF_TYPE_SLICED_VBI_CAPTURE:
> >> case V4L2_BUF_TYPE_SLICED_VBI_OUTPUT:
> >> - return copy_from_user(&kp->fmt.sliced, &up->fmt.sliced,
> >> - sizeof(kp->fmt.sliced)) ? -EFAULT : 0;
> >> + return copy_in_user(&kp->fmt.sliced, &up->fmt.sliced,
> >> + sizeof(kp->fmt.sliced)) ? -EFAULT : 0;
> >> case V4L2_BUF_TYPE_SDR_CAPTURE:
> >> case V4L2_BUF_TYPE_SDR_OUTPUT:
> >> - return copy_from_user(&kp->fmt.sdr, &up->fmt.sdr,
> >> - sizeof(kp->fmt.sdr)) ? -EFAULT : 0;
> >> + return copy_in_user(&kp->fmt.sdr, &up->fmt.sdr,
> >> + sizeof(kp->fmt.sdr)) ? -EFAULT : 0;
> >> case V4L2_BUF_TYPE_META_CAPTURE:
> >> - return copy_from_user(&kp->fmt.meta, &up->fmt.meta,
> >> - sizeof(kp->fmt.meta)) ? -EFAULT : 0;
> >> + return copy_in_user(&kp->fmt.meta, &up->fmt.meta,
> >> + sizeof(kp->fmt.meta)) ? -EFAULT : 0;
> >> default:
> >> return -EINVAL;
> >> }
> >> }
> >>
> >> -static int get_v4l2_format32(struct v4l2_format *kp, struct v4l2_format32
> >> __user *up)
> >> +static int get_v4l2_format32(struct v4l2_format __user *kp,
> >> + struct v4l2_format32 __user *up,
> >> + void __user *aux_buf, int aux_space)
> >> +{
> >> + if (!access_ok(VERIFY_READ, up, sizeof(*up)))
> >> + return -EFAULT;
> >> + return __get_v4l2_format32(kp, up, aux_buf, aux_space);
> >> +}
> >> +
> >> +static int bufsize_v4l2_create32(struct v4l2_create_buffers32 __user *up)
> >> {
> >> if (!access_ok(VERIFY_READ, up, sizeof(*up)))
> >> return -EFAULT;
> >> - return __get_v4l2_format32(kp, up);
> >> + return __bufsize_v4l2_format32(&up->format);
> >> }
> >>
> >> -static int get_v4l2_create32(struct v4l2_create_buffers *kp, struct
> >> v4l2_create_buffers32 __user *up)
> >> +static int get_v4l2_create32(struct v4l2_create_buffers __user *kp,
> >> + struct v4l2_create_buffers32 __user *up,
> >> + void __user *aux_buf, int aux_space)
> >> {
> >> if (!access_ok(VERIFY_READ, up, sizeof(*up)) ||
> >> - copy_from_user(kp, up, offsetof(struct v4l2_create_buffers32,
> >> format)))
> >> + copy_in_user(kp, up,
> >> + offsetof(struct v4l2_create_buffers32, format)))
> >> return -EFAULT;
> >> - return __get_v4l2_format32(&kp->format, &up->format);
> >> + return __get_v4l2_format32(&kp->format, &up->format,
> >> + aux_buf, aux_space);
> >> }
> >>
> >> -static int __put_v4l2_format32(struct v4l2_format *kp, struct
> >> v4l2_format32 __user *up)
> >> +static int __put_v4l2_format32(struct v4l2_format __user *kp,
> >> + struct v4l2_format32 __user *up)
> >> {
> >> - if (put_user(kp->type, &up->type))
> >> + u32 type;
> >> +
> >> + if (get_user(type, &kp->type))
> >> return -EFAULT;
> >>
> >> - switch (kp->type) {
> >> + switch (type) {
> >> case V4L2_BUF_TYPE_VIDEO_CAPTURE:
> >> case V4L2_BUF_TYPE_VIDEO_OUTPUT:
> >> - return copy_to_user(&up->fmt.pix, &kp->fmt.pix,
> >> - sizeof(kp->fmt.pix)) ? -EFAULT : 0;
> >> + return copy_in_user(&up->fmt.pix, &kp->fmt.pix,
> >> + sizeof(kp->fmt.pix)) ? -EFAULT : 0;
> >> case V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE:
> >> case V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE:
> >> - return copy_to_user(&up->fmt.pix_mp, &kp->fmt.pix_mp,
> >> - sizeof(kp->fmt.pix_mp)) ? -EFAULT : 0;
> >> + return copy_in_user(&up->fmt.pix_mp, &kp->fmt.pix_mp,
> >> + sizeof(kp->fmt.pix_mp)) ? -EFAULT : 0;
> >> case V4L2_BUF_TYPE_VIDEO_OVERLAY:
> >> case V4L2_BUF_TYPE_VIDEO_OUTPUT_OVERLAY:
> >> return put_v4l2_window32(&kp->fmt.win, &up->fmt.win);
> >> case V4L2_BUF_TYPE_VBI_CAPTURE:
> >> case V4L2_BUF_TYPE_VBI_OUTPUT:
> >> - return copy_to_user(&up->fmt.vbi, &kp->fmt.vbi,
> >> - sizeof(kp->fmt.vbi)) ? -EFAULT : 0;
> >> + return copy_in_user(&up->fmt.vbi, &kp->fmt.vbi,
> >> + sizeof(kp->fmt.vbi)) ? -EFAULT : 0;
> >> case V4L2_BUF_TYPE_SLICED_VBI_CAPTURE:
> >> case V4L2_BUF_TYPE_SLICED_VBI_OUTPUT:
> >> - return copy_to_user(&up->fmt.sliced, &kp->fmt.sliced,
> >> - sizeof(kp->fmt.sliced)) ? -EFAULT : 0;
> >> + return copy_in_user(&up->fmt.sliced, &kp->fmt.sliced,
> >> + sizeof(kp->fmt.sliced)) ? -EFAULT : 0;
> >> case V4L2_BUF_TYPE_SDR_CAPTURE:
> >> case V4L2_BUF_TYPE_SDR_OUTPUT:
> >> - return copy_to_user(&up->fmt.sdr, &kp->fmt.sdr,
> >> - sizeof(kp->fmt.sdr)) ? -EFAULT : 0;
> >> + return copy_in_user(&up->fmt.sdr, &kp->fmt.sdr,
> >> + sizeof(kp->fmt.sdr)) ? -EFAULT : 0;
> >> case V4L2_BUF_TYPE_META_CAPTURE:
> >> - return copy_to_user(&up->fmt.meta, &kp->fmt.meta,
> >> - sizeof(kp->fmt.meta)) ? -EFAULT : 0;
> >> + return copy_in_user(&up->fmt.meta, &kp->fmt.meta,
> >> + sizeof(kp->fmt.meta)) ? -EFAULT : 0;
> >> default:
> >> return -EINVAL;
> >> }
> >> }
> >>
> >> -static int put_v4l2_format32(struct v4l2_format *kp, struct v4l2_format32
> >> __user *up)
> >> +static int put_v4l2_format32(struct v4l2_format __user *kp,
> >> + struct v4l2_format32 __user *up)
> >> {
> >> if (!access_ok(VERIFY_WRITE, up, sizeof(*up)))
> >
> > For some pointers we do perform such checks, for others we don't. While
> > this patch doesn't change this, it'd be good to align this by either
> > introducing it everywhere or removing it everywhere.
> >
> > I guess that can be done separately.
> >
> >> return -EFAULT;
> >> return __put_v4l2_format32(kp, up);
> >> }
> >>
> >> -static int put_v4l2_create32(struct v4l2_create_buffers *kp, struct
> >> v4l2_create_buffers32 __user *up)
> >> +static int put_v4l2_create32(struct v4l2_create_buffers __user *kp,
> >> + struct v4l2_create_buffers32 __user *up)
> >> {
> >> if (!access_ok(VERIFY_WRITE, up, sizeof(*up)) ||
> >> - copy_to_user(up, kp, offsetof(struct v4l2_create_buffers32,
> >> format)) ||
> >> - copy_to_user(up->reserved, kp->reserved, sizeof(kp->reserved)))
> >> + copy_in_user(up, kp,
> >> + offsetof(struct v4l2_create_buffers32, format)) ||
> >> + copy_in_user(up->reserved, kp->reserved, sizeof(kp->reserved)))
> >> return -EFAULT;
> >> return __put_v4l2_format32(&kp->format, &up->format);
> >> }
> >> @@ -259,25 +327,27 @@ struct v4l2_standard32 {
> >> __u32 reserved[4];
> >> };
> >>
> >> -static int get_v4l2_standard32(struct v4l2_standard *kp, struct
> >> v4l2_standard32 __user *up)
> >> +static int get_v4l2_standard32(struct v4l2_standard __user *kp,
> >> + struct v4l2_standard32 __user *up)
> >> {
> >> /* other fields are not set by the user, nor used by the driver */
> >> if (!access_ok(VERIFY_READ, up, sizeof(*up)) ||
> >> - get_user(kp->index, &up->index))
> >> + convert_in_user(&kp->index, &up->index))
> >> return -EFAULT;
> >> return 0;
> >> }
> >>
> >> -static int put_v4l2_standard32(struct v4l2_standard *kp, struct
> >> v4l2_standard32 __user *up)
> >> +static int put_v4l2_standard32(struct v4l2_standard __user *kp,
> >> + struct v4l2_standard32 __user *up)
> >> {
> >> if (!access_ok(VERIFY_WRITE, up, sizeof(*up)) ||
> >> - put_user(kp->index, &up->index) ||
> >> - put_user(kp->id, &up->id) ||
> >> - copy_to_user(up->name, kp->name, 24) ||
> >> - copy_to_user(&up->frameperiod, &kp->frameperiod,
> >> - sizeof(kp->frameperiod)) ||
> >> - put_user(kp->framelines, &up->framelines) ||
> >> - copy_to_user(up->reserved, kp->reserved, sizeof(kp->reserved)))
> >> + convert_in_user(&up->index, &kp->index) ||
> >> + copy_in_user(&up->id, &kp->id, sizeof(up->id)) ||
> >
> > Why not convert_in_user()?
>
> Will change.
>
> >
> >> + copy_in_user(up->name, kp->name, 24) ||
>
> Urg. I missed this, this has a fixed number as the last argument instead of
> sizeof(kp->name).
Right; I didn't comment on this as it's not a property of this patch. I'd
actually fix this separately. Up to you. It's a very risky habit,
especially with reserved fields.
>
> >> + copy_in_user(&up->frameperiod, &kp->frameperiod,
> >> + sizeof(up->frameperiod)) ||
> >> + convert_in_user(&up->framelines, &kp->framelines) ||
> >> + copy_in_user(up->reserved, kp->reserved, sizeof(up->reserved)))
> >> return -EFAULT;
> >> return 0;
> >> }
> >> @@ -317,10 +387,10 @@ struct v4l2_buffer32 {
> >> __u32 reserved;
> >> };
> >>
> >> -static int get_v4l2_plane32(struct v4l2_plane __user *up, struct
> >> v4l2_plane32 __user *up32,
> >> +static int get_v4l2_plane32(struct v4l2_plane __user *up,
> >> + struct v4l2_plane32 __user *up32,
> >> enum v4l2_memory memory)
> >> {
> >> - void __user *up_pln;
> >> compat_long_t p;
> >
> > Shouldn't this be compat_ulong_t?
>
> That's better, yes.
>
> >
> >>
> >> if (copy_in_user(up, up32, 2 * sizeof(__u32)) ||
> >> @@ -336,10 +406,8 @@ static int get_v4l2_plane32(struct v4l2_plane __user
> >> *up, struct v4l2_plane32 __
> >> return -EFAULT;
> >> break;
> >> case V4L2_MEMORY_USERPTR:
> >> - if (get_user(p, &up32->m.userptr))
> >> - return -EFAULT;
> >> - up_pln = compat_ptr(p);
> >> - if (put_user((unsigned long)up_pln, &up->m.userptr))
> >> + if (get_user(p, &up32->m.userptr) ||
> >> + put_user((unsigned long)compat_ptr(p), &up->m.userptr))
> >> return -EFAULT;
> >> break;
> >> case V4L2_MEMORY_DMABUF:
> >> @@ -351,7 +419,8 @@ static int get_v4l2_plane32(struct v4l2_plane __user
> >> *up, struct v4l2_plane32 __
> >> return 0;
> >> }
> >>
> >> -static int put_v4l2_plane32(struct v4l2_plane __user *up, struct
> >> v4l2_plane32 __user *up32,
> >> +static int put_v4l2_plane32(struct v4l2_plane __user *up,
> >> + struct v4l2_plane32 __user *up32,
> >> enum v4l2_memory memory)
> >> {
> >> unsigned long p;
> >> @@ -375,8 +444,7 @@ static int put_v4l2_plane32(struct v4l2_plane __user
> >> *up, struct v4l2_plane32 __
> >> return -EFAULT;
> >> break;
> >> case V4L2_MEMORY_DMABUF:
> >> - if (copy_in_user(&up32->m.fd, &up->m.fd,
> >> - sizeof(up->m.fd)))
> >> + if (copy_in_user(&up32->m.fd, &up->m.fd, sizeof(up->m.fd)))
> >> return -EFAULT;
> >> break;
> >> }
> >> @@ -384,37 +452,68 @@ static int put_v4l2_plane32(struct v4l2_plane __user
> >> *up, struct v4l2_plane32 __
> >> return 0;
> >> }
> >>
> >> -static int get_v4l2_buffer32(struct v4l2_buffer *kp, struct v4l2_buffer32
> >> __user *up)
> >> +static int bufsize_v4l2_buffer32(struct v4l2_buffer32 __user *up)
> >> +{
> >> + u32 type;
> >> + u32 length;
> >> +
> >> + if (!access_ok(VERIFY_READ, up, sizeof(*up)) ||
> >> + get_user(type, &up->type) ||
> >> + get_user(length, &up->length))
> >> + return -EFAULT;
> >> +
> >> + if (V4L2_TYPE_IS_MULTIPLANAR(type)) {
> >> + if (length > VIDEO_MAX_PLANES)
> >> + return -EINVAL;
> >> +
> >> + /* We don't really care if userspace decides to kill itself
> >
> > /*
> > * ...
> >
> >> + * by passing a very big length value
> >> + */
> >> + return length * sizeof(struct v4l2_plane);
> >> + }
> >> + return 0;
> >> +}
> >> +
> >> +static int get_v4l2_buffer32(struct v4l2_buffer __user *kp,
> >> + struct v4l2_buffer32 __user *up,
> >> + void __user *aux_buf, int aux_space)
> >> {
> >> + u32 type;
> >> + u32 length;
> >> + enum v4l2_memory memory;
> >> struct v4l2_plane32 __user *uplane32;
> >> struct v4l2_plane __user *uplane;
> >> compat_caddr_t p;
> >> + int num_planes;
> >
> > u32?
> >
> >> int ret;
> >>
> >> if (!access_ok(VERIFY_READ, up, sizeof(*up)) ||
> >> - get_user(kp->index, &up->index) ||
> >> - get_user(kp->type, &up->type) ||
> >> - get_user(kp->flags, &up->flags) ||
> >> - get_user(kp->memory, &up->memory) ||
> >> - get_user(kp->length, &up->length))
> >> + convert_in_user(&kp->index, &up->index) ||
> >> + get_user(type, &up->type) ||
> >> + put_user(type, &kp->type) ||
> >> + convert_in_user(&kp->flags, &up->flags) ||
> >> + get_user(memory, &up->memory) ||
> >> + put_user(memory, &kp->memory) ||
> >> + get_user(length, &up->length) ||
> >> + put_user(length, &kp->length))
> >> return -EFAULT;
> >>
> >> - if (V4L2_TYPE_IS_OUTPUT(kp->type))
> >> - if (get_user(kp->bytesused, &up->bytesused) ||
> >> - get_user(kp->field, &up->field) ||
> >> - get_user(kp->timestamp.tv_sec, &up->timestamp.tv_sec) ||
> >> - get_user(kp->timestamp.tv_usec, &up->timestamp.tv_usec))
> >> + if (V4L2_TYPE_IS_OUTPUT(type))
> >> + if (convert_in_user(&kp->bytesused, &up->bytesused) ||
> >> + convert_in_user(&kp->field, &up->field) ||
> >> + convert_in_user(&kp->timestamp.tv_sec,
> >> + &up->timestamp.tv_sec) ||
> >> + convert_in_user(&kp->timestamp.tv_usec,
> >> + &up->timestamp.tv_usec))
> >> return -EFAULT;
> >>
> >> - if (V4L2_TYPE_IS_MULTIPLANAR(kp->type)) {
> >> - unsigned int num_planes;
> >> -
> >> - if (kp->length == 0) {
> >> - kp->m.planes = NULL;
> >> + if (V4L2_TYPE_IS_MULTIPLANAR(type)) {
> >> + num_planes = length;
> >
> > Do you need a local variable for just that?
> >
> >> + if (num_planes == 0) {
> >> /* num_planes == 0 is legal, e.g. when userspace doesn't
> >> * need planes array on DQBUF*/
> >> - return 0;
> >> - } else if (kp->length > VIDEO_MAX_PLANES) {
> >> + return put_user(NULL, &kp->m.planes);
> >> + } else if (num_planes > VIDEO_MAX_PLANES) {
> >> return -EINVAL;
> >> }
> >>
> >> @@ -423,40 +522,47 @@ static int get_v4l2_buffer32(struct v4l2_buffer *kp,
> >> struct v4l2_buffer32 __user
> >>
> >> uplane32 = compat_ptr(p);
> >> if (!access_ok(VERIFY_READ, uplane32,
> >> - kp->length * sizeof(*uplane32)))
> >> + num_planes * sizeof(*uplane32)))
> >
> > Alignment.
> >
> >> return -EFAULT;
> >>
> >> /* We don't really care if userspace decides to kill itself
> >> * by passing a very big num_planes value */
> >> - uplane = compat_alloc_user_space(kp->length * sizeof(*uplane));
> >> - kp->m.planes = (__force struct v4l2_plane *)uplane;
> >> + if (aux_space < num_planes * sizeof(*uplane))
> >> + return -EFAULT;
> >> +
> >> + uplane = aux_buf;
> >> + if (put_user((__force struct v4l2_plane *)uplane,
> >> + &kp->m.planes))
> >> + return -EFAULT;
> >>
> >> - for (num_planes = 0; num_planes < kp->length; num_planes++) {
> >> - ret = get_v4l2_plane32(uplane, uplane32, kp->memory);
> >> + while (--num_planes >= 0) {
> >
> > while (num_planes--) {
> >
> > ?
>
> Yes. I missed this one :-)
>
> >
> >> + ret = get_v4l2_plane32(uplane, uplane32, memory);
> >> if (ret)
> >> return ret;
> >> - ++uplane;
> >> - ++uplane32;
> >> + uplane++;
> >> + uplane32++;
> >> }
> >> } else {
> >> - switch (kp->memory) {
> >> + switch (memory) {
> >> case V4L2_MEMORY_MMAP:
> >> - case V4L2_MEMORY_OVERLAY:
> >> - if (get_user(kp->m.offset, &up->m.offset))
> >> + if (convert_in_user(&kp->m.offset, &up->m.offset))
> >> return -EFAULT;
> >> break;
> >> - case V4L2_MEMORY_USERPTR:
> >> - {
> >> - compat_long_t tmp;
> >> -
> >> - if (get_user(tmp, &up->m.userptr))
> >> - return -EFAULT;
> >> + case V4L2_MEMORY_USERPTR: {
> >> + compat_long_t tmp;
> >
> > compat_ulong_t, I'd call it "userptr". The latter is entirely up to you.
> >
> >>
> >> - kp->m.userptr = (unsigned long)compat_ptr(tmp);
> >> - }
> >> + if (get_user(tmp, &up->m.userptr) ||
> >> + put_user((unsigned long)compat_ptr(tmp),
> >> + &kp->m.userptr))
> >> + return -EFAULT;
> >> + break;
> >> + }
> >> + case V4L2_MEMORY_OVERLAY:
> >> + if (convert_in_user(&kp->m.offset, &up->m.offset))
> >> + return -EFAULT;
> >
> > Is there a reason to split this off from MMAP handling? The code is the
> > same.
>
> Huh, I missed this as well. I merged MMAP and OVERLAY elsewhere.
>
> >
> >> break;
> >> case V4L2_MEMORY_DMABUF:
> >> - if (get_user(kp->m.fd, &up->m.fd))
> >> + if (convert_in_user(&kp->m.fd, &up->m.fd))
> >> return -EFAULT;
> >> break;
> >> }
> >> @@ -465,8 +571,12 @@ static int get_v4l2_buffer32(struct v4l2_buffer *kp,
> >> struct v4l2_buffer32 __user
> >> return 0;
> >> }
> >>
> >> -static int put_v4l2_buffer32(struct v4l2_buffer *kp, struct v4l2_buffer32
> >> __user *up)
> >> +static int put_v4l2_buffer32(struct v4l2_buffer __user *kp,
> >> + struct v4l2_buffer32 __user *up)
> >> {
> >> + u32 type;
> >> + u32 length;
> >> + enum v4l2_memory memory;
> >> struct v4l2_plane32 __user *uplane32;
> >> struct v4l2_plane __user *uplane;
> >> compat_caddr_t p;
> >> @@ -474,53 +584,60 @@ static int put_v4l2_buffer32(struct v4l2_buffer *kp,
> >> struct v4l2_buffer32 __user
> >> int ret;
> >>
> >> if (!access_ok(VERIFY_WRITE, up, sizeof(*up)) ||
> >> - put_user(kp->index, &up->index) ||
> >> - put_user(kp->type, &up->type) ||
> >> - put_user(kp->flags, &up->flags) ||
> >> - put_user(kp->memory, &up->memory))
> >> + convert_in_user(&up->index, &kp->index) ||
> >> + get_user(type, &kp->type) ||
> >> + put_user(type, &up->type) ||
> >> + convert_in_user(&up->flags, &kp->flags) ||
> >> + get_user(memory, &kp->memory) ||
> >> + put_user(memory, &up->memory))
> >> return -EFAULT;
> >>
> >> - if (put_user(kp->bytesused, &up->bytesused) ||
> >> - put_user(kp->field, &up->field) ||
> >> - put_user(kp->timestamp.tv_sec, &up->timestamp.tv_sec) ||
> >> - put_user(kp->timestamp.tv_usec, &up->timestamp.tv_usec) ||
> >> - copy_to_user(&up->timecode, &kp->timecode, sizeof(kp->timecode)) ||
> >> - put_user(kp->sequence, &up->sequence) ||
> >> - put_user(kp->reserved2, &up->reserved2) ||
> >> - put_user(kp->reserved, &up->reserved) ||
> >> - put_user(kp->length, &up->length))
> >> + if (convert_in_user(&up->bytesused, &kp->bytesused) ||
> >> + convert_in_user(&up->field, &kp->field) ||
> >> + convert_in_user(&up->timestamp.tv_sec, &kp->timestamp.tv_sec) ||
> >> + convert_in_user(&up->timestamp.tv_usec, &kp->timestamp.tv_usec) ||
> >> + copy_in_user(&up->timecode, &kp->timecode, sizeof(kp->timecode)) ||
> >> + convert_in_user(&up->sequence, &kp->sequence) ||
> >> + convert_in_user(&up->reserved2, &kp->reserved2) ||
> >> + convert_in_user(&up->reserved, &kp->reserved) ||
> >> + get_user(length, &kp->length) ||
> >> + put_user(length, &up->length))
> >> return -EFAULT;
> >>
> >> - if (V4L2_TYPE_IS_MULTIPLANAR(kp->type)) {
> >> - num_planes = kp->length;
> >> + if (V4L2_TYPE_IS_MULTIPLANAR(type)) {
> >> + num_planes = length;
> >> if (num_planes == 0)
> >> return 0;
> >>
> >> - uplane = (__force struct v4l2_plane __user *)kp->m.planes;
> >> + if (get_user(uplane, ((__force struct v4l2_plane __user
> >> **)&kp->m.planes)))
> >> + return -EFAULT;
> >> if (get_user(p, &up->m.planes))
> >> return -EFAULT;
> >> uplane32 = compat_ptr(p);
> >>
> >> while (--num_planes >= 0) {
> >
> >
> > while (num_planes--) {
> >
> > Or rather: s/num_planes/length/
> >
> > Then you can remove num_planes.
>
> I will probably keep num_planes. It's more descriptive than 'length'.
You have two u32 local variables for the same purpose. I'd suggest to leave
either.
>
> >
> >> - ret = put_v4l2_plane32(uplane, uplane32, kp->memory);
> >> + ret = put_v4l2_plane32(uplane, uplane32, memory);
> >> if (ret)
> >> return ret;
> >> ++uplane;
> >> ++uplane32;
> >> }
> >> } else {
> >> - switch (kp->memory) {
> >> + switch (memory) {
> >> case V4L2_MEMORY_MMAP:
> >> - case V4L2_MEMORY_OVERLAY:
> >> - if (put_user(kp->m.offset, &up->m.offset))
> >> + if (convert_in_user(&up->m.offset, &kp->m.offset))
> >> return -EFAULT;
> >> break;
> >> case V4L2_MEMORY_USERPTR:
> >> - if (put_user(kp->m.userptr, &up->m.userptr))
> >> + if (convert_in_user(&up->m.userptr, &kp->m.userptr))
> >> + return -EFAULT;
> >> + break;
> >> + case V4L2_MEMORY_OVERLAY:
> >> + if (convert_in_user(&up->m.offset, &kp->m.offset))
> >
> > Combine with MMAP case?
>
> Hmm. I did combine this, but I probably lost that change after some later
> patch modifications. Thanks for noticing.
>
> >
> >> return -EFAULT;
> >> break;
> >> case V4L2_MEMORY_DMABUF:
> >> - if (put_user(kp->m.fd, &up->m.fd))
> >> + if (convert_in_user(&up->m.fd, &kp->m.fd))
> >> return -EFAULT;
> >> break;
> >> }
> >> @@ -545,29 +662,32 @@ struct v4l2_framebuffer32 {
> >> } fmt;
> >> };
> >>
> >> -static int get_v4l2_framebuffer32(struct v4l2_framebuffer *kp, struct
> >> v4l2_framebuffer32 __user *up)
> >> +static int get_v4l2_framebuffer32(struct v4l2_framebuffer __user *kp,
> >> + struct v4l2_framebuffer32 __user *up)
> >> {
> >> - u32 tmp;
> >> + compat_caddr_t tmp;
> >>
> >> if (!access_ok(VERIFY_READ, up, sizeof(*up)) ||
> >> get_user(tmp, &up->base) ||
> >> - get_user(kp->capability, &up->capability) ||
> >> - get_user(kp->flags, &up->flags) ||
> >> - copy_from_user(&kp->fmt, &up->fmt, sizeof(up->fmt)))
> >> + put_user((__force void *)compat_ptr(tmp), &kp->base) ||
> >> + convert_in_user(&kp->capability, &up->capability) ||
> >> + convert_in_user(&kp->flags, &up->flags) ||
> >> + copy_in_user(&kp->fmt, &up->fmt, sizeof(kp->fmt)))
> >> return -EFAULT;
> >> - kp->base = (__force void *)compat_ptr(tmp);
> >> return 0;
> >> }
> >>
> >> -static int put_v4l2_framebuffer32(struct v4l2_framebuffer *kp, struct
> >> v4l2_framebuffer32 __user *up)
> >> +static int put_v4l2_framebuffer32(struct v4l2_framebuffer __user *kp,
> >> + struct v4l2_framebuffer32 __user *up)
> >> {
> >> - u32 tmp = (u32)((unsigned long)kp->base);
> >> + void *base;
> >>
> >> if (!access_ok(VERIFY_WRITE, up, sizeof(*up)) ||
> >> - put_user(tmp, &up->base) ||
> >> - put_user(kp->capability, &up->capability) ||
> >> - put_user(kp->flags, &up->flags) ||
> >> - copy_to_user(&up->fmt, &kp->fmt, sizeof(up->fmt)))
> >> + get_user(base, &kp->base) ||
> >> + put_user(ptr_to_compat(base), &up->base) ||
> >> + convert_in_user(&up->capability, &kp->capability) ||
> >> + convert_in_user(&up->flags, &kp->flags) ||
> >> + copy_in_user(&up->fmt, &kp->fmt, sizeof(kp->fmt)))
> >> return -EFAULT;
> >> return 0;
> >> }
> >> @@ -586,16 +706,18 @@ struct v4l2_input32 {
> >>
> >> /* The 64-bit v4l2_input struct has extra padding at the end of the
> >> struct.
> >> Otherwise it is identical to the 32-bit version. */
> >> -static inline int get_v4l2_input32(struct v4l2_input *kp, struct
> >> v4l2_input32 __user *up)
> >> +static inline int get_v4l2_input32(struct v4l2_input __user *kp,
> >> + struct v4l2_input32 __user *up)
> >> {
> >> - if (copy_from_user(kp, up, sizeof(*up)))
> >> + if (copy_in_user(kp, up, sizeof(*up)))
> >> return -EFAULT;
> >> return 0;
> >> }
> >>
> >> -static inline int put_v4l2_input32(struct v4l2_input *kp, struct
> >> v4l2_input32 __user *up)
> >> +static inline int put_v4l2_input32(struct v4l2_input __user *kp,
> >> + struct v4l2_input32 __user *up)
> >> {
> >> - if (copy_to_user(up, kp, sizeof(*up)))
> >> + if (copy_in_user(up, kp, sizeof(*up)))
> >> return -EFAULT;
> >> return 0;
> >> }
> >> @@ -648,39 +770,58 @@ static inline bool ctrl_is_pointer(struct file
> >> *file, u32 id)
> >> return !ret && (qec.flags & V4L2_CTRL_FLAG_HAS_PAYLOAD);
> >> }
> >>
> >> +static int bufsize_v4l2_ext_controls32(struct v4l2_ext_controls32 __user
> >> *up)
> >> +{
> >> + u32 count;
> >> +
> >> + if (!access_ok(VERIFY_READ, up, sizeof(*up)) ||
> >> + get_user(count, &up->count))
> >> + return -EFAULT;
> >> + if (count > V4L2_CID_MAX_CTRLS)
> >> + return -EINVAL;
> >> + return count * sizeof(struct v4l2_ext_control);
> >> +}
> >> +
> >> static int get_v4l2_ext_controls32(struct file *file,
> >> - struct v4l2_ext_controls *kp,
> >> - struct v4l2_ext_controls32 __user *up)
> >> + struct v4l2_ext_controls __user *kp,
> >> + struct v4l2_ext_controls32 __user *up,
> >> + void __user *aux_buf, int aux_space)
> >> {
> >> struct v4l2_ext_control32 __user *ucontrols;
> >> struct v4l2_ext_control __user *kcontrols;
> >> + u32 count;
> >> unsigned int n;
> >
> > u32 n
> >
> >> compat_caddr_t p;
> >>
> >> if (!access_ok(VERIFY_READ, up, sizeof(*up)) ||
> >> - get_user(kp->which, &up->which) ||
> >> - get_user(kp->count, &up->count) ||
> >> - get_user(kp->error_idx, &up->error_idx) ||
> >> - copy_from_user(kp->reserved, up->reserved, sizeof(kp->reserved)))
> >> + convert_in_user(&kp->which, &up->which) ||
> >> + get_user(count, &up->count) ||
> >> + put_user(count, &kp->count) ||
> >> + convert_in_user(&kp->error_idx, &up->error_idx) ||
> >> + copy_in_user(kp->reserved, up->reserved, sizeof(kp->reserved)))
> >> return -EFAULT;
> >> - if (kp->count == 0) {
> >> - kp->controls = NULL;
> >> - return 0;
> >> - } else if (kp->count > V4L2_CID_MAX_CTRLS) {
> >> + if (count == 0)
> >> + return put_user(NULL, &kp->controls);
> >> + else if (kp->count > V4L2_CID_MAX_CTRLS)
> >
> > count
^
Could you comment on this one...
> >
> >> return -EINVAL;
> >> - }
> >> if (get_user(p, &up->controls))
> >> return -EFAULT;
> >> ucontrols = compat_ptr(p);
> >> - if (!access_ok(VERIFY_READ, ucontrols, kp->count * sizeof(*ucontrols)))
> >> + if (!access_ok(VERIFY_READ, ucontrols, count * sizeof(*ucontrols)))
^
...for, as far as I see, this may overflow?
> >> + return -EFAULT;
> >> + if (aux_space < count * sizeof(*kcontrols))
> >> return -EFAULT;
> >> - kcontrols = compat_alloc_user_space(kp->count * sizeof(*kcontrols));
> >> - kp->controls = (__force struct v4l2_ext_control *)kcontrols;
> >> - for (n = 0; n < kp->count; n++) {
> >> + kcontrols = aux_buf;
> >> + if (put_user((__force struct v4l2_ext_control *)kcontrols,
> >> + &kp->controls))
> >> + return -EFAULT;
> >> +
> >> + for (n = 0; n < count; n++) {
> >> u32 id;
> >>
> >> if (copy_in_user(kcontrols, ucontrols, sizeof(*ucontrols)))
> >> return -EFAULT;
> >> +
> >> if (get_user(id, &kcontrols->id))
> >> return -EFAULT;
> >
> > Newline here, too? Not really a problem with this patch though.
> >
> >> if (ctrl_is_pointer(file, id)) {
> >> @@ -699,36 +840,47 @@ static int get_v4l2_ext_controls32(struct file *file,
> >> }
> >>
> >> static int put_v4l2_ext_controls32(struct file *file,
> >> - struct v4l2_ext_controls *kp,
> >> + struct v4l2_ext_controls __user *kp,
> >> struct v4l2_ext_controls32 __user *up)
> >> {
> >> struct v4l2_ext_control32 __user *ucontrols;
> >> - struct v4l2_ext_control __user *kcontrols =
> >> - (__force struct v4l2_ext_control __user *)kp->controls;
> >> - int n = kp->count;
> >> + struct v4l2_ext_control __user *kcontrols;
> >> + u32 count;
> >> + unsigned int n;
> >
> > u32 n
> >
> >> compat_caddr_t p;
> >>
> >> if (!access_ok(VERIFY_WRITE, up, sizeof(*up)) ||
> >> - put_user(kp->which, &up->which) ||
> >> - put_user(kp->count, &up->count) ||
> >> - put_user(kp->error_idx, &up->error_idx) ||
> >> - copy_to_user(up->reserved, kp->reserved, sizeof(up->reserved)))
> >> + get_user(kcontrols, &kp->controls) ||
> >
> > How about doing this as last? "controls" is the last member of the struct.
> >
> >> + convert_in_user(&up->which, &kp->which) ||
> >> + get_user(count, &kp->count) ||
> >> + put_user(count, &up->count) ||
> >> + convert_in_user(&up->error_idx, &kp->error_idx) ||
> >> + copy_in_user(up->reserved, kp->reserved, sizeof(up->reserved)))
> >> return -EFAULT;
> >> - if (!kp->count)
> >> + if (!count)
> >> return 0;
> >>
> >> if (get_user(p, &up->controls))
> >> return -EFAULT;
> >> ucontrols = compat_ptr(p);
> >> - if (!access_ok(VERIFY_WRITE, ucontrols, n * sizeof(*ucontrols)))
> >> + if (!access_ok(VERIFY_WRITE, ucontrols, count * sizeof(*ucontrols)))
> >> return -EFAULT;
> >>
> >> - while (--n >= 0) {
> >> - unsigned size = sizeof(*ucontrols);
> >> + for (n = 0; n < count; n++) {
> >> + unsigned int size = sizeof(*ucontrols);
> >> u32 id;
> >>
> >> + if (copy_in_user(&ucontrols->id, &kcontrols->id,
> >> + sizeof(ucontrols->id)) ||
> >
> > get_user(id, &kcontrols->id) ||
> > put_user(id, &ucontrols->id) ||
> >
> >> + copy_in_user(&ucontrols->size, &kcontrols->size,
> >> + sizeof(ucontrols->size)) ||
> >
> > convert_in_user() ?
> >
> >> + copy_in_user(&ucontrols->reserved2, &kcontrols->reserved2,
> >> + sizeof(ucontrols->reserved2)))
> >> + return -EFAULT;
> >> +
> >> if (get_user(id, &kcontrols->id))
> >> return -EFAULT;
> >
> > ...and the above if () can be removed.
> >
> >> +
> >> /* Do not modify the pointer when copying a pointer control.
> >> The contents of the pointer was changed, not the pointer
> >> itself. */
> >> @@ -736,6 +888,7 @@ static int put_v4l2_ext_controls32(struct file *file,
> >> size -= sizeof(ucontrols->value64);
> >
> > This chunk is yearning for newlines. Not a fault of this patch though.
> >
> >> if (copy_in_user(ucontrols, kcontrols, size))
> >> return -EFAULT;
> >> +
> >> ucontrols++;
> >> kcontrols++;
> >> }
> >> @@ -755,17 +908,18 @@ struct v4l2_event32 {
> >> __u32 reserved[8];
> >> };
> >>
> >> -static int put_v4l2_event32(struct v4l2_event *kp, struct v4l2_event32
> >> __user *up)
> >> +static int put_v4l2_event32(struct v4l2_event __user *kp,
> >> + struct v4l2_event32 __user *up)
> >> {
> >> if (!access_ok(VERIFY_WRITE, up, sizeof(*up)) ||
> >> - put_user(kp->type, &up->type) ||
> >> - copy_to_user(&up->u, &kp->u, sizeof(kp->u)) ||
> >> - put_user(kp->pending, &up->pending) ||
> >> - put_user(kp->sequence, &up->sequence) ||
> >> - put_user(kp->timestamp.tv_sec, &up->timestamp.tv_sec) ||
> >> - put_user(kp->timestamp.tv_nsec, &up->timestamp.tv_nsec) ||
> >> - put_user(kp->id, &up->id) ||
> >> - copy_to_user(up->reserved, kp->reserved, sizeof(kp->reserved)))
> >> + convert_in_user(&up->type, &kp->type) ||
> >> + copy_in_user(&up->u, &kp->u, sizeof(kp->u)) ||
> >> + convert_in_user(&up->pending, &kp->pending) ||
> >> + convert_in_user(&up->sequence, &kp->sequence) ||
> >> + convert_in_user(&up->timestamp.tv_sec, &kp->timestamp.tv_sec) ||
> >> + convert_in_user(&up->timestamp.tv_nsec, &kp->timestamp.tv_nsec) ||
> >> + convert_in_user(&up->id, &kp->id) ||
> >> + copy_in_user(up->reserved, kp->reserved, sizeof(up->reserved)))
> >> return -EFAULT;
> >> return 0;
> >> }
> >> @@ -778,31 +932,34 @@ struct v4l2_edid32 {
> >> compat_caddr_t edid;
> >> };
> >>
> >> -static int get_v4l2_edid32(struct v4l2_edid *kp, struct v4l2_edid32
> >> __user *up)
> >> +static int get_v4l2_edid32(struct v4l2_edid __user *kp,
> >> + struct v4l2_edid32 __user *up)
> >> {
> >> - u32 tmp;
> >> + compat_uptr_t tmp;
> >>
> >> if (!access_ok(VERIFY_READ, up, sizeof(*up)) ||
> >> - get_user(kp->pad, &up->pad) ||
> >> - get_user(kp->start_block, &up->start_block) ||
> >> - get_user(kp->blocks, &up->blocks) ||
> >> + convert_in_user(&kp->pad, &up->pad) ||
> >> + convert_in_user(&kp->start_block, &up->start_block) ||
> >> + convert_in_user(&kp->blocks, &up->blocks) ||
> >> get_user(tmp, &up->edid) ||
> >> - copy_from_user(kp->reserved, up->reserved, sizeof(kp->reserved)))
> >> + put_user(compat_ptr(tmp), &kp->edid) ||
> >> + copy_in_user(kp->reserved, up->reserved, sizeof(kp->reserved)))
> >> return -EFAULT;
> >> - kp->edid = (__force u8 *)compat_ptr(tmp);
> >> return 0;
> >> }
> >>
> >> -static int put_v4l2_edid32(struct v4l2_edid *kp, struct v4l2_edid32
> >> __user *up)
> >> +static int put_v4l2_edid32(struct v4l2_edid __user *kp,
> >> + struct v4l2_edid32 __user *up)
> >> {
> >> - u32 tmp = (u32)((unsigned long)kp->edid);
> >> + void *edid;
> >>
> >> if (!access_ok(VERIFY_WRITE, up, sizeof(*up)) ||
> >> - put_user(kp->pad, &up->pad) ||
> >> - put_user(kp->start_block, &up->start_block) ||
> >> - put_user(kp->blocks, &up->blocks) ||
> >> - put_user(tmp, &up->edid) ||
> >> - copy_to_user(up->reserved, kp->reserved, sizeof(up->reserved)))
> >> + convert_in_user(&up->pad, &kp->pad) ||
> >> + convert_in_user(&up->start_block, &kp->start_block) ||
> >> + convert_in_user(&up->blocks, &kp->blocks) ||
> >> + get_user(edid, &kp->edid) ||
> >> + put_user(ptr_to_compat(edid), &up->edid) ||
> >> + copy_in_user(up->reserved, kp->reserved, sizeof(up->reserved)))
> >> return -EFAULT;
> >> return 0;
> >> }
> >> @@ -835,22 +992,30 @@ static int put_v4l2_edid32(struct v4l2_edid *kp,
> >> struct v4l2_edid32 __user *up)
> >> #define VIDIOC_G_OUTPUT32 _IOR ('V', 46, s32)
> >> #define VIDIOC_S_OUTPUT32 _IOWR('V', 47, s32)
> >>
> >> +static void __user *alloc_userspace(unsigned int size, int aux_space,
> >> long *err)
> >
> > This one is weird. Rather than following the pattern and returning an error
> > and assigning the result to a pointer passed to it, it does exactly the
> > opposite. I'd change that. I guess this is a matter of opinion to some
> > degree at least, though.
>
> I agree, I'll change that.
>
> >
> >> +{
> >> + void __user *up_native;
> >> +
> >> + if (aux_space < 0) {
> >> + *err = aux_space;
> >> + return NULL;
> >> + }
> >> + up_native = compat_alloc_user_space(size + aux_space);
> >> + if (!up_native)
> >> + *err = -ENOMEM;
> >> + else if (clear_user(up_native, size))
> >> + *err = -EFAULT;
> >> + else
> >> + return up_native;
> >> + return NULL;
> >> +}
> >> +
> >> static long do_video_ioctl(struct file *file, unsigned int cmd, unsigned
> >> long arg)
> >> {
> >> - union {
> >> - struct v4l2_format v2f;
> >> - struct v4l2_buffer v2b;
> >> - struct v4l2_framebuffer v2fb;
> >> - struct v4l2_input v2i;
> >> - struct v4l2_standard v2s;
> >> - struct v4l2_ext_controls v2ecs;
> >> - struct v4l2_event v2ev;
> >> - struct v4l2_create_buffers v2crt;
> >> - struct v4l2_edid v2edid;
> >> - unsigned long vx;
> >> - int vi;
> >> - } karg;
> >> void __user *up = compat_ptr(arg);
> >> + void __user *up_native = NULL;
> >> + void __user *aux_buf;
> >> + int aux_space;
> >
> > size_t?
> >
> >> int compatible_arg = 1;
> >> long err = 0;
> >>
> >> @@ -889,30 +1054,47 @@ static long do_video_ioctl(struct file *file,
> >> unsigned int cmd, unsigned long ar
> >> case VIDIOC_STREAMOFF:
> >> case VIDIOC_S_INPUT:
> >> case VIDIOC_S_OUTPUT:
> >> - err = get_user(karg.vi, (s32 __user *)up);
> >> + up_native = alloc_userspace(sizeof(unsigned int __user),
> >> + 0, &err);
> >> + if (!err && convert_in_user((unsigned int __user *)up_native,
> >> + (compat_uint_t __user *)up))
> >> + return -EFAULT;
> >> compatible_arg = 0;
> >> break;
> >>
> >> case VIDIOC_G_INPUT:
> >> case VIDIOC_G_OUTPUT:
> >> + up_native = alloc_userspace(sizeof(unsigned int __user),
> >> + 0, &err);
> >> compatible_arg = 0;
> >> break;
> >>
> >> case VIDIOC_G_EDID:
> >> case VIDIOC_S_EDID:
> >> - err = get_v4l2_edid32(&karg.v2edid, up);
> >> + up_native = alloc_userspace(sizeof(struct v4l2_edid), 0, &err);
> >> + err = err ? : get_v4l2_edid32(up_native, up);
> >
> > I'd write this as:
> >
> > if (!err)
> > err = get_v4l2_edid32(native_up, up);
> >
> > Same below.
> >
> >> compatible_arg = 0;
> >> break;
> >>
> >> case VIDIOC_G_FMT:
> >> case VIDIOC_S_FMT:
> >> case VIDIOC_TRY_FMT:
> >> - err = get_v4l2_format32(&karg.v2f, up);
> >> + aux_space = bufsize_v4l2_format32(up);
> >> + up_native = alloc_userspace(sizeof(struct v4l2_format),
> >> + aux_space, &err);
> >> + aux_buf = up_native + sizeof(struct v4l2_format);
> >> + err = err ? : get_v4l2_format32(up_native, up,
> >> + aux_buf, aux_space);
> >> compatible_arg = 0;
> >> break;
> >>
> >> case VIDIOC_CREATE_BUFS:
> >> - err = get_v4l2_create32(&karg.v2crt, up);
> >> + aux_space = bufsize_v4l2_create32(up);
> >> + up_native = alloc_userspace(sizeof(struct v4l2_create_buffers),
> >> + aux_space, &err);
> >> + aux_buf = up_native + sizeof(struct v4l2_create_buffers);
> >> + err = err ? : get_v4l2_create32(up_native, up,
> >> + aux_buf, aux_space);
> >> compatible_arg = 0;
> >> break;
> >>
> >> @@ -920,36 +1102,54 @@ static long do_video_ioctl(struct file *file,
> >> unsigned int cmd, unsigned long ar
> >> case VIDIOC_QUERYBUF:
> >> case VIDIOC_QBUF:
> >> case VIDIOC_DQBUF:
> >> - err = get_v4l2_buffer32(&karg.v2b, up);
> >> + aux_space = bufsize_v4l2_buffer32(up);
> >> + up_native = alloc_userspace(sizeof(struct v4l2_buffer),
> >> + aux_space, &err);
> >> + aux_buf = up_native + sizeof(struct v4l2_buffer);
> >> + err = err ? : get_v4l2_buffer32(up_native, up,
> >> + aux_buf, aux_space);
> >> compatible_arg = 0;
> >> break;
> >>
> >> case VIDIOC_S_FBUF:
> >> - err = get_v4l2_framebuffer32(&karg.v2fb, up);
> >> + up_native = alloc_userspace(sizeof(struct v4l2_framebuffer),
> >> + 0, &err);
> >> + err = err ? : get_v4l2_framebuffer32(up_native, up);
> >> compatible_arg = 0;
> >> break;
> >>
> >> case VIDIOC_G_FBUF:
> >> + up_native = alloc_userspace(sizeof(struct v4l2_framebuffer),
> >> + 0, &err);
> >> compatible_arg = 0;
> >> break;
> >>
> >> case VIDIOC_ENUMSTD:
> >> - err = get_v4l2_standard32(&karg.v2s, up);
> >> + up_native = alloc_userspace(sizeof(struct v4l2_standard),
> >> + 0, &err);
> >> + err = err ? : get_v4l2_standard32(up_native, up);
> >> compatible_arg = 0;
> >> break;
> >>
> >> case VIDIOC_ENUMINPUT:
> >> - err = get_v4l2_input32(&karg.v2i, up);
> >> + up_native = alloc_userspace(sizeof(struct v4l2_input), 0, &err);
> >> + err = err ? : get_v4l2_input32(up_native, up);
> >> compatible_arg = 0;
> >> break;
> >>
> >> case VIDIOC_G_EXT_CTRLS:
> >> case VIDIOC_S_EXT_CTRLS:
> >> case VIDIOC_TRY_EXT_CTRLS:
> >> - err = get_v4l2_ext_controls32(file, &karg.v2ecs, up);
> >> + aux_space = bufsize_v4l2_ext_controls32(up);
> >> + up_native = alloc_userspace(sizeof(struct v4l2_ext_controls),
> >> + aux_space, &err);
> >> + aux_buf = up_native + sizeof(struct v4l2_ext_controls);
> >> + err = err ? : get_v4l2_ext_controls32(file, up_native, up,
> >> + aux_buf, aux_space);
> >> compatible_arg = 0;
> >> break;
> >> case VIDIOC_DQEVENT:
> >> + up_native = alloc_userspace(sizeof(struct v4l2_event), 0, &err);
> >> compatible_arg = 0;
> >> break;
> >> }
> >> @@ -958,13 +1158,8 @@ static long do_video_ioctl(struct file *file,
> >> unsigned int cmd, unsigned long ar
> >>
> >> if (compatible_arg)
> >> err = native_ioctl(file, cmd, (unsigned long)up);
> >> - else {
> >> - mm_segment_t old_fs = get_fs();
> >> -
> >> - set_fs(KERNEL_DS);
> >> - err = native_ioctl(file, cmd, (unsigned long)&karg);
> >> - set_fs(old_fs);
> >> - }
> >> + else
> >> + err = native_ioctl(file, cmd, (unsigned long)up_native);
> >>
> >> if (err == -ENOTTY || err == -EFAULT || err == -ENOIOCTLCMD)
> >> return err;
> >> @@ -976,11 +1171,11 @@ 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:
> >> - if (put_v4l2_ext_controls32(file, &karg.v2ecs, up))
> >> + if (put_v4l2_ext_controls32(file, up_native, up))
> >> err = -EFAULT;
> >> break;
> >> case VIDIOC_S_EDID:
> >> - if (put_v4l2_edid32(&karg.v2edid, up))
> >> + if (put_v4l2_edid32(up_native, up))
> >> err = -EFAULT;
> >> break;
> >> }
> >> @@ -992,44 +1187,45 @@ static long do_video_ioctl(struct file *file,
> >> unsigned int cmd, unsigned long ar
> >> case VIDIOC_S_OUTPUT:
> >> case VIDIOC_G_INPUT:
> >> case VIDIOC_G_OUTPUT:
> >> - err = put_user(((s32)karg.vi), (s32 __user *)up);
> >> + err = convert_in_user((compat_uint_t __user *)up,
> >> + ((unsigned int __user *)up_native));
> >
> > Do note that convert_in_user() returns Boolean. This will effectively yield
> > 1 in case of an error here. How about changing convert_in_user() to return
> > whatever get_user / put_user do, to avoid errors in the future?
>
> That's better indeed. Good catch.
>
> >
> >> break;
> >>
> >> case VIDIOC_G_FBUF:
> >> - err = put_v4l2_framebuffer32(&karg.v2fb, up);
> >> + err = put_v4l2_framebuffer32(up_native, up);
> >> break;
> >>
> >> case VIDIOC_DQEVENT:
> >> - err = put_v4l2_event32(&karg.v2ev, up);
> >> + err = put_v4l2_event32(up_native, up);
> >> break;
> >>
> >> case VIDIOC_G_EDID:
> >> - err = put_v4l2_edid32(&karg.v2edid, up);
> >> + err = put_v4l2_edid32(up_native, up);
> >> break;
> >>
> >> case VIDIOC_G_FMT:
> >> case VIDIOC_S_FMT:
> >> case VIDIOC_TRY_FMT:
> >> - err = put_v4l2_format32(&karg.v2f, up);
> >> + err = put_v4l2_format32(up_native, up);
> >> break;
> >>
> >> case VIDIOC_CREATE_BUFS:
> >> - err = put_v4l2_create32(&karg.v2crt, up);
> >> + err = put_v4l2_create32(up_native, up);
> >> break;
> >>
> >> case VIDIOC_PREPARE_BUF:
> >> case VIDIOC_QUERYBUF:
> >> case VIDIOC_QBUF:
> >> case VIDIOC_DQBUF:
> >> - err = put_v4l2_buffer32(&karg.v2b, up);
> >> + err = put_v4l2_buffer32(up_native, up);
> >> break;
> >>
> >> case VIDIOC_ENUMSTD:
> >> - err = put_v4l2_standard32(&karg.v2s, up);
> >> + err = put_v4l2_standard32(up_native, up);
> >> break;
> >>
> >> case VIDIOC_ENUMINPUT:
> >> - err = put_v4l2_input32(&karg.v2i, up);
> >> + err = put_v4l2_input32(up_native, up);
> >> break;
> >> }
> >> return err;
> >
>
> Thanks for the thorough review! Much appreciated.
You're welcome. And thanks for working on this!
--
Regards,
Sakari Ailus
e-mail: sakari.ai...@iki.fi
