[PATCH 1/4] powerpc/64s: implement probe_kernel_read/write without touching AMR

[PATCH 1/4] powerpc/64s: implement probe_kernel_read/write without touching AMR

[Nicholas Piggin]

There is no need to allow user accesses when probing kernel addresses.

Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
---
 arch/powerpc/include/asm/uaccess.h | 25 ++++++++++-----
 arch/powerpc/lib/Makefile          |  2 +-
 arch/powerpc/lib/uaccess.c         | 50 ++++++++++++++++++++++++++++++
 3 files changed, 68 insertions(+), 9 deletions(-)
 create mode 100644 arch/powerpc/lib/uaccess.c

diff --git a/arch/powerpc/include/asm/uaccess.h b/arch/powerpc/include/asm/uaccess.h
index 2f500debae21..670910df3cc7 100644
--- a/arch/powerpc/include/asm/uaccess.h
+++ b/arch/powerpc/include/asm/uaccess.h
@@ -341,8 +341,8 @@ raw_copy_in_user(void __user *to, const void __user *from, unsigned long n)
 }
 #endif /* __powerpc64__ */
 
-static inline unsigned long raw_copy_from_user(void *to,
-		const void __user *from, unsigned long n)
+static inline unsigned long
+raw_copy_from_user_allowed(void *to, const void __user *from, unsigned long n)
 {
 	unsigned long ret;
 	if (__builtin_constant_p(n) && (n <= 8)) {
@@ -351,19 +351,19 @@ static inline unsigned long raw_copy_from_user(void *to,
 		switch (n) {
 		case 1:
 			barrier_nospec();
-			__get_user_size(*(u8 *)to, from, 1, ret);
+			__get_user_size_allowed(*(u8 *)to, from, 1, ret);
 			break;
 		case 2:
 			barrier_nospec();
-			__get_user_size(*(u16 *)to, from, 2, ret);
+			__get_user_size_allowed(*(u16 *)to, from, 2, ret);
 			break;
 		case 4:
 			barrier_nospec();
-			__get_user_size(*(u32 *)to, from, 4, ret);
+			__get_user_size_allowed(*(u32 *)to, from, 4, ret);
 			break;
 		case 8:
 			barrier_nospec();
-			__get_user_size(*(u64 *)to, from, 8, ret);
+			__get_user_size_allowed(*(u64 *)to, from, 8, ret);
 			break;
 		}
 		if (ret == 0)
@@ -371,9 +371,18 @@ static inline unsigned long raw_copy_from_user(void *to,
 	}
 
 	barrier_nospec();
-	allow_read_from_user(from, n);
 	ret = __copy_tofrom_user((__force void __user *)to, from, n);
-	prevent_read_from_user(from, n);
+	return ret;
+}
+
+static inline unsigned long
+raw_copy_from_user(void *to, const void __user *from, unsigned long n)
+{
+	unsigned long ret;
+
+	allow_read_from_user(to, n);
+	ret = raw_copy_from_user_allowed(to, from, n);
+	prevent_read_from_user(to, n);
 	return ret;
 }
 
diff --git a/arch/powerpc/lib/Makefile b/arch/powerpc/lib/Makefile
index b8de3be10eb4..a15060b5008e 100644
--- a/arch/powerpc/lib/Makefile
+++ b/arch/powerpc/lib/Makefile
@@ -36,7 +36,7 @@ extra-$(CONFIG_PPC64)	+= crtsavres.o
 endif
 
 obj-$(CONFIG_PPC_BOOK3S_64) += copyuser_power7.o copypage_power7.o \
-			       memcpy_power7.o
+			       memcpy_power7.o uaccess.o
 
 obj64-y	+= copypage_64.o copyuser_64.o mem_64.o hweight_64.o \
 	   memcpy_64.o memcpy_mcsafe_64.o
diff --git a/arch/powerpc/lib/uaccess.c b/arch/powerpc/lib/uaccess.c
new file mode 100644
index 000000000000..0057ab52d6fe
--- /dev/null
+++ b/arch/powerpc/lib/uaccess.c
@@ -0,0 +1,50 @@
+#include <linux/mm.h>
+#include <linux/uaccess.h>
+
+static __always_inline long
+probe_read_common(void *dst, const void __user *src, size_t size)
+{
+	long ret;
+
+	pagefault_disable();
+	ret = raw_copy_from_user_allowed(dst, src, size);
+	pagefault_enable();
+
+	return ret ? -EFAULT : 0;
+}
+
+static __always_inline long
+probe_write_common(void __user *dst, const void *src, size_t size)
+{
+	long ret;
+
+	pagefault_disable();
+	ret = raw_copy_to_user_allowed(dst, src, size);
+	pagefault_enable();
+
+	return ret ? -EFAULT : 0;
+}
+
+long probe_kernel_read(void *dst, const void *src, size_t size)
+{
+	long ret;
+	mm_segment_t old_fs = get_fs();
+
+	set_fs(KERNEL_DS);
+	ret = probe_read_common(dst, (__force const void __user *)src, size);
+	set_fs(old_fs);
+
+	return ret;
+}
+
+long probe_kernel_write(void *dst, const void *src, size_t size)
+{
+	long ret;
+	mm_segment_t old_fs = get_fs();
+
+	set_fs(KERNEL_DS);
+	ret = probe_write_common((__force void __user *)dst, src, size);
+	set_fs(old_fs);
+
+	return ret;
+}
-- 
2.23.0

[PATCH 2/4] powerpc/64s: use mmu_has_feature in set_kuap() and get_kuap()

[Nicholas Piggin]

Commit 8150a153c013 ("powerpc/64s: Use early_mmu_has_feature() in
set_kuap()"), had to switch to using the _early feature test, because
probe_kernel_read was being called very early. After the previous
patch, probe_kernel_read no longer touches kuap, so it can go back to
using the non-_early variant, for better performance.

Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
---
 arch/powerpc/include/asm/book3s/64/kup-radix.h | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/arch/powerpc/include/asm/book3s/64/kup-radix.h b/arch/powerpc/include/asm/book3s/64/kup-radix.h
index 3bcef989a35d..67a7fd0182e6 100644
--- a/arch/powerpc/include/asm/book3s/64/kup-radix.h
+++ b/arch/powerpc/include/asm/book3s/64/kup-radix.h
@@ -79,7 +79,7 @@ static inline void kuap_check_amr(void)
 
 static inline unsigned long get_kuap(void)
 {
-	if (!early_mmu_has_feature(MMU_FTR_RADIX_KUAP))
+	if (!mmu_has_feature(MMU_FTR_RADIX_KUAP))
 		return 0;
 
 	return mfspr(SPRN_AMR);
@@ -87,7 +87,7 @@ static inline unsigned long get_kuap(void)
 
 static inline void set_kuap(unsigned long value)
 {
-	if (!early_mmu_has_feature(MMU_FTR_RADIX_KUAP))
+	if (!mmu_has_feature(MMU_FTR_RADIX_KUAP))
 		return;
 
 	/*
-- 
2.23.0

[PATCH 3/4] powerpc/uaccess: evaluate macro arguments once, before user access is allowed

[Nicholas Piggin]

get/put_user can be called with nontrivial arguments. fs/proc/page.c
has a good example:

    if (put_user(stable_page_flags(ppage), out)) {

stable_page_flags is quite a lot of code, including spin locks in the
page allocator.

Ensure these arguments are evaluated before user access is allowed.
This improves security by reducing code with access to userspace, but
it also fixes a PREEMPT bug with KUAP on powerpc/64s:
stable_page_flags is currently called with AMR set to allow writes,
it ends up calling spin_unlock(), which can call preempt_schedule. But
the task switch code can not be called with AMR set (it relies on
interrupts saving the register), so this blows up.

It's fine if the code inside allow_user_access is preemptible, because
a timer or IPI will save the AMR, but it's not okay to explicitly
cause a reschedule.

Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
---
 arch/powerpc/include/asm/uaccess.h | 97 ++++++++++++++++++------------
 1 file changed, 59 insertions(+), 38 deletions(-)

diff --git a/arch/powerpc/include/asm/uaccess.h b/arch/powerpc/include/asm/uaccess.h
index 670910df3cc7..1cf8595aeef1 100644
--- a/arch/powerpc/include/asm/uaccess.h
+++ b/arch/powerpc/include/asm/uaccess.h
@@ -162,36 +162,48 @@ do {								\
 	prevent_write_to_user(ptr, size);			\
 } while (0)
 
-#define __put_user_nocheck(x, ptr, size, do_allow)			\
+#define __put_user_nocheck(x, ptr, size, do_allow)		\
 ({								\
 	long __pu_err;						\
 	__typeof__(*(ptr)) __user *__pu_addr = (ptr);		\
+	__typeof__(*(ptr)) __pu_val = (x);			\
+	__typeof__(size) __pu_size = (size);			\
+								\
 	if (!is_kernel_addr((unsigned long)__pu_addr))		\
 		might_fault();					\
-	__chk_user_ptr(ptr);					\
-	if (do_allow)								\
-		__put_user_size((x), __pu_addr, (size), __pu_err);		\
-	else									\
-		__put_user_size_allowed((x), __pu_addr, (size), __pu_err);	\
+	__chk_user_ptr(__pu_addr);				\
+	if (do_allow)						\
+		__put_user_size(__pu_val, __pu_addr, __pu_size, __pu_err); \
+	else							\
+		__put_user_size_allowed(__pu_val, __pu_addr, __pu_size, __pu_err); \
+								\
 	__pu_err;						\
 })
 
-#define __put_user_check(x, ptr, size)					\
-({									\
-	long __pu_err = -EFAULT;					\
-	__typeof__(*(ptr)) __user *__pu_addr = (ptr);			\
-	might_fault();							\
-	if (access_ok(__pu_addr, size))			\
-		__put_user_size((x), __pu_addr, (size), __pu_err);	\
-	__pu_err;							\
+#define __put_user_check(x, ptr, size)				\
+({								\
+	long __pu_err = -EFAULT;				\
+	__typeof__(*(ptr)) __user *__pu_addr = (ptr);		\
+	__typeof__(*(ptr)) __pu_val = (x);			\
+	__typeof__(size) __pu_size = (size);			\
+								\
+	might_fault();						\
+	if (access_ok(__pu_addr, __pu_size))			\
+		__put_user_size(__pu_val, __pu_addr, __pu_size, __pu_err); \
+								\
+	__pu_err;						\
 })
 
 #define __put_user_nosleep(x, ptr, size)			\
 ({								\
 	long __pu_err;						\
 	__typeof__(*(ptr)) __user *__pu_addr = (ptr);		\
-	__chk_user_ptr(ptr);					\
-	__put_user_size((x), __pu_addr, (size), __pu_err);	\
+	__typeof__(*(ptr)) __pu_val = (x);			\
+	__typeof__(size) __pu_size = (size);			\
+								\
+	__chk_user_ptr(__pu_addr);				\
+	__put_user_size(__pu_val, __pu_addr, __pu_size, __pu_err); \
+								\
 	__pu_err;						\
 })
 
@@ -278,46 +290,55 @@ do {								\
 #define __long_type(x) \
 	__typeof__(__builtin_choose_expr(sizeof(x) > sizeof(0UL), 0ULL, 0UL))
 
-#define __get_user_nocheck(x, ptr, size, do_allow)			\
+#define __get_user_nocheck(x, ptr, size, do_allow)		\
 ({								\
 	long __gu_err;						\
 	__long_type(*(ptr)) __gu_val;				\
-	__typeof__(*(ptr)) __user *__gu_addr = (ptr);	\
-	__chk_user_ptr(ptr);					\
+	__typeof__(*(ptr)) __user *__gu_addr = (ptr);		\
+	__typeof__(size) __gu_size = (size);			\
+								\
+	__chk_user_ptr(__gu_addr);				\
 	if (!is_kernel_addr((unsigned long)__gu_addr))		\
 		might_fault();					\
 	barrier_nospec();					\
-	if (do_allow)								\
-		__get_user_size(__gu_val, __gu_addr, (size), __gu_err);		\
-	else									\
-		__get_user_size_allowed(__gu_val, __gu_addr, (size), __gu_err);	\
+	if (do_allow)						\
+		__get_user_size(__gu_val, __gu_addr, __gu_size, __gu_err); \
+	else							\
+		__get_user_size_allowed(__gu_val, __gu_addr, __gu_size, __gu_err); \
 	(x) = (__typeof__(*(ptr)))__gu_val;			\
+								\
 	__gu_err;						\
 })
 
-#define __get_user_check(x, ptr, size)					\
-({									\
-	long __gu_err = -EFAULT;					\
-	__long_type(*(ptr)) __gu_val = 0;				\
+#define __get_user_check(x, ptr, size)				\
+({								\
+	long __gu_err = -EFAULT;				\
+	__long_type(*(ptr)) __gu_val = 0;			\
 	__typeof__(*(ptr)) __user *__gu_addr = (ptr);		\
-	might_fault();							\
-	if (access_ok(__gu_addr, (size))) {		\
-		barrier_nospec();					\
-		__get_user_size(__gu_val, __gu_addr, (size), __gu_err);	\
-	}								\
-	(x) = (__force __typeof__(*(ptr)))__gu_val;				\
-	__gu_err;							\
+	__typeof__(size) __gu_size = (size);			\
+								\
+	might_fault();						\
+	if (access_ok(__gu_addr, __gu_size)) {			\
+		barrier_nospec();				\
+		__get_user_size(__gu_val, __gu_addr, __gu_size, __gu_err); \
+	}							\
+	(x) = (__force __typeof__(*(ptr)))__gu_val;		\
+								\
+	__gu_err;						\
 })
 
 #define __get_user_nosleep(x, ptr, size)			\
 ({								\
 	long __gu_err;						\
 	__long_type(*(ptr)) __gu_val;				\
-	__typeof__(*(ptr)) __user *__gu_addr = (ptr);	\
-	__chk_user_ptr(ptr);					\
+	__typeof__(*(ptr)) __user *__gu_addr = (ptr);		\
+	__typeof__(size) __gu_size = (size);			\
+								\
+	__chk_user_ptr(__gu_addr);				\
 	barrier_nospec();					\
-	__get_user_size(__gu_val, __gu_addr, (size), __gu_err);	\
-	(x) = (__force __typeof__(*(ptr)))__gu_val;			\
+	__get_user_size(__gu_val, __gu_addr, __gu_size, __gu_err); \
+	(x) = (__force __typeof__(*(ptr)))__gu_val;		\
+								\
 	__gu_err;						\
 })
 
-- 
2.23.0

Re: [PATCH 3/4] powerpc/uaccess: evaluate macro arguments once, before user access is allowed

[Christophe Leroy]

Le 27/03/2020 à 08:02, Nicholas Piggin a écrit :
> get/put_user can be called with nontrivial arguments. fs/proc/page.c
> has a good example:
> 
>      if (put_user(stable_page_flags(ppage), out)) {
> 
> stable_page_flags is quite a lot of code, including spin locks in the
> page allocator.
> 
> Ensure these arguments are evaluated before user access is allowed.
> This improves security by reducing code with access to userspace, but
> it also fixes a PREEMPT bug with KUAP on powerpc/64s:
> stable_page_flags is currently called with AMR set to allow writes,
> it ends up calling spin_unlock(), which can call preempt_schedule. But
> the task switch code can not be called with AMR set (it relies on
> interrupts saving the register), so this blows up.
> 
> It's fine if the code inside allow_user_access is preemptible, because
> a timer or IPI will save the AMR, but it's not okay to explicitly
> cause a reschedule.
> 
> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
> ---
>   arch/powerpc/include/asm/uaccess.h | 97 ++++++++++++++++++------------
>   1 file changed, 59 insertions(+), 38 deletions(-)
> 
> diff --git a/arch/powerpc/include/asm/uaccess.h b/arch/powerpc/include/asm/uaccess.h
> index 670910df3cc7..1cf8595aeef1 100644
> --- a/arch/powerpc/include/asm/uaccess.h
> +++ b/arch/powerpc/include/asm/uaccess.h
> @@ -162,36 +162,48 @@ do {								\
>   	prevent_write_to_user(ptr, size);			\
>   } while (0)
>   
> -#define __put_user_nocheck(x, ptr, size, do_allow)			\
> +#define __put_user_nocheck(x, ptr, size, do_allow)		\

No need to touch this line. Anyway at the end, you still have several \ 
which are not aligned.

>   ({								\
>   	long __pu_err;						\
>   	__typeof__(*(ptr)) __user *__pu_addr = (ptr);		\
> +	__typeof__(*(ptr)) __pu_val = (x);			\
> +	__typeof__(size) __pu_size = (size);			\
> +								\
>   	if (!is_kernel_addr((unsigned long)__pu_addr))		\
>   		might_fault();					\
> -	__chk_user_ptr(ptr);					\
> -	if (do_allow)								\

No need to touch that line

> -		__put_user_size((x), __pu_addr, (size), __pu_err);		\
> -	else									\

No need to touch that line

> -		__put_user_size_allowed((x), __pu_addr, (size), __pu_err);	\
> +	__chk_user_ptr(__pu_addr);				\
> +	if (do_allow)						\
> +		__put_user_size(__pu_val, __pu_addr, __pu_size, __pu_err); \
> +	else							\
> +		__put_user_size_allowed(__pu_val, __pu_addr, __pu_size, __pu_err); \
> +								\
>   	__pu_err;						\
>   })
>   
> -#define __put_user_check(x, ptr, size)					\
> -({									\
> -	long __pu_err = -EFAULT;					\
> -	__typeof__(*(ptr)) __user *__pu_addr = (ptr);			\
> -	might_fault();							\
> -	if (access_ok(__pu_addr, size))			\
> -		__put_user_size((x), __pu_addr, (size), __pu_err);	\
> -	__pu_err;							\

Same comment applies, you are touching some lines just to change the \, 
but at the end you still have some misaligned ones.

It would help the review not to touch unchanged lines just for that.

Same comment applies a few places below as well.

> +#define __put_user_check(x, ptr, size)				\
> +({								\
> +	long __pu_err = -EFAULT;				\
> +	__typeof__(*(ptr)) __user *__pu_addr = (ptr);		\
> +	__typeof__(*(ptr)) __pu_val = (x);			\
> +	__typeof__(size) __pu_size = (size);			\
> +								\
> +	might_fault();						\
> +	if (access_ok(__pu_addr, __pu_size))			\
> +		__put_user_size(__pu_val, __pu_addr, __pu_size, __pu_err); \
> +								\
> +	__pu_err;						\
>   })
>   
>   #define __put_user_nosleep(x, ptr, size)			\
>   ({								\
>   	long __pu_err;						\
>   	__typeof__(*(ptr)) __user *__pu_addr = (ptr);		\
> -	__chk_user_ptr(ptr);					\
> -	__put_user_size((x), __pu_addr, (size), __pu_err);	\
> +	__typeof__(*(ptr)) __pu_val = (x);			\
> +	__typeof__(size) __pu_size = (size);			\
> +								\
> +	__chk_user_ptr(__pu_addr);				\
> +	__put_user_size(__pu_val, __pu_addr, __pu_size, __pu_err); \
> +								\
>   	__pu_err;						\
>   })
>   
> @@ -278,46 +290,55 @@ do {								\
>   #define __long_type(x) \
>   	__typeof__(__builtin_choose_expr(sizeof(x) > sizeof(0UL), 0ULL, 0UL))
>   
> -#define __get_user_nocheck(x, ptr, size, do_allow)			\
> +#define __get_user_nocheck(x, ptr, size, do_allow)		\
>   ({								\
>   	long __gu_err;						\
>   	__long_type(*(ptr)) __gu_val;				\
> -	__typeof__(*(ptr)) __user *__gu_addr = (ptr);	\
> -	__chk_user_ptr(ptr);					\
> +	__typeof__(*(ptr)) __user *__gu_addr = (ptr);		\
> +	__typeof__(size) __gu_size = (size);			\
> +								\
> +	__chk_user_ptr(__gu_addr);				\
>   	if (!is_kernel_addr((unsigned long)__gu_addr))		\
>   		might_fault();					\
>   	barrier_nospec();					\
> -	if (do_allow)								\
> -		__get_user_size(__gu_val, __gu_addr, (size), __gu_err);		\
> -	else									\
> -		__get_user_size_allowed(__gu_val, __gu_addr, (size), __gu_err);	\
> +	if (do_allow)						\
> +		__get_user_size(__gu_val, __gu_addr, __gu_size, __gu_err); \
> +	else							\
> +		__get_user_size_allowed(__gu_val, __gu_addr, __gu_size, __gu_err); \
>   	(x) = (__typeof__(*(ptr)))__gu_val;			\
> +								\
>   	__gu_err;						\
>   })
>   
> -#define __get_user_check(x, ptr, size)					\
> -({									\
> -	long __gu_err = -EFAULT;					\
> -	__long_type(*(ptr)) __gu_val = 0;				\
> +#define __get_user_check(x, ptr, size)				\
> +({								\
> +	long __gu_err = -EFAULT;				\
> +	__long_type(*(ptr)) __gu_val = 0;			\
>   	__typeof__(*(ptr)) __user *__gu_addr = (ptr);		\
> -	might_fault();							\
> -	if (access_ok(__gu_addr, (size))) {		\
> -		barrier_nospec();					\
> -		__get_user_size(__gu_val, __gu_addr, (size), __gu_err);	\
> -	}								\
> -	(x) = (__force __typeof__(*(ptr)))__gu_val;				\
> -	__gu_err;							\
> +	__typeof__(size) __gu_size = (size);			\
> +								\
> +	might_fault();						\
> +	if (access_ok(__gu_addr, __gu_size)) {			\
> +		barrier_nospec();				\
> +		__get_user_size(__gu_val, __gu_addr, __gu_size, __gu_err); \
> +	}							\
> +	(x) = (__force __typeof__(*(ptr)))__gu_val;		\
> +								\
> +	__gu_err;						\
>   })
>   
>   #define __get_user_nosleep(x, ptr, size)			\
>   ({								\
>   	long __gu_err;						\
>   	__long_type(*(ptr)) __gu_val;				\
> -	__typeof__(*(ptr)) __user *__gu_addr = (ptr);	\
> -	__chk_user_ptr(ptr);					\
> +	__typeof__(*(ptr)) __user *__gu_addr = (ptr);		\
> +	__typeof__(size) __gu_size = (size);			\
> +								\
> +	__chk_user_ptr(__gu_addr);				\
>   	barrier_nospec();					\
> -	__get_user_size(__gu_val, __gu_addr, (size), __gu_err);	\
> -	(x) = (__force __typeof__(*(ptr)))__gu_val;			\
> +	__get_user_size(__gu_val, __gu_addr, __gu_size, __gu_err); \
> +	(x) = (__force __typeof__(*(ptr)))__gu_val;		\
> +								\
>   	__gu_err;						\
>   })
>   
> 


Christophe

Re: [PATCH 3/4] powerpc/uaccess: evaluate macro arguments once, before user access is allowed

[Nicholas Piggin]

Christophe Leroy's on March 27, 2020 5:21 pm:
> 
> 
> Le 27/03/2020 à 08:02, Nicholas Piggin a écrit :
>> get/put_user can be called with nontrivial arguments. fs/proc/page.c
>> has a good example:
>> 
>>      if (put_user(stable_page_flags(ppage), out)) {
>> 
>> stable_page_flags is quite a lot of code, including spin locks in the
>> page allocator.
>> 
>> Ensure these arguments are evaluated before user access is allowed.
>> This improves security by reducing code with access to userspace, but
>> it also fixes a PREEMPT bug with KUAP on powerpc/64s:
>> stable_page_flags is currently called with AMR set to allow writes,
>> it ends up calling spin_unlock(), which can call preempt_schedule. But
>> the task switch code can not be called with AMR set (it relies on
>> interrupts saving the register), so this blows up.
>> 
>> It's fine if the code inside allow_user_access is preemptible, because
>> a timer or IPI will save the AMR, but it's not okay to explicitly
>> cause a reschedule.
>> 
>> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
>> ---
>>   arch/powerpc/include/asm/uaccess.h | 97 ++++++++++++++++++------------
>>   1 file changed, 59 insertions(+), 38 deletions(-)
>> 
>> diff --git a/arch/powerpc/include/asm/uaccess.h b/arch/powerpc/include/asm/uaccess.h
>> index 670910df3cc7..1cf8595aeef1 100644
>> --- a/arch/powerpc/include/asm/uaccess.h
>> +++ b/arch/powerpc/include/asm/uaccess.h
>> @@ -162,36 +162,48 @@ do {								\
>>   	prevent_write_to_user(ptr, size);			\
>>   } while (0)
>>   
>> -#define __put_user_nocheck(x, ptr, size, do_allow)			\
>> +#define __put_user_nocheck(x, ptr, size, do_allow)		\
> 
> No need to touch this line. Anyway at the end, you still have several \ 
> which are not aligned.
> 
>>   ({								\
>>   	long __pu_err;						\
>>   	__typeof__(*(ptr)) __user *__pu_addr = (ptr);		\
>> +	__typeof__(*(ptr)) __pu_val = (x);			\
>> +	__typeof__(size) __pu_size = (size);			\
>> +								\
>>   	if (!is_kernel_addr((unsigned long)__pu_addr))		\
>>   		might_fault();					\
>> -	__chk_user_ptr(ptr);					\
>> -	if (do_allow)								\
> 
> No need to touch that line
> 
>> -		__put_user_size((x), __pu_addr, (size), __pu_err);		\
>> -	else									\
> 
> No need to touch that line

Okay fair point I can redo the patch.

Thanks,
Nick

[PATCH 4/4] powerpc/uaccess: add more __builtin_expect annotations

[Nicholas Piggin]

Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
---
 arch/powerpc/include/asm/uaccess.h | 18 +++++++++---------
 1 file changed, 9 insertions(+), 9 deletions(-)

diff --git a/arch/powerpc/include/asm/uaccess.h b/arch/powerpc/include/asm/uaccess.h
index 1cf8595aeef1..896d43d8c891 100644
--- a/arch/powerpc/include/asm/uaccess.h
+++ b/arch/powerpc/include/asm/uaccess.h
@@ -48,16 +48,16 @@ static inline void set_fs(mm_segment_t fs)
  * gap between user addresses and the kernel addresses
  */
 #define __access_ok(addr, size, segment)	\
-	(((addr) <= (segment).seg) && ((size) <= (segment).seg))
+	likely(((addr) <= (segment).seg) && ((size) <= (segment).seg))
 
 #else
 
 static inline int __access_ok(unsigned long addr, unsigned long size,
 			mm_segment_t seg)
 {
-	if (addr > seg.seg)
+	if (unlikely(addr > seg.seg))
 		return 0;
-	return (size == 0 || size - 1 <= seg.seg - addr);
+	return likely(size == 0 || size - 1 <= seg.seg - addr);
 }
 
 #endif
@@ -177,7 +177,7 @@ do {								\
 	else							\
 		__put_user_size_allowed(__pu_val, __pu_addr, __pu_size, __pu_err); \
 								\
-	__pu_err;						\
+	__builtin_expect(__pu_err, 0);				\
 })
 
 #define __put_user_check(x, ptr, size)				\
@@ -191,7 +191,7 @@ do {								\
 	if (access_ok(__pu_addr, __pu_size))			\
 		__put_user_size(__pu_val, __pu_addr, __pu_size, __pu_err); \
 								\
-	__pu_err;						\
+	__builtin_expect(__pu_err, 0);				\
 })
 
 #define __put_user_nosleep(x, ptr, size)			\
@@ -204,7 +204,7 @@ do {								\
 	__chk_user_ptr(__pu_addr);				\
 	__put_user_size(__pu_val, __pu_addr, __pu_size, __pu_err); \
 								\
-	__pu_err;						\
+	__builtin_expect(__pu_err, 0);				\
 })
 
 
@@ -307,7 +307,7 @@ do {								\
 		__get_user_size_allowed(__gu_val, __gu_addr, __gu_size, __gu_err); \
 	(x) = (__typeof__(*(ptr)))__gu_val;			\
 								\
-	__gu_err;						\
+	__builtin_expect(__gu_err, 0);				\
 })
 
 #define __get_user_check(x, ptr, size)				\
@@ -324,7 +324,7 @@ do {								\
 	}							\
 	(x) = (__force __typeof__(*(ptr)))__gu_val;		\
 								\
-	__gu_err;						\
+	__builtin_expect(__gu_err, 0);				\
 })
 
 #define __get_user_nosleep(x, ptr, size)			\
