__user with scalar data types

__user with scalar data types

[Jordan Crouse]

A number of us over in DRM land have been using __u64 scalar types
to store pointers for uapi structures in accordance with Daniel Vetter's
now classic treatise on ioctls:

http://blog.ffwll.ch/2013/11/botching-up-ioctls.html

A smaller number of us have further been marking the __u64 with __user,
to wit:

struct uapistruct {
	...
	__u64 __user myptr;
	---
};

And then converting it for use in the kernel as such:

{
	void __user *userptr = (void __user *)(uintptr_t)args->myptr;

	copy_from_user(local, userptr, size);
	...
}

The problem is that sparse doesn't like the momentary switch to
uintptr_t:

	warning: dereference of noderef expression

Which raised a bikeshed debate over whether it is appropriate to mark a scalar
type as __user.  My opinion is that it is appropriate because __user should mark
user memory regardless of the container.

I'm looking for opinions or semi-authoritative edicts to determine if we should
either start changing our uapi headers or go off and try to figure out how to
make sparse understand this particular usage.

Thanks!
Jordan
-- 
The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: __user with scalar data types

[Al Viro]

On Mon, Jun 19, 2017 at 10:15:09AM -0600, Jordan Crouse wrote:

> Which raised a bikeshed debate over whether it is appropriate to mark a scalar
> type as __user.  My opinion is that it is appropriate because __user should mark
> user memory regardless of the container.

What the hell?  __user is a qualifier like const, volatile, etc.  It's a property
of *pointer* *type*.  Not some nebulous "marks userland memory" thing.

> I'm looking for opinions or semi-authoritative edicts to determine if we should
> either start changing our uapi headers or go off and try to figure out how to
> make sparse understand this particular usage.

Stop cargo-culting, please.

Re: __user with scalar data types

[Luc Van Oostenryck]

On Mon, Jun 19, 2017 at 10:15:09AM -0600, Jordan Crouse wrote:
> A number of us over in DRM land have been using __u64 scalar types
> to store pointers for uapi structures in accordance with Daniel Vetter's
> now classic treatise on ioctls:
> 
> http://blog.ffwll.ch/2013/11/botching-up-ioctls.html
> 
> A smaller number of us have further been marking the __u64 with __user,
> to wit:
> 
> struct uapistruct {
> 	...
> 	__u64 __user myptr;
> 	---
> };

It wouldn't make sense to have this:
	struct uapistruct {
		__u64 __user myptr;
		__u64        anothermember;
	};
In other words, eiter all members are in the user address space
or none are. So, a struct member should not be marked __user
(exactly as for 'const' or 'volatile').

It wouldn't also make sense to move the __user to the whole struct,
giving something like:
	struct uapistruct {
		__u64 myptr;
		__u64 anothermember;
	} __user;
because it's not the type that belong to the user address space
but some specific objects.

Of course, your real problem here is that you're using a __u64 to
store a pointer and then expect this __u64 to have some properties
unique to pointers.


-- Luc Van Oostenryck (sparse hacker)

Re: __user with scalar data types

[Luc Van Oostenryck]

On Mon, Jun 19, 2017 at 10:15:09AM -0600, Jordan Crouse wrote:
> struct uapistruct {
> 	...
> 	__u64 __user myptr;
> 	---
> };
> 
> And then converting it for use in the kernel as such:
> 
> {
> 	void __user *userptr = (void __user *)(uintptr_t)args->myptr;
> 
> 	copy_from_user(local, userptr, size);
> 	...
> }
> 
> The problem is that sparse doesn't like the momentary switch to
> uintptr_t:
> 
> 	warning: dereference of noderef expression

This warning doesn't come from the cast to uintptr_t but
simply from dereferencing the field which can't be dereferenced
since it's marked as '__user'. In other words, doing
'args->myptr' rightfully trigger the warning and no cast
will or should stop that.

Also, you can't expect the '__user' to be transmitted from
'myptr' to the pointer (without taking the address of 'myptr').
It's exactly like 'const int' vs. 'const int *': the '__user' or
the 'const' is not at the same level in the type hierarchy
('const object' vs. 'non-const pointer to const object').


-- Luc Van Oostenryck

Re: __user with scalar data types

[Al Viro]

On Mon, Jun 19, 2017 at 10:32:18PM +0200, Luc Van Oostenryck wrote:
> On Mon, Jun 19, 2017 at 10:15:09AM -0600, Jordan Crouse wrote:
> > struct uapistruct {
> > 	...
> > 	__u64 __user myptr;
> > 	---
> > };
> > 
> > And then converting it for use in the kernel as such:
> > 
> > {
> > 	void __user *userptr = (void __user *)(uintptr_t)args->myptr;
> > 
> > 	copy_from_user(local, userptr, size);
> > 	...
> > }
> > 
> > The problem is that sparse doesn't like the momentary switch to
> > uintptr_t:
> > 
> > 	warning: dereference of noderef expression
> 
> This warning doesn't come from the cast to uintptr_t but
> simply from dereferencing the field which can't be dereferenced
> since it's marked as '__user'. In other words, doing
> 'args->myptr' rightfully trigger the warning and no cast
> will or should stop that.
> 
> Also, you can't expect the '__user' to be transmitted from
> 'myptr' to the pointer (without taking the address of 'myptr').
> It's exactly like 'const int' vs. 'const int *': the '__user' or
> the 'const' is not at the same level in the type hierarchy
> ('const object' vs. 'non-const pointer to const object').

