Hi Sakari,
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?
>
>>
>> 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?
Done.
>
>> +({ \
>> + 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?
I stuck to __assign_tmp, I think __convert_in_user_tmp is overly paranoid :-)
>
>> + \
>> + get_user(val, from) || put_user(val, to); \
I'm keeping this. I thought about changing || to '? :' but I chickened out
making
that change. I'll fix the G/S_INPUT/OUTPUT handling instead.
>> +})
>> +
>> 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?
Changed.
>
>>
>> - 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.
Done.
>
> I'd also separate the size calculated by the function and the return value.
Done. Much nicer indeed.
>
>> {
>> - 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?
Didn't do this. I didn't think it improved the code.
>
>> + 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.
Didn't touch this. It wouldn't hurt to review the code. I'll see if I can do
that later this week.
>
>> 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()?
Changed.
>
>> + copy_in_user(up->name, kp->name, 24) ||
>> + 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?
Changed.
>
>>
>> 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
>
> /*
> * ...
Changed here and elsewhere.
>
>> + * 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?
Changed.
>
>> 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?
I moved the u32 num_planes declaration above to here. I still kept it
though since I think it helps understand what 'length' stands for.
>
>> + 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.
Changed.
>
>> 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--) {
>
> ?
Changed.
>
>> + 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.
Changed.
>
>>
>> - 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.
Changed.
>
>> 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--) {
Changed.
>
> Or rather: s/num_planes/length/
>
> Then you can remove num_planes.
But kept num_planes.
>
>> - 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?
Changed.
>
>> 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
Changed.
>
>> 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
Changed.
>
>> 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)))
>> + 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.
Changed.
>
>> 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
Changed.
>
>> 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.
Changed.
>
>> + 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) ||
Changed.
>
>> + copy_in_user(&ucontrols->size, &kcontrols->size,
>> + sizeof(ucontrols->size)) ||
>
> convert_in_user() ?
Changed.
>
>> + 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.
Done.
>
>> +
>> /* 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.
Changed. It much nicer now.
>
>> +{
>> + 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?
I went with u32 :-)
>
>> 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.
Done.
>
>> 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?
As mentioned at the top I decided not to change convert_in_user (now
assign_in_user)
and modified this code here instead.
>
>> 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;
>
Regards,
Hans