@@ -339,7 +339,7 @@ do {								\
 	__get_user_size(__gu_val, __gu_addr, __gu_size, __gu_err); \
 	(x) = (__force __typeof__(*(ptr)))__gu_val;		\
 								\
-	__gu_err;						\
+	__builtin_expect(__gu_err, 0);				\
 })
 
 
-- 
2.23.0

Re: [PATCH 1/4] powerpc/64s: implement probe_kernel_read/write without touching AMR

[Christophe Leroy]

Le 27/03/2020 à 08:02, Nicholas Piggin a écrit :
> There is no need to allow user accesses when probing kernel addresses.
> 
> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
> ---
>   arch/powerpc/include/asm/uaccess.h | 25 ++++++++++-----
>   arch/powerpc/lib/Makefile          |  2 +-
>   arch/powerpc/lib/uaccess.c         | 50 ++++++++++++++++++++++++++++++
>   3 files changed, 68 insertions(+), 9 deletions(-)
>   create mode 100644 arch/powerpc/lib/uaccess.c
> 

[...]

> diff --git a/arch/powerpc/lib/Makefile b/arch/powerpc/lib/Makefile
> index b8de3be10eb4..a15060b5008e 100644
> --- a/arch/powerpc/lib/Makefile
> +++ b/arch/powerpc/lib/Makefile
> @@ -36,7 +36,7 @@ extra-$(CONFIG_PPC64)	+= crtsavres.o
>   endif
>   
>   obj-$(CONFIG_PPC_BOOK3S_64) += copyuser_power7.o copypage_power7.o \
> -			       memcpy_power7.o
> +			       memcpy_power7.o uaccess.o

Why only book3s/64 ? It applies to the 8xx and book3s/32 as well, I 
think it should just be for all powerpc.

>   
>   obj64-y	+= copypage_64.o copyuser_64.o mem_64.o hweight_64.o \
>   	   memcpy_64.o memcpy_mcsafe_64.o
> diff --git a/arch/powerpc/lib/uaccess.c b/arch/powerpc/lib/uaccess.c
> new file mode 100644
> index 000000000000..0057ab52d6fe
> --- /dev/null
> +++ b/arch/powerpc/lib/uaccess.c
> @@ -0,0 +1,50 @@
> +#include <linux/mm.h>
> +#include <linux/uaccess.h>
> +
> +static __always_inline long
> +probe_read_common(void *dst, const void __user *src, size_t size)
> +{
> +	long ret;
> +
> +	pagefault_disable();
> +	ret = raw_copy_from_user_allowed(dst, src, size);
> +	pagefault_enable();
> +
> +	return ret ? -EFAULT : 0;
> +}
> +
> +static __always_inline long
> +probe_write_common(void __user *dst, const void *src, size_t size)
> +{
> +	long ret;
> +
> +	pagefault_disable();
> +	ret = raw_copy_to_user_allowed(dst, src, size);
> +	pagefault_enable();
> +
> +	return ret ? -EFAULT : 0;
> +}
> +
> +long probe_kernel_read(void *dst, const void *src, size_t size)
> +{
> +	long ret;
> +	mm_segment_t old_fs = get_fs();
> +
> +	set_fs(KERNEL_DS);
> +	ret = probe_read_common(dst, (__force const void __user *)src, size);

I think you should squash probe_read_common() here, having it separated 
is a lot of lines for no added value. It also may make people believe it 
overwrites the generic probe_read_common()


> +	set_fs(old_fs);
> +
> +	return ret;
> +}
> +
> +long probe_kernel_write(void *dst, const void *src, size_t size)
> +{
> +	long ret;
> +	mm_segment_t old_fs = get_fs();
> +
> +	set_fs(KERNEL_DS);
> +	ret = probe_write_common((__force void __user *)dst, src, size);

Same comment as for probe_read_common()

> +	set_fs(old_fs);
> +
> +	return ret;
> +}
> 