Besides, suppose you add a special type for that.  How would it
have to behave, really?  AFAICS, you want something similar to
__bitwise, except that (assuming this type is T)
	T + integer => T
	T - integer => T
	T & integer => integer
	T | integer => T
	T - T => integer (quietly decay to underlying type for both
arguments, then treat as normal -)
	T & T => T (probably, but might be worth a warning)
	T | T => T (ditto)
	comparison - same as for __bitwise
	constant conversion: 0 should convert clean, anything else - a warning
	cast to pointer => warn unless the target type is __user?  But that's
not going to help with cast through uintptr_t...
	?: as usual
	any other arithmetics => warn and decay to underlying integer type

It might be not impossible to implement, but it sure as hell won't be __user
and it'll need careful thinking about the semantics of those annotations.
The outline above is just that - figuring out if there are any nasty corner
cases will take some work.

Re: __user with scalar data types

[Luc Van Oostenryck]

On Mon, Jun 19, 2017 at 09:46:37PM +0100, Al Viro wrote:
> On Mon, Jun 19, 2017 at 10:32:18PM +0200, Luc Van Oostenryck wrote:
> > On Mon, Jun 19, 2017 at 10:15:09AM -0600, Jordan Crouse wrote:
> > > struct uapistruct {
> > > 	...
> > > 	__u64 __user myptr;
> > > 	---
> > > };
> > > 
> > > And then converting it for use in the kernel as such:
> > > 
> > > {
> > > 	void __user *userptr = (void __user *)(uintptr_t)args->myptr;
> > > 
> > > 	copy_from_user(local, userptr, size);
> > > 	...
> > > }
> > > 
> > > The problem is that sparse doesn't like the momentary switch to
> > > uintptr_t:
> > > 
> > > 	warning: dereference of noderef expression
> > 
> > This warning doesn't come from the cast to uintptr_t but
> > simply from dereferencing the field which can't be dereferenced
> > since it's marked as '__user'. In other words, doing
> > 'args->myptr' rightfully trigger the warning and no cast
> > will or should stop that.
> > 
> > Also, you can't expect the '__user' to be transmitted from
> > 'myptr' to the pointer (without taking the address of 'myptr').
> > It's exactly like 'const int' vs. 'const int *': the '__user' or
> > the 'const' is not at the same level in the type hierarchy
> > ('const object' vs. 'non-const pointer to const object').
> 
> Besides, suppose you add a special type for that.  How would it
> have to behave, really?  AFAICS, you want something similar to
> __bitwise, except that (assuming this type is T)
> 	T + integer => T
> 	T - integer => T
> 	T & integer => integer
> 	T | integer => T
> 	T - T => integer (quietly decay to underlying type for both
> arguments, then treat as normal -)
> 	T & T => T (probably, but might be worth a warning)
> 	T | T => T (ditto)
> 	comparison - same as for __bitwise
> 	constant conversion: 0 should convert clean, anything else - a warning
> 	cast to pointer => warn unless the target type is __user?  But that's
> not going to help with cast through uintptr_t...
> 	?: as usual
> 	any other arithmetics => warn and decay to underlying integer type

And how it should behave with typeof()?
Because it's already unclear to me what should be the result of:
	typeof(X {__user,__noderef,__nocast,__bitwise} [*])
and I don't think sparse do the right thing with this.

That said, I'm of the opinion that simply thinking about implementing this
special type is close to a capital sin.

-- Luc

Re: __user with scalar data types

[Daniel Vetter]

On Mon, Jun 19, 2017 at 6:34 PM, Al Viro <viro@zeniv.linux.org.uk> wrote:
> On Mon, Jun 19, 2017 at 10:15:09AM -0600, Jordan Crouse wrote:
>
>> Which raised a bikeshed debate over whether it is appropriate to mark a scalar
>> type as __user.  My opinion is that it is appropriate because __user should mark
>> user memory regardless of the container.
>
> What the hell?  __user is a qualifier like const, volatile, etc.  It's a property
> of *pointer* *type*.  Not some nebulous "marks userland memory" thing.
>
>> I'm looking for opinions or semi-authoritative edicts to determine if we should
>> either start changing our uapi headers or go off and try to figure out how to
>> make sparse understand this particular usage.
>
> Stop cargo-culting, please.

