[PATCH] [V2] x86/asm: add __user on copy_user_handle_tail() pointers

* [PATCH] [V2] x86/asm: add __user on copy_user_handle_tail() pointers
@ 2019-03-30 11:56 Ben Dooks
  2019-03-30 19:35 ` Mukesh Ojha
  2019-04-02  8:43 ` [tip:x86/asm] x86/asm: Annotate copy_user_handle_tail() pointers with __user tip-bot for Ben Dooks
  0 siblings, 2 replies; 3+ messages in thread
From: Ben Dooks @ 2019-03-30 11:56 UTC (permalink / raw)
  To: linux-kernel; +Cc: x86, mingo, bp, hpa, torvalds, tglx, Ben Dooks

The copy_user_handle_tail() clearly uses both from and to as pointers
to user-space memory. This triggers sparse warning on using the calls
to get and put to user-space. This can be fixed easily by changing the
call to take __user annotated pointer.s

arch/x86/lib/usercopy_64.c:68:21: warning: incorrect type in argument 1 (different address spaces)
arch/x86/lib/usercopy_64.c:68:21:    expected void const volatile [noderef] <asn:1>*<noident>
arch/x86/lib/usercopy_64.c:68:21:    got char *
arch/x86/lib/usercopy_64.c:70:21: warning: incorrect type in argument 1 (different address spaces)
arch/x86/lib/usercopy_64.c:70:21:    expected void const volatile [noderef] <asn:1>*<noident>
arch/x86/lib/usercopy_64.c:70:21:    got char *to

From Linus Torvalds:

On Thu, Mar 28, 2019 at 12:24 AM Borislav Petkov <bp@alien8.de> wrote:

Well, but copy_user_generic() (which ends up calling the
copy_user_handle_tail() eventually) casts those __user pointers to
(__force void *). Converting them back to __user looks strange to me.

Linus?

Well, it does that because the x86 version of copy_user_generic() can
work in either direction, so it works when either the source or
destination (or both) are user pointers, but they don't _have_ to be.

So the "userness" of a pointer in that context is a bit ambiguous, and
so we've picked the pointers to be just plain "void *".

That said, arguably we should have gone the other way and just made
them both "__user" pointers, and do the cast the other way around.

But there's no absolutely right answer here, and nobody should ever
use copy_user_generic() directly (ie it is very much meant to be only
used as a internal helper for the cases that get the pointer
annotations right).

Signed-off-by: Ben Dooks <ben.dooks@codethink.co.uk>
---
 arch/x86/include/asm/uaccess_64.h | 2 +-
 arch/x86/lib/usercopy_64.c        | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/arch/x86/include/asm/uaccess_64.h b/arch/x86/include/asm/uaccess_64.h
index a9d637bc301d..cbca2cb28939 100644
--- a/arch/x86/include/asm/uaccess_64.h
+++ b/arch/x86/include/asm/uaccess_64.h
@@ -208,7 +208,7 @@ __copy_from_user_flushcache(void *dst, const void __user *src, unsigned size)
 }
 
 unsigned long
-copy_user_handle_tail(char *to, char *from, unsigned len);
+copy_user_handle_tail(char __user *to, char __user *from, unsigned len);
 
 unsigned long
 mcsafe_handle_tail(char *to, char *from, unsigned len);
diff --git a/arch/x86/lib/usercopy_64.c b/arch/x86/lib/usercopy_64.c
index ee42bb0cbeb3..aa180424e77a 100644
--- a/arch/x86/lib/usercopy_64.c
+++ b/arch/x86/lib/usercopy_64.c
@@ -60,7 +60,7 @@ EXPORT_SYMBOL(clear_user);
  * it is not necessary to optimize tail handling.
  */
 __visible unsigned long
-copy_user_handle_tail(char *to, char *from, unsigned len)
+copy_user_handle_tail(char __user *to, char __user *from, unsigned len)
 {
 	for (; len; --len, to++) {
 		char c;
-- 
2.20.1


^ permalink raw reply related	[flat|nested] 3+ messages in thread