Christophe

Re: [PATCH 1/4] powerpc/64s: implement probe_kernel_read/write without touching AMR

[Nicholas Piggin]

Christophe Leroy's on March 27, 2020 5:13 pm:
> 
> 
> Le 27/03/2020 à 08:02, Nicholas Piggin a écrit :
>> There is no need to allow user accesses when probing kernel addresses.
>> 
>> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
>> ---
>>   arch/powerpc/include/asm/uaccess.h | 25 ++++++++++-----
>>   arch/powerpc/lib/Makefile          |  2 +-
>>   arch/powerpc/lib/uaccess.c         | 50 ++++++++++++++++++++++++++++++
>>   3 files changed, 68 insertions(+), 9 deletions(-)
>>   create mode 100644 arch/powerpc/lib/uaccess.c
>> 
> 
> [...]
> 
>> diff --git a/arch/powerpc/lib/Makefile b/arch/powerpc/lib/Makefile
>> index b8de3be10eb4..a15060b5008e 100644
>> --- a/arch/powerpc/lib/Makefile
>> +++ b/arch/powerpc/lib/Makefile
>> @@ -36,7 +36,7 @@ extra-$(CONFIG_PPC64)	+= crtsavres.o
>>   endif
>>   
>>   obj-$(CONFIG_PPC_BOOK3S_64) += copyuser_power7.o copypage_power7.o \
>> -			       memcpy_power7.o
>> +			       memcpy_power7.o uaccess.o
> 
> Why only book3s/64 ? It applies to the 8xx and book3s/32 as well, I 
> think it should just be for all powerpc.