Yep that's cargo-culted, but from a quick grep only msm and qxl
headers do this (the other __user annotations in uapi/drm are for
pointers, where it's correct). Adding those maintainers.

Also, if you use u64_to_user_ptr helper macro sparse should have
caught this (if not we'd need to improve the macro).
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: __user with scalar data types

[Gerd Hoffmann]

[-- Attachment #1: Type: text/plain, Size: 470 bytes --]

  Hi,

> Yep that's cargo-culted, but from a quick grep only msm and qxl
> headers do this (the other __user annotations in uapi/drm are for
> pointers, where it's correct). Adding those maintainers.

Yep, those looks pointless indeed.

> Also, if you use u64_to_user_ptr helper macro sparse should have
> caught this (if not we'd need to improve the macro).

And qxl should actually use it.

Fix attached (compile-tested only so far), does that look ok?

cheers,
  Gerd

[-- Attachment #2: Type: text/x-patch, Size: 2827 bytes --]

diff --git a/include/uapi/drm/qxl_drm.h b/include/uapi/drm/qxl_drm.h
index 7eef422130..880999d2d8 100644
--- a/include/uapi/drm/qxl_drm.h
+++ b/include/uapi/drm/qxl_drm.h
@@ -80,8 +80,8 @@ struct drm_qxl_reloc {
 };
 
 struct drm_qxl_command {
-	__u64	 __user command; /* void* */
-	__u64	 __user relocs; /* struct drm_qxl_reloc* */
+	__u64		command; /* void* */
+	__u64		relocs; /* struct drm_qxl_reloc* */
 	__u32		type;
 	__u32		command_size;
 	__u32		relocs_num;
@@ -91,7 +91,7 @@ struct drm_qxl_command {
 struct drm_qxl_execbuffer {
 	__u32		flags;		/* for future use */
 	__u32		commands_num;
-	__u64	 __user commands;	/* struct drm_qxl_command* */
+	__u64		commands;	/* struct drm_qxl_command* */
 };
 
 struct drm_qxl_update_area {
diff --git a/drivers/gpu/drm/qxl/qxl_ioctl.c b/drivers/gpu/drm/qxl/qxl_ioctl.c
index 0b82a87916..31effed4a3 100644
--- a/drivers/gpu/drm/qxl/qxl_ioctl.c
+++ b/drivers/gpu/drm/qxl/qxl_ioctl.c
@@ -163,7 +163,7 @@ static int qxl_process_single_command(struct qxl_device *qdev,
 		return -EINVAL;
 
 	if (!access_ok(VERIFY_READ,
-		       (void *)(unsigned long)cmd->command,
+		       u64_to_user_ptr(cmd->command),
 		       cmd->command_size))
 		return -EFAULT;
 
@@ -183,7 +183,9 @@ static int qxl_process_single_command(struct qxl_device *qdev,
 
 	/* TODO copy slow path code from i915 */
 	fb_cmd = qxl_bo_kmap_atomic_page(qdev, cmd_bo, (release->release_offset & PAGE_SIZE));
-	unwritten = __copy_from_user_inatomic_nocache(fb_cmd + sizeof(union qxl_release_info) + (release->release_offset & ~PAGE_SIZE), (void *)(unsigned long)cmd->command, cmd->command_size);
+	unwritten = __copy_from_user_inatomic_nocache
+		(fb_cmd + sizeof(union qxl_release_info) + (release->release_offset & ~PAGE_SIZE),
+		 u64_to_user_ptr(cmd->command), cmd->command_size);
 
 	{
 		struct qxl_drawable *draw = fb_cmd;
@@ -201,10 +203,9 @@ static int qxl_process_single_command(struct qxl_device *qdev,
 	num_relocs = 0;
 	for (i = 0; i < cmd->relocs_num; ++i) {
 		struct drm_qxl_reloc reloc;
+		struct drm_qxl_reloc __user *u = u64_to_user_ptr(cmd->relocs);
 
-		if (copy_from_user(&reloc,
-				       &((struct drm_qxl_reloc *)(uintptr_t)cmd->relocs)[i],
-				       sizeof(reloc))) {
+		if (copy_from_user(&reloc, u + i, sizeof(reloc))) {
 			ret = -EFAULT;
 			goto out_free_bos;
 		}
@@ -282,10 +283,10 @@ static int qxl_execbuffer_ioctl(struct drm_device *dev, void *data,
 
 	for (cmd_num = 0; cmd_num < execbuffer->commands_num; ++cmd_num) {
 
-		struct drm_qxl_command *commands =
-			(struct drm_qxl_command *)(uintptr_t)execbuffer->commands;
+		struct drm_qxl_command __user *commands =
+			u64_to_user_ptr(execbuffer->commands);
 
-		if (copy_from_user(&user_cmd, &commands[cmd_num],
+		if (copy_from_user(&user_cmd, commands + cmd_num,
 				       sizeof(user_cmd)))
 			return -EFAULT;
 

Re: __user with scalar data types

[Daniel Vetter]

On Tue, Jun 20, 2017 at 9:42 AM, Gerd Hoffmann <kraxel@redhat.com> wrote:
>> Yep that's cargo-culted, but from a quick grep only msm and qxl
>> headers do this (the other __user annotations in uapi/drm are for
>> pointers, where it's correct). Adding those maintainers.
>
> Yep, those looks pointless indeed.
>
>> Also, if you use u64_to_user_ptr helper macro sparse should have
>> caught this (if not we'd need to improve the macro).
>
> And qxl should actually use it.
>
> Fix attached (compile-tested only so far), does that look ok?

Yup. Assuming sparse is happy: Acked-by: me.

Cheers, Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch