Re: [PATCH v2 1/4] media: v4l2-compat-ioctl32: fix several __user annotations
Hi Mauro,
Thanks for the update. Just a few comments below...
On Thu, Apr 19, 2018 at 12:33:29PM -0400, Mauro Carvalho Chehab wrote:
> Smatch report several issues with bad __user annotations:
>
> drivers/media/v4l2-core/v4l2-compat-ioctl32.c:447:21: warning: incorrect
> type in argument 1 (different address spaces)
> drivers/media/v4l2-core/v4l2-compat-ioctl32.c:447:21:expected void
> [noderef] *uptr
> drivers/media/v4l2-core/v4l2-compat-ioctl32.c:447:21:got void *
> drivers/media/v4l2-core/v4l2-compat-ioctl32.c:621:21: warning: incorrect
> type in argument 1 (different address spaces)
> drivers/media/v4l2-core/v4l2-compat-ioctl32.c:621:21:expected void
> const volatile [noderef] *
> drivers/media/v4l2-core/v4l2-compat-ioctl32.c:621:21:got struct
> v4l2_plane [noderef] **
> drivers/media/v4l2-core/v4l2-compat-ioctl32.c:693:13: warning: incorrect
> type in argument 1 (different address spaces)
> drivers/media/v4l2-core/v4l2-compat-ioctl32.c:693:13:expected void
> [noderef] *uptr
> drivers/media/v4l2-core/v4l2-compat-ioctl32.c:693:13:got void
> *[assigned] base
> drivers/media/v4l2-core/v4l2-compat-ioctl32.c:871:13: warning: incorrect
> type in assignment (different address spaces)
> drivers/media/v4l2-core/v4l2-compat-ioctl32.c:871:13:expected struct
> v4l2_ext_control [noderef] *kcontrols
> drivers/media/v4l2-core/v4l2-compat-ioctl32.c:871:13:got struct
> v4l2_ext_control *
> drivers/media/v4l2-core/v4l2-compat-ioctl32.c:957:13: warning: incorrect
> type in assignment (different address spaces)
> drivers/media/v4l2-core/v4l2-compat-ioctl32.c:957:13:expected unsigned
> char [usertype] *__pu_val
> drivers/media/v4l2-core/v4l2-compat-ioctl32.c:957:13:got void [noderef]
> *
> drivers/media/v4l2-core/v4l2-compat-ioctl32.c:973:13: warning: incorrect
> type in argument 1 (different address spaces)
> drivers/media/v4l2-core/v4l2-compat-ioctl32.c:973:13:expected void
> [noderef] *uptr
> drivers/media/v4l2-core/v4l2-compat-ioctl32.c:973:13:got void
> *[assigned] edid
>
> Fix them.
>
> Signed-off-by: Mauro Carvalho Chehab
> ---
> drivers/media/v4l2-core/v4l2-compat-ioctl32.c | 51
> ++-
> 1 file changed, 35 insertions(+), 16 deletions(-)
>
> diff --git a/drivers/media/v4l2-core/v4l2-compat-ioctl32.c
> b/drivers/media/v4l2-core/v4l2-compat-ioctl32.c
> index d03a44d89649..51c7c5ab15ef 100644
> --- a/drivers/media/v4l2-core/v4l2-compat-ioctl32.c
> +++ b/drivers/media/v4l2-core/v4l2-compat-ioctl32.c
> @@ -443,8 +443,8 @@ static int put_v4l2_plane32(struct v4l2_plane __user *up,
> return -EFAULT;
> break;
> case V4L2_MEMORY_USERPTR:
> - if (get_user(p, &up->m.userptr) ||
> - put_user((compat_ulong_t)ptr_to_compat((__force void *)p),
> + if (get_user(p, &up->m.userptr)||
Space before "||", i.e. as it was?
> + put_user((compat_ulong_t)ptr_to_compat((void __user *)p),
>&up32->m.userptr))
> return -EFAULT;
> break;
> @@ -587,7 +587,7 @@ static int put_v4l2_buffer32(struct v4l2_buffer __user
> *kp,
> u32 length;
> enum v4l2_memory memory;
> struct v4l2_plane32 __user *uplane32;
> - struct v4l2_plane __user *uplane;
> + struct v4l2_plane *uplane;
> compat_caddr_t p;
> int ret;
>
> @@ -617,15 +617,22 @@ static int put_v4l2_buffer32(struct v4l2_buffer __user
> *kp,
>
> if (num_planes == 0)
> return 0;
> -
> - if (get_user(uplane, ((__force struct v4l2_plane __user
> **)&kp->m.planes)))
> + /* We need to define uplane without __user, even though
/*
* ...
> + * it does point to data in userspace here. The reason is
> + * that v4l2-ioctl.c copies it from userspace to kernelspace,
> + * so its definition in videodev2.h doesn't have a
> + * __user markup. Defining uplane with __user causes
> + * smatch warnings, so instead declare it without __user
> + * and cast it as a userspace pointer to put_v4l2_plane32().
> + */
> + if (get_user(uplane, &kp->m.planes))
This line looks much better indeed...
> return -EFAULT;
> if (get_user(p, &up->m.planes))
> return -EFAULT;
> uplane32 = compat_ptr(p);
>
> while (num_planes--) {
> - ret = put_v4l2_plane32(uplane, uplane32, memory);
> + ret = put_v4l2_plane32((void __user *)uplane, uplane32,
> memory);
Over 80; please wrap.
> if (ret)
> return ret;
> ++uplane;
> @@ -675,7 +682,7 @@ static int get_v4l2_framebuffer32(struct v4l2_framebuffer
> __user *kp,
>
> if (!access_ok(VERIFY_READ,
Re: [PATCH v2 1/4] media: v4l2-compat-ioctl32: fix several __user annotations
On 04/19/18 18:33, Mauro Carvalho Chehab wrote:
> Smatch report several issues with bad __user annotations:
>
> drivers/media/v4l2-core/v4l2-compat-ioctl32.c:447:21: warning: incorrect
> type in argument 1 (different address spaces)
> drivers/media/v4l2-core/v4l2-compat-ioctl32.c:447:21:expected void
> [noderef] *uptr
> drivers/media/v4l2-core/v4l2-compat-ioctl32.c:447:21:got void *
> drivers/media/v4l2-core/v4l2-compat-ioctl32.c:621:21: warning: incorrect
> type in argument 1 (different address spaces)
> drivers/media/v4l2-core/v4l2-compat-ioctl32.c:621:21:expected void
> const volatile [noderef] *
> drivers/media/v4l2-core/v4l2-compat-ioctl32.c:621:21:got struct
> v4l2_plane [noderef] **
> drivers/media/v4l2-core/v4l2-compat-ioctl32.c:693:13: warning: incorrect
> type in argument 1 (different address spaces)
> drivers/media/v4l2-core/v4l2-compat-ioctl32.c:693:13:expected void
> [noderef] *uptr
> drivers/media/v4l2-core/v4l2-compat-ioctl32.c:693:13:got void
> *[assigned] base
> drivers/media/v4l2-core/v4l2-compat-ioctl32.c:871:13: warning: incorrect
> type in assignment (different address spaces)
> drivers/media/v4l2-core/v4l2-compat-ioctl32.c:871:13:expected struct
> v4l2_ext_control [noderef] *kcontrols
> drivers/media/v4l2-core/v4l2-compat-ioctl32.c:871:13:got struct
> v4l2_ext_control *
> drivers/media/v4l2-core/v4l2-compat-ioctl32.c:957:13: warning: incorrect
> type in assignment (different address spaces)
> drivers/media/v4l2-core/v4l2-compat-ioctl32.c:957:13:expected unsigned
> char [usertype] *__pu_val
> drivers/media/v4l2-core/v4l2-compat-ioctl32.c:957:13:got void [noderef]
> *
> drivers/media/v4l2-core/v4l2-compat-ioctl32.c:973:13: warning: incorrect
> type in argument 1 (different address spaces)
> drivers/media/v4l2-core/v4l2-compat-ioctl32.c:973:13:expected void
> [noderef] *uptr
> drivers/media/v4l2-core/v4l2-compat-ioctl32.c:973:13:got void
> *[assigned] edid
>
> Fix them.
>
> Signed-off-by: Mauro Carvalho Chehab
Reviewed-by: Hans Verkuil
Regards,
Hans
> ---
> drivers/media/v4l2-core/v4l2-compat-ioctl32.c | 51
> ++-
> 1 file changed, 35 insertions(+), 16 deletions(-)
>
> diff --git a/drivers/media/v4l2-core/v4l2-compat-ioctl32.c
> b/drivers/media/v4l2-core/v4l2-compat-ioctl32.c
> index d03a44d89649..51c7c5ab15ef 100644
> --- a/drivers/media/v4l2-core/v4l2-compat-ioctl32.c
> +++ b/drivers/media/v4l2-core/v4l2-compat-ioctl32.c
> @@ -443,8 +443,8 @@ static int put_v4l2_plane32(struct v4l2_plane __user *up,
> return -EFAULT;
> break;
> case V4L2_MEMORY_USERPTR:
> - if (get_user(p, &up->m.userptr) ||
> - put_user((compat_ulong_t)ptr_to_compat((__force void *)p),
> + if (get_user(p, &up->m.userptr)||
> + put_user((compat_ulong_t)ptr_to_compat((void __user *)p),
>&up32->m.userptr))
> return -EFAULT;
> break;
> @@ -587,7 +587,7 @@ static int put_v4l2_buffer32(struct v4l2_buffer __user
> *kp,
> u32 length;
> enum v4l2_memory memory;
> struct v4l2_plane32 __user *uplane32;
> - struct v4l2_plane __user *uplane;
> + struct v4l2_plane *uplane;
> compat_caddr_t p;
> int ret;
>
> @@ -617,15 +617,22 @@ static int put_v4l2_buffer32(struct v4l2_buffer __user
> *kp,
>
> if (num_planes == 0)
> return 0;
> -
> - if (get_user(uplane, ((__force struct v4l2_plane __user
> **)&kp->m.planes)))
> + /* We need to define uplane without __user, even though
> + * it does point to data in userspace here. The reason is
> + * that v4l2-ioctl.c copies it from userspace to kernelspace,
> + * so its definition in videodev2.h doesn't have a
> + * __user markup. Defining uplane with __user causes
> + * smatch warnings, so instead declare it without __user
> + * and cast it as a userspace pointer to put_v4l2_plane32().
> + */
> + if (get_user(uplane, &kp->m.planes))
> return -EFAULT;
> if (get_user(p, &up->m.planes))
> return -EFAULT;
> uplane32 = compat_ptr(p);
>
> while (num_planes--) {
> - ret = put_v4l2_plane32(uplane, uplane32, memory);
> + ret = put_v4l2_plane32((void __user *)uplane, uplane32,
> memory);
> if (ret)
> return ret;
> ++uplane;
> @@ -675,7 +682,7 @@ static int get_v4l2_framebuffer32(struct v4l2_framebuffer
> __user *kp,
>
> if (!access_ok(VERIFY_READ, up, sizeof(*up)) ||
> get_user(tmp, &up->base) ||
> - put_user((__force void *)compat_ptr(tmp), &kp->base) ||
> + put_us
[PATCH v2 1/4] media: v4l2-compat-ioctl32: fix several __user annotations
Smatch report several issues with bad __user annotations:
drivers/media/v4l2-core/v4l2-compat-ioctl32.c:447:21: warning: incorrect type
in argument 1 (different address spaces)
drivers/media/v4l2-core/v4l2-compat-ioctl32.c:447:21:expected void
[noderef] *uptr
drivers/media/v4l2-core/v4l2-compat-ioctl32.c:447:21:got void *
drivers/media/v4l2-core/v4l2-compat-ioctl32.c:621:21: warning: incorrect type
in argument 1 (different address spaces)
drivers/media/v4l2-core/v4l2-compat-ioctl32.c:621:21:expected void const
volatile [noderef] *
drivers/media/v4l2-core/v4l2-compat-ioctl32.c:621:21:got struct
v4l2_plane [noderef] **
drivers/media/v4l2-core/v4l2-compat-ioctl32.c:693:13: warning: incorrect type
in argument 1 (different address spaces)
drivers/media/v4l2-core/v4l2-compat-ioctl32.c:693:13:expected void
[noderef] *uptr
drivers/media/v4l2-core/v4l2-compat-ioctl32.c:693:13:got void *[assigned]
base
drivers/media/v4l2-core/v4l2-compat-ioctl32.c:871:13: warning: incorrect type
in assignment (different address spaces)
drivers/media/v4l2-core/v4l2-compat-ioctl32.c:871:13:expected struct
v4l2_ext_control [noderef] *kcontrols
drivers/media/v4l2-core/v4l2-compat-ioctl32.c:871:13:got struct
v4l2_ext_control *
drivers/media/v4l2-core/v4l2-compat-ioctl32.c:957:13: warning: incorrect type
in assignment (different address spaces)
drivers/media/v4l2-core/v4l2-compat-ioctl32.c:957:13:expected unsigned
char [usertype] *__pu_val
drivers/media/v4l2-core/v4l2-compat-ioctl32.c:957:13:got void [noderef]
*
drivers/media/v4l2-core/v4l2-compat-ioctl32.c:973:13: warning: incorrect type
in argument 1 (different address spaces)
drivers/media/v4l2-core/v4l2-compat-ioctl32.c:973:13:expected void
[noderef] *uptr
drivers/media/v4l2-core/v4l2-compat-ioctl32.c:973:13:got void *[assigned]
edid
Fix them.
Signed-off-by: Mauro Carvalho Chehab
---
drivers/media/v4l2-core/v4l2-compat-ioctl32.c | 51 ++-
1 file changed, 35 insertions(+), 16 deletions(-)
diff --git a/drivers/media/v4l2-core/v4l2-compat-ioctl32.c
b/drivers/media/v4l2-core/v4l2-compat-ioctl32.c
index d03a44d89649..51c7c5ab15ef 100644
--- a/drivers/media/v4l2-core/v4l2-compat-ioctl32.c
+++ b/drivers/media/v4l2-core/v4l2-compat-ioctl32.c
@@ -443,8 +443,8 @@ static int put_v4l2_plane32(struct v4l2_plane __user *up,
return -EFAULT;
break;
case V4L2_MEMORY_USERPTR:
- if (get_user(p, &up->m.userptr) ||
- put_user((compat_ulong_t)ptr_to_compat((__force void *)p),
+ if (get_user(p, &up->m.userptr)||
+ put_user((compat_ulong_t)ptr_to_compat((void __user *)p),
&up32->m.userptr))
return -EFAULT;
break;
@@ -587,7 +587,7 @@ static int put_v4l2_buffer32(struct v4l2_buffer __user *kp,
u32 length;
enum v4l2_memory memory;
struct v4l2_plane32 __user *uplane32;
- struct v4l2_plane __user *uplane;
+ struct v4l2_plane *uplane;
compat_caddr_t p;
int ret;
@@ -617,15 +617,22 @@ static int put_v4l2_buffer32(struct v4l2_buffer __user
*kp,
if (num_planes == 0)
return 0;
-
- if (get_user(uplane, ((__force struct v4l2_plane __user
**)&kp->m.planes)))
+ /* We need to define uplane without __user, even though
+* it does point to data in userspace here. The reason is
+* that v4l2-ioctl.c copies it from userspace to kernelspace,
+* so its definition in videodev2.h doesn't have a
+* __user markup. Defining uplane with __user causes
+* smatch warnings, so instead declare it without __user
+* and cast it as a userspace pointer to put_v4l2_plane32().
+*/
+ if (get_user(uplane, &kp->m.planes))
return -EFAULT;
if (get_user(p, &up->m.planes))
return -EFAULT;
uplane32 = compat_ptr(p);
while (num_planes--) {
- ret = put_v4l2_plane32(uplane, uplane32, memory);
+ ret = put_v4l2_plane32((void __user *)uplane, uplane32,
memory);
if (ret)
return ret;
++uplane;
@@ -675,7 +682,7 @@ static int get_v4l2_framebuffer32(struct v4l2_framebuffer
__user *kp,
if (!access_ok(VERIFY_READ, up, sizeof(*up)) ||
get_user(tmp, &up->base) ||
- put_user((__force void *)compat_ptr(tmp), &kp->base) ||
+ put_user((void __force *)compat_ptr(tmp), &kp->base) ||
assign_in_user(&kp->capability, &up->capability) ||
assign_in_user(&kp->flags, &up->flags) ||
copy_in_user(&kp->fmt, &up->fmt, sizeof(kp->fmt)))
@@