Okay I can do that.

>>   
>>   obj64-y	+= copypage_64.o copyuser_64.o mem_64.o hweight_64.o \
>>   	   memcpy_64.o memcpy_mcsafe_64.o
>> diff --git a/arch/powerpc/lib/uaccess.c b/arch/powerpc/lib/uaccess.c
>> new file mode 100644
>> index 000000000000..0057ab52d6fe
>> --- /dev/null
>> +++ b/arch/powerpc/lib/uaccess.c
>> @@ -0,0 +1,50 @@
>> +#include <linux/mm.h>
>> +#include <linux/uaccess.h>
>> +
>> +static __always_inline long
>> +probe_read_common(void *dst, const void __user *src, size_t size)
>> +{
>> +	long ret;
>> +
>> +	pagefault_disable();
>> +	ret = raw_copy_from_user_allowed(dst, src, size);
>> +	pagefault_enable();
>> +
>> +	return ret ? -EFAULT : 0;
>> +}
>> +
>> +static __always_inline long
>> +probe_write_common(void __user *dst, const void *src, size_t size)
>> +{
>> +	long ret;
>> +
>> +	pagefault_disable();
>> +	ret = raw_copy_to_user_allowed(dst, src, size);
>> +	pagefault_enable();
>> +
>> +	return ret ? -EFAULT : 0;
>> +}
>> +
>> +long probe_kernel_read(void *dst, const void *src, size_t size)
>> +{
>> +	long ret;
>> +	mm_segment_t old_fs = get_fs();
>> +
>> +	set_fs(KERNEL_DS);
>> +	ret = probe_read_common(dst, (__force const void __user *)src, size);
> 
> I think you should squash probe_read_common() here, having it separated 
> is a lot of lines for no added value. It also may make people believe it 
> overwrites the generic probe_read_common()

Yeah I'll see how that looks.

Thanks,
Nick