[PATCH 1/2] powerpc/uaccess: Simplify unsafe_put_user() implementation

[PATCH 1/2] powerpc/uaccess: Simplify unsafe_put_user() implementation

[Michael Ellerman]

Currently unsafe_put_user() expands to __put_user_goto(), which
expands to __put_user_nocheck_goto().

There are no other uses of __put_user_nocheck_goto(), and although
there are some other uses of __put_user_goto() those could just use
unsafe_put_user().

Every layer of indirection introduces the possibility that some code
is calling that layer, and makes keeping track of the required
semantics at each point more complicated.

So drop __put_user_goto(), and rename __put_user_nocheck_goto() to
__unsafe_put_user_goto(). The "nocheck" is implied by "unsafe".

Replace the few uses of __put_user_goto() with unsafe_put_user().

Signed-off-by: Michael Ellerman <mpe@ellerman.id.au>
---
 arch/powerpc/include/asm/uaccess.h | 15 +++++++--------
 1 file changed, 7 insertions(+), 8 deletions(-)

diff --git a/arch/powerpc/include/asm/uaccess.h b/arch/powerpc/include/asm/uaccess.h
index efa4be2fa951..70347ee34c94 100644
--- a/arch/powerpc/include/asm/uaccess.h
+++ b/arch/powerpc/include/asm/uaccess.h
@@ -52,8 +52,6 @@ static inline bool __access_ok(unsigned long addr, unsigned long size)
 	__get_user_nocheck((x), (ptr), sizeof(*(ptr)), true)
 #define __put_user(x, ptr) \
 	__put_user_nocheck((__typeof__(*(ptr)))(x), (ptr), sizeof(*(ptr)))
-#define __put_user_goto(x, ptr, label) \
-	__put_user_nocheck_goto((__typeof__(*(ptr)))(x), (ptr), sizeof(*(ptr)), label)
 
 #define __get_user_allowed(x, ptr) \
 	__get_user_nocheck((x), (ptr), sizeof(*(ptr)), false)
@@ -213,7 +211,7 @@ do {								\
 	}							\
 } while (0)
 
