Messages in this thread | | | Subject | Re: [PATCH 3/3] riscv: fix __user annotation for __copy_user() | From | Atish Patra <> | Date | Thu, 7 Jun 2018 09:45:41 -0700 |
| |
On 6/7/18 9:30 AM, Palmer Dabbelt wrote: > On Mon, 04 Jun 2018 12:28:47 PDT (-0700), atish.patra@wdc.com wrote: >> On 6/4/18 12:09 PM, Luc Van Oostenryck wrote: >>> On Mon, Jun 04, 2018 at 11:46:50AM -0700, Atish Patra wrote: >>>> On 6/1/18 8:22 AM, Luc Van Oostenryck wrote: >>>>> __copy_user() is a function, written in assembly, used to copy >>>>> memory between kernel & user space. As such its to & from args >>>>> may both take a user pointer or a kernel pointer. >>>>> >>>>> However the prototype for this function declare these two args >>>>> as 'void __user *', which is no more & no less correct than >>>>> declaring them as 'void *'. In fact theer is no possible correct >>>> >>>> /s/theer/there >>>> >>>>> annotation for such a function. >>>>> >>>>> The problem is worked around here by declaring these args as >>>>> unsigned long and casting them to the right type in each of >>>>> two callers raw_copy_{to,from}_user() as some kind of cast would >>>>> be needed anyway. >>>>> >>>>> Note: another solution, maybe cleaner but slightly more complex, >>>>> would be to declare two version of __copy_user, >>>>> either in the asm file or via an alias, each having already >>>>> the correct typing for raw_copy_{to,from}_user(). >>>>> >>>> >>>> I feel that would be a better solution as it is implemented similarly >>>> in ARM as well. >>>> >>>> I am unable to understand how "unsigned long" is better than "void*". >>>> x86 implementation has both arguments as void*. Can you please clarify ? >>> >>> "better" is quite relative and it must be understood that sparse >>> allow to cast pointers o fany kinds to and from unsigned long >>> without any warnings (while doing a cast between different address >>> space will emit a warning unless you use '__force'). >>> >> >> Got it. >>> As I tried to explain here above, the fact that this function is >>> declared as taking 2 __user pointers requires to use of casts >>> (ugly casts with __force) to get over the __user. By declaring >>> them as taking unsigned long, you still have to use casts but, IMO, >>> it's cleaner >>> >> >> Thanks for the detailed explanation. >> >>> Note: they're generic pointers/addresses anyway, they can't be >>> dereferenced anyway so unsigned is as good as a plain void* >>> or a void __user* >>> Note: using unsigned long here, fundamentally to bypass the __user, >>> is the same as casting a const pointer back to a plain pointer >>> via an intermediate cast to unsigned long. People can argue >>> that's kinda cheating, and they would be right of course, but >>> using __force or declaring twice the function with two different >>> names and prototype is also a form of cheating. >>> Note: if this would be my code, I would choose the solution with >>> two declarations. >> >> I prefer that as well. > > I haven't compiled this, but I think it should work. It's on the fix-sparse > branch of kernel.org/palmer/linux.git . > > commit 88ffc46f92c16db0fa37edb79038d37ced0a8b41 > gpg: Signature made Thu 07 Jun 2018 09:29:02 AM PDT > gpg: using RSA key 00CE76D1834960DFCE886DF8EF4CA1502CCBAB41 > gpg: issuer "palmer@dabbelt.com" > gpg: Good signature from "Palmer Dabbelt <palmer@dabbelt.com>" [ultimate] > gpg: aka "Palmer Dabbelt <palmer@sifive.com>" [ultimate] > Author: Palmer Dabbelt <palmer@sifive.com> > Date: Thu Jun 7 09:20:57 2018 -0700 > > RISC-V: Split the definition of __copy_user > > We use a single __copy_user assembly function to copy memory both from > and to userspace. While this works, it triggers sparse errors because > we're implicitly casting between the kernel and user address spaces by > calling __copy_user. > > This patch splits the C declaration into a pair of functions, > __asm_copy_{to,from}_user, that have sane semantics WRT __user. This > split tricks sparse into treating these function calls as safe. The > assembly implementation then just aliases these two symbols to > __copy_user, which I renamed to __asm_copy_tofrom_user. The result is > an spare-safe implementation that pays to performance or code size > penalty. > > Signed-off-by: Palmer Dabbelt <palmer@sifive.com> > > diff --git a/arch/riscv/include/asm/uaccess.h b/arch/riscv/include/asm/uaccess.h > index 14b0b22fb578..0dbbbab05dac 100644 > --- a/arch/riscv/include/asm/uaccess.h > +++ b/arch/riscv/include/asm/uaccess.h > @@ -392,19 +392,21 @@ do { \ > }) > > > -extern unsigned long __must_check __copy_user(void __user *to, > +extern unsigned long __must_check __asm_copy_from_user_user(void *to, > const void __user *from, unsigned long n);
Minor typos:
/s/__asm_copy_from_user_user/__asm_copy_from_user/
> +extern unsigned long __must_check __asm_copy_to_user_user(void __user *to, > + const void *from, unsigned long n);
/s/__asm_copy_to_user_user/__asm_copy_to_user/
> > static inline unsigned long > raw_copy_from_user(void *to, const void __user *from, unsigned long n) > { > - return __copy_user(to, from, n); > + return __asm_copy_from_user(to, from, n); > } > > static inline unsigned long > raw_copy_to_user(void __user *to, const void *from, unsigned long n) > { > - return __copy_user(to, from, n); > + return __asm_copy_to_user(to, from, n); > } > > extern long strncpy_from_user(char *dest, const char __user *src, long count); > diff --git a/arch/riscv/lib/uaccess.S b/arch/riscv/lib/uaccess.S > index 58fb2877c865..bd51e47ebd44 100644 > --- a/arch/riscv/lib/uaccess.S > +++ b/arch/riscv/lib/uaccess.S > @@ -13,7 +13,15 @@ _epc: > .previous > .endm > > -ENTRY(__copy_user) > +/* __asm_copy_to_user and __asm_copy_from_user are actually the same function, > + * they're just provided as two different symbols to C code so sparse doesn't > + * yell about casting between two different address spaces. */ > +.global __asm_copy_to_user > +.set __asm_copy_to_user,__asm_copy_tofrom_user > +.global __asm_copy_from_user > +.set __asm_copy_from_user,__asm_copy_tofrom_user > + > +ENTRY(__asm_copy_tofrom_user) > > /* Enable access to user memory */ > li t6, SR_SUM > -ENDPROC(__copy_user) +ENDPROC(__asm_copy_tofrom_user)
I have done boot testing with above typo fixes.
Regards, Atish
| |