-#define __put_user_nocheck_goto(x, ptr, size, label)		\
+#define __unsafe_put_user_goto(x, ptr, size, label)		\
 do {								\
 	__typeof__(*(ptr)) __user *__pu_addr = (ptr);		\
 	if (!is_kernel_addr((unsigned long)__pu_addr))		\
@@ -530,7 +528,8 @@ user_write_access_begin(const void __user *ptr, size_t len)
 
 #define unsafe_op_wrap(op, err) do { if (unlikely(op)) goto err; } while (0)
 #define unsafe_get_user(x, p, e) unsafe_op_wrap(__get_user_allowed(x, p), e)
-#define unsafe_put_user(x, p, e) __put_user_goto(x, p, e)
+#define unsafe_put_user(x, p, e) \
+	__unsafe_put_user_goto((__typeof__(*(p)))(x), (p), sizeof(*(p)), e)
 
 #define unsafe_copy_from_user(d, s, l, e) \
 	unsafe_op_wrap(__copy_tofrom_user((__force void __user *)d, s, l), e)
@@ -543,17 +542,17 @@ do {									\
 	int _i;								\
 									\
 	for (_i = 0; _i < (_len & ~(sizeof(long) - 1)); _i += sizeof(long))		\
-		__put_user_goto(*(long*)(_src + _i), (long __user *)(_dst + _i), e);\
+		unsafe_put_user(*(long*)(_src + _i), (long __user *)(_dst + _i), e); \
 	if (IS_ENABLED(CONFIG_PPC64) && (_len & 4)) {			\
-		__put_user_goto(*(u32*)(_src + _i), (u32 __user *)(_dst + _i), e);	\
+		unsafe_put_user(*(u32*)(_src + _i), (u32 __user *)(_dst + _i), e); \
 		_i += 4;						\
 	}								\
 	if (_len & 2) {							\
-		__put_user_goto(*(u16*)(_src + _i), (u16 __user *)(_dst + _i), e);	\
+		unsafe_put_user(*(u16*)(_src + _i), (u16 __user *)(_dst + _i), e); \
 		_i += 2;						\
 	}								\
 	if (_len & 1) \
-		__put_user_goto(*(u8*)(_src + _i), (u8 __user *)(_dst + _i), e);\
+		unsafe_put_user(*(u8*)(_src + _i), (u8 __user *)(_dst + _i), e); \
 } while (0)
 
 #define HAVE_GET_KERNEL_NOFAULT
-- 
2.25.1

[PATCH 2/2] powerpc/uaccess: Move might_fault() into user_access_begin()

[Michael Ellerman]

We have a might_fault() check in __unsafe_put_user_goto(), but that is
dangerous as it potentially calls lots of code while user access is
enabled.

It also triggers the check Alexey added to the irq restore path to
catch cases like that:

  WARNING: CPU: 30 PID: 1 at arch/powerpc/include/asm/book3s/64/kup.h:324 arch_local_irq_restore+0x160/0x190
  NIP arch_local_irq_restore+0x160/0x190
  LR  lock_is_held_type+0x140/0x200
  Call Trace:
    0xc00000007f392ff8 (unreliable)
    ___might_sleep+0x180/0x320
    __might_fault+0x50/0xe0
    filldir64+0x2d0/0x5d0
    call_filldir+0xc8/0x180
    ext4_readdir+0x948/0xb40
    iterate_dir+0x1ec/0x240
    sys_getdents64+0x80/0x290
    system_call_exception+0x160/0x280
    system_call_common+0xf0/0x27c

So remove the might fault check from unsafe_put_user().

Any call to unsafe_put_user() must be inside a region that's had user
access enabled with user_access_begin(), so move the might_fault() in
there. That also allows us to drop the is_kernel_addr() test, because
there should be no code using user_access_begin() in order to access a
kernel address.

Signed-off-by: Michael Ellerman <mpe@ellerman.id.au>
---
 arch/powerpc/include/asm/uaccess.h | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/arch/powerpc/include/asm/uaccess.h b/arch/powerpc/include/asm/uaccess.h
index 70347ee34c94..71640eca7341 100644
--- a/arch/powerpc/include/asm/uaccess.h
+++ b/arch/powerpc/include/asm/uaccess.h
@@ -214,8 +214,6 @@ do {								\
 #define __unsafe_put_user_goto(x, ptr, size, label)		\
 do {								\
 	__typeof__(*(ptr)) __user *__pu_addr = (ptr);		\
-	if (!is_kernel_addr((unsigned long)__pu_addr))		\
-		might_fault();					\
 	__chk_user_ptr(ptr);					\
 	__put_user_size_goto((x), __pu_addr, (size), label);	\
 } while (0)
@@ -494,6 +492,8 @@ extern void memcpy_page_flushcache(char *to, struct page *page, size_t offset,
 
 static __must_check inline bool user_access_begin(const void __user *ptr, size_t len)
 {
+	might_fault();
+
 	if (unlikely(!access_ok(ptr, len)))
 		return false;
 	allow_read_write_user((void __user *)ptr, ptr, len);
-- 
2.25.1

Re: [PATCH 2/2] powerpc/uaccess: Move might_fault() into user_access_begin()

[Christophe Leroy]

Le 08/02/2021 à 14:57, Michael Ellerman a écrit :
> We have a might_fault() check in __unsafe_put_user_goto(), but that is
> dangerous as it potentially calls lots of code while user access is
> enabled.
> 
> It also triggers the check Alexey added to the irq restore path to
> catch cases like that:
> 
>    WARNING: CPU: 30 PID: 1 at arch/powerpc/include/asm/book3s/64/kup.h:324 arch_local_irq_restore+0x160/0x190
>    NIP arch_local_irq_restore+0x160/0x190
>    LR  lock_is_held_type+0x140/0x200
>    Call Trace:
>      0xc00000007f392ff8 (unreliable)
>      ___might_sleep+0x180/0x320
>      __might_fault+0x50/0xe0
>      filldir64+0x2d0/0x5d0
>      call_filldir+0xc8/0x180
>      ext4_readdir+0x948/0xb40
>      iterate_dir+0x1ec/0x240
>      sys_getdents64+0x80/0x290
>      system_call_exception+0x160/0x280
>      system_call_common+0xf0/0x27c
> 
> So remove the might fault check from unsafe_put_user().
> 
> Any call to unsafe_put_user() must be inside a region that's had user
> access enabled with user_access_begin(), so move the might_fault() in
> there. That also allows us to drop the is_kernel_addr() test, because
> there should be no code using user_access_begin() in order to access a
> kernel address.

x86 and mips only have might_fault() on get_user() and put_user(), neither on __get_user() nor on 
__put_user() nor on the unsafe alternative.

When have might_fault() in __get_user_nocheck() that is used by __get_user() and 
__get_user_allowed() ie by unsafe_get_user().

Shoudln't those be dropped as well ?

Christophe

> 
> Signed-off-by: Michael Ellerman <mpe@ellerman.id.au>
> ---
>   arch/powerpc/include/asm/uaccess.h | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/powerpc/include/asm/uaccess.h b/arch/powerpc/include/asm/uaccess.h
> index 70347ee34c94..71640eca7341 100644
> --- a/arch/powerpc/include/asm/uaccess.h
> +++ b/arch/powerpc/include/asm/uaccess.h
> @@ -214,8 +214,6 @@ do {								\
>   #define __unsafe_put_user_goto(x, ptr, size, label)		\
>   do {								\
>   	__typeof__(*(ptr)) __user *__pu_addr = (ptr);		\
> -	if (!is_kernel_addr((unsigned long)__pu_addr))		\
> -		might_fault();					\
>   	__chk_user_ptr(ptr);					\
>   	__put_user_size_goto((x), __pu_addr, (size), label);	\
>   } while (0)
> @@ -494,6 +492,8 @@ extern void memcpy_page_flushcache(char *to, struct page *page, size_t offset,
>   
>   static __must_check inline bool user_access_begin(const void __user *ptr, size_t len)
>   {
> +	might_fault();
> +
>   	if (unlikely(!access_ok(ptr, len)))
>   		return false;
>   	allow_read_write_user((void __user *)ptr, ptr, len);
> 

Re: [PATCH 2/2] powerpc/uaccess: Move might_fault() into user_access_begin()

[Michael Ellerman]

Christophe Leroy <christophe.leroy@csgroup.eu> writes:
> Le 08/02/2021 à 14:57, Michael Ellerman a écrit :
>> We have a might_fault() check in __unsafe_put_user_goto(), but that is
>> dangerous as it potentially calls lots of code while user access is
>> enabled.
>> 
>> It also triggers the check Alexey added to the irq restore path to
>> catch cases like that:
>> 
>>    WARNING: CPU: 30 PID: 1 at arch/powerpc/include/asm/book3s/64/kup.h:324 arch_local_irq_restore+0x160/0x190
>>    NIP arch_local_irq_restore+0x160/0x190
>>    LR  lock_is_held_type+0x140/0x200
>>    Call Trace:
>>      0xc00000007f392ff8 (unreliable)
>>      ___might_sleep+0x180/0x320
>>      __might_fault+0x50/0xe0
>>      filldir64+0x2d0/0x5d0
>>      call_filldir+0xc8/0x180
>>      ext4_readdir+0x948/0xb40
>>      iterate_dir+0x1ec/0x240
>>      sys_getdents64+0x80/0x290
>>      system_call_exception+0x160/0x280
>>      system_call_common+0xf0/0x27c
>> 
>> So remove the might fault check from unsafe_put_user().
>> 
>> Any call to unsafe_put_user() must be inside a region that's had user
>> access enabled with user_access_begin(), so move the might_fault() in
>> there. That also allows us to drop the is_kernel_addr() test, because
>> there should be no code using user_access_begin() in order to access a
>> kernel address.
>
> x86 and mips only have might_fault() on get_user() and put_user(),
> neither on __get_user() nor on __put_user() nor on the unsafe
> alternative.

Yeah, that's their choice, or perhaps it's by accident.

arm64 on the other hand has might_fault() in all variants.

A __get_user() can fault just as much as a get_user(), so there's no
reason the check should be omitted from __get_user(), other than perhaps
some historical argument about __get_user() being the "fast" case.

> When have might_fault() in __get_user_nocheck() that is used by
> __get_user() and __get_user_allowed() ie by unsafe_get_user().
>
> Shoudln't those be dropped as well ?

That was handled by Alexey's patch, which I ended up merging with this
one:

  https://git.kernel.org/torvalds/c/7d506ca97b665b95e698a53697dad99fae813c1a


ie. we still have might_fault() in __get_user_nocheck(), but it's
guarded by a check of do_allow, so we won't call it for
__get_user_allowed().

So I think the code (in my next branch) is correct, we don't have any
might_fault() calls in unsafe regions.

But I'd still be happier if we could simplify our uaccess.h more, it's a
bit of a rats nest. We could start by making __get/put_user() ==
get/put_user() the same way arm64 did.

cheers

Re: [PATCH 2/2] powerpc/uaccess: Move might_fault() into user_access_begin()

[Christophe Leroy]

Michael Ellerman <mpe@ellerman.id.au> a écrit :

> Christophe Leroy <christophe.leroy@csgroup.eu> writes:
>> Le 08/02/2021 à 14:57, Michael Ellerman a écrit :
>>> We have a might_fault() check in __unsafe_put_user_goto(), but that is
>>> dangerous as it potentially calls lots of code while user access is
>>> enabled.
>>>
>>> It also triggers the check Alexey added to the irq restore path to
>>> catch cases like that:
>>>
>>>    WARNING: CPU: 30 PID: 1 at  
>>> arch/powerpc/include/asm/book3s/64/kup.h:324  
>>> arch_local_irq_restore+0x160/0x190
>>>    NIP arch_local_irq_restore+0x160/0x190
>>>    LR  lock_is_held_type+0x140/0x200
>>>    Call Trace:
>>>      0xc00000007f392ff8 (unreliable)
>>>      ___might_sleep+0x180/0x320
>>>      __might_fault+0x50/0xe0
>>>      filldir64+0x2d0/0x5d0
>>>      call_filldir+0xc8/0x180
>>>      ext4_readdir+0x948/0xb40
>>>      iterate_dir+0x1ec/0x240
>>>      sys_getdents64+0x80/0x290
>>>      system_call_exception+0x160/0x280
>>>      system_call_common+0xf0/0x27c
>>>
>>> So remove the might fault check from unsafe_put_user().
>>>
>>> Any call to unsafe_put_user() must be inside a region that's had user
>>> access enabled with user_access_begin(), so move the might_fault() in
>>> there. That also allows us to drop the is_kernel_addr() test, because
>>> there should be no code using user_access_begin() in order to access a
>>> kernel address.
>>
>> x86 and mips only have might_fault() on get_user() and put_user(),
>> neither on __get_user() nor on __put_user() nor on the unsafe
>> alternative.
>
> Yeah, that's their choice, or perhaps it's by accident.
>
> arm64 on the other hand has might_fault() in all variants.
>
> A __get_user() can fault just as much as a get_user(), so there's no
> reason the check should be omitted from __get_user(), other than perhaps
> some historical argument about __get_user() being the "fast" case.
>
>> When have might_fault() in __get_user_nocheck() that is used by
>> __get_user() and __get_user_allowed() ie by unsafe_get_user().
>>
>> Shoudln't those be dropped as well ?
>
> That was handled by Alexey's patch, which I ended up merging with this
> one:
>
>   https://git.kernel.org/torvalds/c/7d506ca97b665b95e698a53697dad99fae813c1a
>
>
> ie. we still have might_fault() in __get_user_nocheck(), but it's
> guarded by a check of do_allow, so we won't call it for
> __get_user_allowed().
>
> So I think the code (in my next branch) is correct, we don't have any
> might_fault() calls in unsafe regions.
>
> But I'd still be happier if we could simplify our uaccess.h more, it's a
> bit of a rats nest. We could start by making __get/put_user() ==
> get/put_user() the same way arm64 did.

I agree there are several easy simplifications to do there. I'll look  
at that in the coming weeks.

I'm not sure it is good to make __get/put_user equal get/put_user as  
it would mean calling access_ok() everytime. But we could most likely  
make something simpler with get_user() calling access_ok() then  
__get_user().

I think we should also audit our use of the _inatomic variants.  
might_fault() voids when pagefault is disabled so I think the inatomic  
variants should be needed. As far as I can see, powerpc is the only  
arch having that.

Need to also check why we still need that is_kernel_addr() check  
because since the removal of set_fs(), __get/put_user() helpers  
shouldn't be used anymore for kernel addresses

Christophe