* [PATCH v2 1/4] powerpc/64s: implement probe_kernel_read/write without touching AMR
@ 2020-04-03 9:35 Nicholas Piggin
2020-04-03 9:35 ` [PATCH v2 2/4] powerpc/64s: use mmu_has_feature in set_kuap() and get_kuap() Nicholas Piggin
` (4 more replies)
0 siblings, 5 replies; 12+ messages in thread
From: Nicholas Piggin @ 2020-04-03 9:35 UTC (permalink / raw)
To: linuxppc-dev; +Cc: Nicholas Piggin
There is no need to allow user accesses when probing kernel addresses.
Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
---
v2:
- Enable for all powerpc (suggested by Christophe)
- Fold helper function together (Christophe)
- Rename uaccess.c to maccess.c to match kernel/maccess.c.
arch/powerpc/include/asm/uaccess.h | 25 +++++++++++++++-------
arch/powerpc/lib/Makefile | 2 +-
arch/powerpc/lib/maccess.c | 34 ++++++++++++++++++++++++++++++
3 files changed, 52 insertions(+), 9 deletions(-)
create mode 100644 arch/powerpc/lib/maccess.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..77af10ad0b0d 100644
--- a/arch/powerpc/lib/Makefile
+++ b/arch/powerpc/lib/Makefile
@@ -16,7 +16,7 @@ CFLAGS_code-patching.o += -DDISABLE_BRANCH_PROFILING
CFLAGS_feature-fixups.o += -DDISABLE_BRANCH_PROFILING
endif
-obj-y += alloc.o code-patching.o feature-fixups.o pmem.o
+obj-y += alloc.o code-patching.o feature-fixups.o pmem.o maccess.o
ifndef CONFIG_KASAN
obj-y += string.o memcmp_$(BITS).o
diff --git a/arch/powerpc/lib/maccess.c b/arch/powerpc/lib/maccess.c
new file mode 100644
index 000000000000..ce5465db1e2d
--- /dev/null
+++ b/arch/powerpc/lib/maccess.c
@@ -0,0 +1,34 @@
+#include <linux/mm.h>
+#include <linux/uaccess.h>
+
+/*
+ * Override the generic weak linkage functions to avoid changing KUP state via
+ * the generic user access functions, as this is accessing kernel addresses.
+ */
+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);
+ pagefault_disable();
+ ret = raw_copy_from_user_allowed(dst, (__force const void __user *)src, size);
+ pagefault_enable();
+ set_fs(old_fs);
+
+ return ret ? -EFAULT : 0;
+}
+
+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);
+ pagefault_disable();
+ ret = raw_copy_to_user_allowed((__force void __user *)dst, src, size);
+ pagefault_enable();
+ set_fs(old_fs);
+
+ return ret ? -EFAULT : 0;
+}
--
2.23.0
^ permalink raw reply related [flat|nested] 12+ messages in thread
* [PATCH v2 2/4] powerpc/64s: use mmu_has_feature in set_kuap() and get_kuap()
2020-04-03 9:35 [PATCH v2 1/4] powerpc/64s: implement probe_kernel_read/write without touching AMR Nicholas Piggin
@ 2020-04-03 9:35 ` Nicholas Piggin
2020-04-03 9:35 ` [PATCH v2 3/4] powerpc/uaccess: evaluate macro arguments once, before user access is allowed Nicholas Piggin
` (3 subsequent siblings)
4 siblings, 0 replies; 12+ messages in thread
From: Nicholas Piggin @ 2020-04-03 9:35 UTC (permalink / raw)
To: linuxppc-dev; +Cc: 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
^ permalink raw reply related [flat|nested] 12+ messages in thread
* [PATCH v2 3/4] powerpc/uaccess: evaluate macro arguments once, before user access is allowed
2020-04-03 9:35 [PATCH v2 1/4] powerpc/64s: implement probe_kernel_read/write without touching AMR Nicholas Piggin
2020-04-03 9:35 ` [PATCH v2 2/4] powerpc/64s: use mmu_has_feature in set_kuap() and get_kuap() Nicholas Piggin
@ 2020-04-03 9:35 ` Nicholas Piggin
2020-04-03 9:35 ` [PATCH v2 4/4] powerpc/uaccess: add more __builtin_expect annotations Nicholas Piggin
` (2 subsequent siblings)
4 siblings, 0 replies; 12+ messages in thread
From: Nicholas Piggin @ 2020-04-03 9:35 UTC (permalink / raw)
To: linuxppc-dev; +Cc: 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>
---
v2:
- Reduce unnecessary lines changed (Christophe)
arch/powerpc/include/asm/uaccess.h | 49 +++++++++++++++++++++---------
1 file changed, 35 insertions(+), 14 deletions(-)
diff --git a/arch/powerpc/include/asm/uaccess.h b/arch/powerpc/include/asm/uaccess.h
index 670910df3cc7..144d01645d68 100644
--- a/arch/powerpc/include/asm/uaccess.h
+++ b/arch/powerpc/include/asm/uaccess.h
@@ -166,13 +166,17 @@ do { \
({ \
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); \
+ __chk_user_ptr(__pu_addr); \
if (do_allow) \
- __put_user_size((x), __pu_addr, (size), __pu_err); \
+ __put_user_size(__pu_val, __pu_addr, __pu_size, __pu_err); \
else \
- __put_user_size_allowed((x), __pu_addr, (size), __pu_err); \
+ __put_user_size_allowed(__pu_val, __pu_addr, __pu_size, __pu_err); \
+ \
__pu_err; \
})
@@ -180,9 +184,13 @@ do { \
({ \
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, size)) \
- __put_user_size((x), __pu_addr, (size), __pu_err); \
+ if (access_ok(__pu_addr, __pu_size)) \
+ __put_user_size(__pu_val, __pu_addr, __pu_size, __pu_err); \
+ \
__pu_err; \
})
@@ -190,8 +198,12 @@ do { \
({ \
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; \
})
@@ -283,15 +295,18 @@ do { \
long __gu_err; \
__long_type(*(ptr)) __gu_val; \
__typeof__(*(ptr)) __user *__gu_addr = (ptr); \
- __chk_user_ptr(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); \
+ __get_user_size(__gu_val, __gu_addr, __gu_size, __gu_err); \
else \
- __get_user_size_allowed(__gu_val, __gu_addr, (size), __gu_err); \
+ __get_user_size_allowed(__gu_val, __gu_addr, __gu_size, __gu_err); \
(x) = (__typeof__(*(ptr)))__gu_val; \
+ \
__gu_err; \
})
@@ -300,12 +315,15 @@ do { \
long __gu_err = -EFAULT; \
__long_type(*(ptr)) __gu_val = 0; \
__typeof__(*(ptr)) __user *__gu_addr = (ptr); \
+ __typeof__(size) __gu_size = (size); \
+ \
might_fault(); \
- if (access_ok(__gu_addr, (size))) { \
+ if (access_ok(__gu_addr, __gu_size)) { \
barrier_nospec(); \
- __get_user_size(__gu_val, __gu_addr, (size), __gu_err); \
+ __get_user_size(__gu_val, __gu_addr, __gu_size, __gu_err); \
} \
(x) = (__force __typeof__(*(ptr)))__gu_val; \
+ \
__gu_err; \
})
@@ -314,10 +332,13 @@ do { \
long __gu_err; \
__long_type(*(ptr)) __gu_val; \
__typeof__(*(ptr)) __user *__gu_addr = (ptr); \
- __chk_user_ptr(ptr); \
+ __typeof__(size) __gu_size = (size); \
+ \
+ __chk_user_ptr(__gu_addr); \
barrier_nospec(); \
- __get_user_size(__gu_val, __gu_addr, (size), __gu_err); \
+ __get_user_size(__gu_val, __gu_addr, __gu_size, __gu_err); \
(x) = (__force __typeof__(*(ptr)))__gu_val; \
+ \
__gu_err; \
})
--
2.23.0
^ permalink raw reply related [flat|nested] 12+ messages in thread
* [PATCH v2 4/4] powerpc/uaccess: add more __builtin_expect annotations
2020-04-03 9:35 [PATCH v2 1/4] powerpc/64s: implement probe_kernel_read/write without touching AMR Nicholas Piggin
2020-04-03 9:35 ` [PATCH v2 2/4] powerpc/64s: use mmu_has_feature in set_kuap() and get_kuap() Nicholas Piggin
2020-04-03 9:35 ` [PATCH v2 3/4] powerpc/uaccess: evaluate macro arguments once, before user access is allowed Nicholas Piggin
@ 2020-04-03 9:35 ` Nicholas Piggin
2020-04-03 10:35 ` Nicholas Piggin
2020-04-04 14:56 ` kbuild test robot
2020-04-03 10:31 ` [PATCH v2 1/4] powerpc/64s: implement probe_kernel_read/write without touching AMR Christophe Leroy
2020-06-10 12:41 ` Christophe Leroy
4 siblings, 2 replies; 12+ messages in thread
From: Nicholas Piggin @ 2020-04-03 9:35 UTC (permalink / raw)
To: linuxppc-dev; +Cc: Nicholas Piggin
Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
---
arch/powerpc/include/asm/uaccess.h | 16 +++++++++-------
1 file changed, 9 insertions(+), 7 deletions(-)
diff --git a/arch/powerpc/include/asm/uaccess.h b/arch/powerpc/include/asm/uaccess.h
index 144d01645d68..c6b0203c3750 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) \
@@ -192,6 +192,7 @@ do { \
__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 +205,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 +308,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) \
@@ -325,6 +326,7 @@ do { \
(x) = (__force __typeof__(*(ptr)))__gu_val; \
\
__gu_err; \
+ __builtin_expect(__gu_err, 0); \
})
#define __get_user_nosleep(x, ptr, size) \
@@ -339,7 +341,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
^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH v2 1/4] powerpc/64s: implement probe_kernel_read/write without touching AMR
2020-04-03 9:35 [PATCH v2 1/4] powerpc/64s: implement probe_kernel_read/write without touching AMR Nicholas Piggin
` (2 preceding siblings ...)
2020-04-03 9:35 ` [PATCH v2 4/4] powerpc/uaccess: add more __builtin_expect annotations Nicholas Piggin
@ 2020-04-03 10:31 ` Christophe Leroy
2020-04-03 11:05 ` Nicholas Piggin
2020-06-10 12:41 ` Christophe Leroy
4 siblings, 1 reply; 12+ messages in thread
From: Christophe Leroy @ 2020-04-03 10:31 UTC (permalink / raw)
To: Nicholas Piggin, linuxppc-dev
Le 03/04/2020 à 11:35, Nicholas Piggin a écrit :
> There is no need to allow user accesses when probing kernel addresses.
I just discovered the following commit
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=75a1a607bb7e6d918be3aca11ec2214a275392f4
This commit adds probe_kernel_read_strict() and probe_kernel_write_strict().
When reading the commit log, I understand that probe_kernel_read() may
be used to access some user memory. Which will not work anymore with
your patch.
Isn't it probe_kernel_read_strict() and probe_kernel_write_strict() that
you want to add ?
>
> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
> ---
> v2:
> - Enable for all powerpc (suggested by Christophe)
> - Fold helper function together (Christophe)
> - Rename uaccess.c to maccess.c to match kernel/maccess.c.
>
> arch/powerpc/include/asm/uaccess.h | 25 +++++++++++++++-------
> arch/powerpc/lib/Makefile | 2 +-
> arch/powerpc/lib/maccess.c | 34 ++++++++++++++++++++++++++++++
x86 does it in mm/maccess.c
> 3 files changed, 52 insertions(+), 9 deletions(-)
> create mode 100644 arch/powerpc/lib/maccess.c
>
Christophe
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v2 4/4] powerpc/uaccess: add more __builtin_expect annotations
2020-04-03 9:35 ` [PATCH v2 4/4] powerpc/uaccess: add more __builtin_expect annotations Nicholas Piggin
@ 2020-04-03 10:35 ` Nicholas Piggin
2020-04-04 14:56 ` kbuild test robot
1 sibling, 0 replies; 12+ messages in thread
From: Nicholas Piggin @ 2020-04-03 10:35 UTC (permalink / raw)
To: linuxppc-dev
Nicholas Piggin's on April 3, 2020 7:35 pm:
> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
Sorry that was a bad rebase, here's the fixed patch.
---
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 144d01645d68..8a0474682c9b 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
^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH v2 1/4] powerpc/64s: implement probe_kernel_read/write without touching AMR
2020-04-03 10:31 ` [PATCH v2 1/4] powerpc/64s: implement probe_kernel_read/write without touching AMR Christophe Leroy
@ 2020-04-03 11:05 ` Nicholas Piggin
2020-04-07 4:01 ` Nicholas Piggin
0 siblings, 1 reply; 12+ messages in thread
From: Nicholas Piggin @ 2020-04-03 11:05 UTC (permalink / raw)
To: Christophe Leroy, linuxppc-dev
Cc: linux-arch, Daniel Borkmann, x86, linux-kernel,
Alexei Starovoitov, Masami Hiramatsu, Linus Torvalds
Christophe Leroy's on April 3, 2020 8:31 pm:
>
>
> Le 03/04/2020 à 11:35, Nicholas Piggin a écrit :
>> There is no need to allow user accesses when probing kernel addresses.
>
> I just discovered the following commit
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=75a1a607bb7e6d918be3aca11ec2214a275392f4
>
> This commit adds probe_kernel_read_strict() and probe_kernel_write_strict().
>
> When reading the commit log, I understand that probe_kernel_read() may
> be used to access some user memory. Which will not work anymore with
> your patch.
Hmm, I looked at _strict but obviously not hard enough. Good catch.
I don't think probe_kernel_read() should ever access user memory,
the comment certainly says it doesn't, but that patch sort of implies
that they do.
I think it's wrong. The non-_strict maybe could return userspace data to
you if you did pass a user address? I don't see why that shouldn't just
be disallowed always though.
And if the _strict version is required to be safe, then it seems like a
bug or security issue to just allow everyone that doesn't explicitly
override it to use the default implementation.
Also, the way the weak linkage is done in that patch, means parisc and
um archs that were previously overriding probe_kernel_read() now get
the default probe_kernel_read_strict(), which would be wrong for them.
>
> Isn't it probe_kernel_read_strict() and probe_kernel_write_strict() that
> you want to add ?
>
>>
>> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
>> ---
>> v2:
>> - Enable for all powerpc (suggested by Christophe)
>> - Fold helper function together (Christophe)
>> - Rename uaccess.c to maccess.c to match kernel/maccess.c.
>>
>> arch/powerpc/include/asm/uaccess.h | 25 +++++++++++++++-------
>> arch/powerpc/lib/Makefile | 2 +-
>> arch/powerpc/lib/maccess.c | 34 ++++++++++++++++++++++++++++++
>
> x86 does it in mm/maccess.c
Yeah I'll fix that up, thanks.
Thanks,
Nick
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v2 4/4] powerpc/uaccess: add more __builtin_expect annotations
2020-04-03 9:35 ` [PATCH v2 4/4] powerpc/uaccess: add more __builtin_expect annotations Nicholas Piggin
2020-04-03 10:35 ` Nicholas Piggin
@ 2020-04-04 14:56 ` kbuild test robot
1 sibling, 0 replies; 12+ messages in thread
From: kbuild test robot @ 2020-04-04 14:56 UTC (permalink / raw)
To: Nicholas Piggin; +Cc: linuxppc-dev, kbuild-all
[-- Attachment #1: Type: text/plain, Size: 34901 bytes --]
Hi Nicholas,
I love your patch! Perhaps something to improve:
[auto build test WARNING on powerpc/next]
[also build test WARNING on v5.6 next-20200404]
[cannot apply to scottwood/next]
[if your patch is applied to the wrong git tree, please drop us a note to help
improve the system. BTW, we also suggest to use '--base' option to specify the
base tree in git format-patch, please see https://stackoverflow.com/a/37406982]
url: https://github.com/0day-ci/linux/commits/Nicholas-Piggin/powerpc-64s-implement-probe_kernel_read-write-without-touching-AMR/20200404-192647
base: https://git.kernel.org/pub/scm/linux/kernel/git/powerpc/linux.git next
config: powerpc-rhel-kconfig (attached as .config)
compiler: powerpc64le-linux-gcc (GCC) 9.3.0
reproduce:
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# save the attached .config to linux build tree
GCC_VERSION=9.3.0 make.cross ARCH=powerpc
If you fix the issue, kindly add following tag as appropriate
Reported-by: kbuild test robot <lkp@intel.com>
All warnings (new ones prefixed by >>):
In file included from include/linux/uaccess.h:11,
from arch/powerpc/include/asm/sections.h:7,
from include/linux/interrupt.h:20,
from include/linux/serial_core.h:13,
from drivers/of/fdt.c:25:
include/asm-generic/termios-base.h: In function 'user_termio_to_kernel_termios':
arch/powerpc/include/asm/uaccess.h:328:2: warning: statement with no effect [-Wunused-value]
328 | __gu_err; \
| ^~~~~~~~
>> arch/powerpc/include/asm/uaccess.h:89:2: note: in expansion of macro '__get_user_check'
89 | __get_user_check((x), (ptr), sizeof(*(ptr)))
| ^~~~~~~~~~~~~~~~
>> include/asm-generic/termios-base.h:20:6: note: in expansion of macro 'get_user'
20 | if (get_user(tmp, &termio->c_iflag) < 0)
| ^~~~~~~~
arch/powerpc/include/asm/uaccess.h:328:2: warning: statement with no effect [-Wunused-value]
328 | __gu_err; \
| ^~~~~~~~
>> arch/powerpc/include/asm/uaccess.h:89:2: note: in expansion of macro '__get_user_check'
89 | __get_user_check((x), (ptr), sizeof(*(ptr)))
| ^~~~~~~~~~~~~~~~
include/asm-generic/termios-base.h:24:6: note: in expansion of macro 'get_user'
24 | if (get_user(tmp, &termio->c_oflag) < 0)
| ^~~~~~~~
arch/powerpc/include/asm/uaccess.h:328:2: warning: statement with no effect [-Wunused-value]
328 | __gu_err; \
| ^~~~~~~~
>> arch/powerpc/include/asm/uaccess.h:89:2: note: in expansion of macro '__get_user_check'
89 | __get_user_check((x), (ptr), sizeof(*(ptr)))
| ^~~~~~~~~~~~~~~~
include/asm-generic/termios-base.h:28:6: note: in expansion of macro 'get_user'
28 | if (get_user(tmp, &termio->c_cflag) < 0)
| ^~~~~~~~
arch/powerpc/include/asm/uaccess.h:328:2: warning: statement with no effect [-Wunused-value]
328 | __gu_err; \
| ^~~~~~~~
>> arch/powerpc/include/asm/uaccess.h:89:2: note: in expansion of macro '__get_user_check'
89 | __get_user_check((x), (ptr), sizeof(*(ptr)))
| ^~~~~~~~~~~~~~~~
include/asm-generic/termios-base.h:32:6: note: in expansion of macro 'get_user'
32 | if (get_user(tmp, &termio->c_lflag) < 0)
| ^~~~~~~~
arch/powerpc/include/asm/uaccess.h:328:2: warning: statement with no effect [-Wunused-value]
328 | __gu_err; \
| ^~~~~~~~
>> arch/powerpc/include/asm/uaccess.h:89:2: note: in expansion of macro '__get_user_check'
89 | __get_user_check((x), (ptr), sizeof(*(ptr)))
| ^~~~~~~~~~~~~~~~
include/asm-generic/termios-base.h:36:6: note: in expansion of macro 'get_user'
36 | if (get_user(termios->c_line, &termio->c_line) < 0)
| ^~~~~~~~
include/asm-generic/termios-base.h: In function 'kernel_termios_to_user_termio':
arch/powerpc/include/asm/uaccess.h:194:2: warning: statement with no effect [-Wunused-value]
194 | __pu_err; \
| ^~~~~~~~
>> arch/powerpc/include/asm/uaccess.h:91:2: note: in expansion of macro '__put_user_check'
91 | __put_user_check((__typeof__(*(ptr)))(x), (ptr), sizeof(*(ptr)))
| ^~~~~~~~~~~~~~~~
>> include/asm-generic/termios-base.h:54:6: note: in expansion of macro 'put_user'
54 | if (put_user(termios->c_iflag, &termio->c_iflag) < 0 ||
| ^~~~~~~~
arch/powerpc/include/asm/uaccess.h:194:2: warning: statement with no effect [-Wunused-value]
194 | __pu_err; \
| ^~~~~~~~
>> arch/powerpc/include/asm/uaccess.h:91:2: note: in expansion of macro '__put_user_check'
91 | __put_user_check((__typeof__(*(ptr)))(x), (ptr), sizeof(*(ptr)))
| ^~~~~~~~~~~~~~~~
include/asm-generic/termios-base.h:55:6: note: in expansion of macro 'put_user'
55 | put_user(termios->c_oflag, &termio->c_oflag) < 0 ||
| ^~~~~~~~
arch/powerpc/include/asm/uaccess.h:194:2: warning: statement with no effect [-Wunused-value]
194 | __pu_err; \
| ^~~~~~~~
>> arch/powerpc/include/asm/uaccess.h:91:2: note: in expansion of macro '__put_user_check'
91 | __put_user_check((__typeof__(*(ptr)))(x), (ptr), sizeof(*(ptr)))
| ^~~~~~~~~~~~~~~~
include/asm-generic/termios-base.h:56:6: note: in expansion of macro 'put_user'
56 | put_user(termios->c_cflag, &termio->c_cflag) < 0 ||
| ^~~~~~~~
arch/powerpc/include/asm/uaccess.h:194:2: warning: statement with no effect [-Wunused-value]
194 | __pu_err; \
| ^~~~~~~~
>> arch/powerpc/include/asm/uaccess.h:91:2: note: in expansion of macro '__put_user_check'
91 | __put_user_check((__typeof__(*(ptr)))(x), (ptr), sizeof(*(ptr)))
| ^~~~~~~~~~~~~~~~
include/asm-generic/termios-base.h:57:6: note: in expansion of macro 'put_user'
57 | put_user(termios->c_lflag, &termio->c_lflag) < 0 ||
| ^~~~~~~~
arch/powerpc/include/asm/uaccess.h:194:2: warning: statement with no effect [-Wunused-value]
194 | __pu_err; \
| ^~~~~~~~
>> arch/powerpc/include/asm/uaccess.h:91:2: note: in expansion of macro '__put_user_check'
91 | __put_user_check((__typeof__(*(ptr)))(x), (ptr), sizeof(*(ptr)))
| ^~~~~~~~~~~~~~~~
include/asm-generic/termios-base.h:58:6: note: in expansion of macro 'put_user'
58 | put_user(termios->c_line, &termio->c_line) < 0 ||
| ^~~~~~~~
--
In file included from include/linux/uaccess.h:11,
from include/linux/sched/task.h:11,
from include/linux/sched/signal.h:9,
from include/linux/ptrace.h:7,
from arch/powerpc/kernel/signal_32.c:23:
arch/powerpc/kernel/signal_32.c: In function 'handle_rt_signal32':
arch/powerpc/include/asm/uaccess.h:194:2: warning: statement with no effect [-Wunused-value]
194 | __pu_err; \
| ^~~~~~~~
>> arch/powerpc/include/asm/uaccess.h:91:2: note: in expansion of macro '__put_user_check'
91 | __put_user_check((__typeof__(*(ptr)))(x), (ptr), sizeof(*(ptr)))
| ^~~~~~~~~~~~~~~~
>> arch/powerpc/kernel/signal_32.c:967:6: note: in expansion of macro 'put_user'
967 | if (put_user(regs->gpr[1], (u32 __user *)newsp))
| ^~~~~~~~
arch/powerpc/kernel/signal_32.c: In function 'handle_signal32':
arch/powerpc/include/asm/uaccess.h:194:2: warning: statement with no effect [-Wunused-value]
194 | __pu_err; \
| ^~~~~~~~
>> arch/powerpc/include/asm/uaccess.h:91:2: note: in expansion of macro '__put_user_check'
91 | __put_user_check((__typeof__(*(ptr)))(x), (ptr), sizeof(*(ptr)))
| ^~~~~~~~~~~~~~~~
arch/powerpc/kernel/signal_32.c:1423:6: note: in expansion of macro 'put_user'
1423 | if (put_user(regs->gpr[1], (u32 __user *)newsp))
| ^~~~~~~~
--
In file included from include/linux/uaccess.h:11,
from include/linux/sched/task.h:11,
from arch/powerpc/kernel/process.c:16:
arch/powerpc/kernel/process.c: In function 'get_fpexc_mode':
arch/powerpc/include/asm/uaccess.h:194:2: warning: statement with no effect [-Wunused-value]
194 | __pu_err; \
| ^~~~~~~~
>> arch/powerpc/include/asm/uaccess.h:91:2: note: in expansion of macro '__put_user_check'
91 | __put_user_check((__typeof__(*(ptr)))(x), (ptr), sizeof(*(ptr)))
| ^~~~~~~~~~~~~~~~
>> arch/powerpc/kernel/process.c:1909:9: note: in expansion of macro 'put_user'
1909 | return put_user(val, (unsigned int __user *) adr);
| ^~~~~~~~
arch/powerpc/kernel/process.c: In function 'get_endian':
arch/powerpc/include/asm/uaccess.h:194:2: warning: statement with no effect [-Wunused-value]
194 | __pu_err; \
| ^~~~~~~~
>> arch/powerpc/include/asm/uaccess.h:91:2: note: in expansion of macro '__put_user_check'
91 | __put_user_check((__typeof__(*(ptr)))(x), (ptr), sizeof(*(ptr)))
| ^~~~~~~~~~~~~~~~
arch/powerpc/kernel/process.c:1953:9: note: in expansion of macro 'put_user'
1953 | return put_user(val, (unsigned int __user *)adr);
| ^~~~~~~~
arch/powerpc/kernel/process.c: In function 'get_unalign_ctl':
arch/powerpc/include/asm/uaccess.h:194:2: warning: statement with no effect [-Wunused-value]
194 | __pu_err; \
| ^~~~~~~~
>> arch/powerpc/include/asm/uaccess.h:91:2: note: in expansion of macro '__put_user_check'
91 | __put_user_check((__typeof__(*(ptr)))(x), (ptr), sizeof(*(ptr)))
| ^~~~~~~~~~~~~~~~
arch/powerpc/kernel/process.c:1964:9: note: in expansion of macro 'put_user'
1964 | return put_user(tsk->thread.align_ctl, (unsigned int __user *)adr);
| ^~~~~~~~
--
In file included from include/linux/uaccess.h:11,
from include/linux/sched/task.h:11,
from include/linux/sched/signal.h:9,
from include/linux/ptrace.h:7,
from arch/powerpc/kernel/traps.c:22:
arch/powerpc/kernel/traps.c: In function 'emulate_string_inst':
arch/powerpc/include/asm/uaccess.h:328:2: warning: statement with no effect [-Wunused-value]
328 | __gu_err; \
| ^~~~~~~~
>> arch/powerpc/include/asm/uaccess.h:89:2: note: in expansion of macro '__get_user_check'
89 | __get_user_check((x), (ptr), sizeof(*(ptr)))
| ^~~~~~~~~~~~~~~~
>> arch/powerpc/kernel/traps.c:1243:9: note: in expansion of macro 'get_user'
1243 | if (get_user(val, (u8 __user *)EA))
| ^~~~~~~~
arch/powerpc/include/asm/uaccess.h:194:2: warning: statement with no effect [-Wunused-value]
194 | __pu_err; \
| ^~~~~~~~
>> arch/powerpc/include/asm/uaccess.h:91:2: note: in expansion of macro '__put_user_check'
91 | __put_user_check((__typeof__(*(ptr)))(x), (ptr), sizeof(*(ptr)))
| ^~~~~~~~~~~~~~~~
>> arch/powerpc/kernel/traps.c:1254:9: note: in expansion of macro 'put_user'
1254 | if (put_user(val, (u8 __user *)EA))
| ^~~~~~~~
arch/powerpc/kernel/traps.c: In function 'emulate_instruction':
arch/powerpc/include/asm/uaccess.h:328:2: warning: statement with no effect [-Wunused-value]
328 | __gu_err; \
| ^~~~~~~~
>> arch/powerpc/include/asm/uaccess.h:89:2: note: in expansion of macro '__get_user_check'
89 | __get_user_check((x), (ptr), sizeof(*(ptr)))
| ^~~~~~~~~~~~~~~~
arch/powerpc/kernel/traps.c:1338:6: note: in expansion of macro 'get_user'
1338 | if (get_user(instword, (u32 __user *)(regs->nip)))
| ^~~~~~~~
arch/powerpc/kernel/traps.c: In function 'facility_unavailable_exception':
arch/powerpc/include/asm/uaccess.h:328:2: warning: statement with no effect [-Wunused-value]
328 | __gu_err; \
| ^~~~~~~~
>> arch/powerpc/include/asm/uaccess.h:89:2: note: in expansion of macro '__get_user_check'
89 | __get_user_check((x), (ptr), sizeof(*(ptr)))
| ^~~~~~~~~~~~~~~~
arch/powerpc/kernel/traps.c:1770:7: note: in expansion of macro 'get_user'
1770 | if (get_user(instword, (u32 __user *)(regs->nip))) {
| ^~~~~~~~
--
In file included from include/linux/uaccess.h:11,
from include/linux/sched/task.h:11,
from include/linux/sched/signal.h:9,
from include/linux/ptrace.h:7,
from arch/powerpc/kernel/signal_64.c:21:
arch/powerpc/kernel/signal_64.c: In function '__do_sys_swapcontext':
arch/powerpc/include/asm/uaccess.h:328:2: warning: statement with no effect [-Wunused-value]
328 | __gu_err; \
| ^~~~~~~~
>> arch/powerpc/include/asm/uaccess.h:89:2: note: in expansion of macro '__get_user_check'
89 | __get_user_check((x), (ptr), sizeof(*(ptr)))
| ^~~~~~~~~~~~~~~~
>> arch/powerpc/kernel/signal_64.c:644:6: note: in expansion of macro 'get_user'
644 | get_user(new_msr, &new_ctx->uc_mcontext.gp_regs[PT_MSR]))
| ^~~~~~~~
arch/powerpc/kernel/signal_64.c: In function 'handle_rt_signal64':
arch/powerpc/include/asm/uaccess.h:194:2: warning: statement with no effect [-Wunused-value]
194 | __pu_err; \
| ^~~~~~~~
>> arch/powerpc/include/asm/uaccess.h:91:2: note: in expansion of macro '__put_user_check'
91 | __put_user_check((__typeof__(*(ptr)))(x), (ptr), sizeof(*(ptr)))
| ^~~~~~~~~~~~~~~~
>> arch/powerpc/kernel/signal_64.c:880:9: note: in expansion of macro 'put_user'
880 | err |= put_user(regs->gpr[1], (unsigned long __user *)newsp);
| ^~~~~~~~
arch/powerpc/include/asm/uaccess.h:328:2: warning: statement with no effect [-Wunused-value]
328 | __gu_err; \
| ^~~~~~~~
>> arch/powerpc/include/asm/uaccess.h:89:2: note: in expansion of macro '__get_user_check'
89 | __get_user_check((x), (ptr), sizeof(*(ptr)))
| ^~~~~~~~~~~~~~~~
arch/powerpc/kernel/signal_64.c:895:10: note: in expansion of macro 'get_user'
895 | err |= get_user(regs->nip, &funct_desc_ptr->entry);
| ^~~~~~~~
arch/powerpc/include/asm/uaccess.h:328:2: warning: statement with no effect [-Wunused-value]
328 | __gu_err; \
| ^~~~~~~~
>> arch/powerpc/include/asm/uaccess.h:89:2: note: in expansion of macro '__get_user_check'
89 | __get_user_check((x), (ptr), sizeof(*(ptr)))
| ^~~~~~~~~~~~~~~~
arch/powerpc/kernel/signal_64.c:896:10: note: in expansion of macro 'get_user'
896 | err |= get_user(regs->gpr[2], &funct_desc_ptr->toc);
| ^~~~~~~~
arch/powerpc/include/asm/uaccess.h:328:2: warning: statement with no effect [-Wunused-value]
328 | __gu_err; \
| ^~~~~~~~
>> arch/powerpc/include/asm/uaccess.h:89:2: note: in expansion of macro '__get_user_check'
89 | __get_user_check((x), (ptr), sizeof(*(ptr)))
| ^~~~~~~~~~~~~~~~
arch/powerpc/kernel/signal_64.c:906:10: note: in expansion of macro 'get_user'
906 | err |= get_user(regs->gpr[4], (unsigned long __user *)&frame->pinfo);
| ^~~~~~~~
arch/powerpc/include/asm/uaccess.h:328:2: warning: statement with no effect [-Wunused-value]
328 | __gu_err; \
| ^~~~~~~~
>> arch/powerpc/include/asm/uaccess.h:89:2: note: in expansion of macro '__get_user_check'
89 | __get_user_check((x), (ptr), sizeof(*(ptr)))
| ^~~~~~~~~~~~~~~~
arch/powerpc/kernel/signal_64.c:907:10: note: in expansion of macro 'get_user'
907 | err |= get_user(regs->gpr[5], (unsigned long __user *)&frame->puc);
| ^~~~~~~~
--
In file included from include/linux/uaccess.h:11,
from arch/powerpc/kernel/vecemu.c:12:
arch/powerpc/kernel/vecemu.c: In function 'emulate_altivec':
arch/powerpc/include/asm/uaccess.h:328:2: warning: statement with no effect [-Wunused-value]
328 | __gu_err; \
| ^~~~~~~~
>> arch/powerpc/include/asm/uaccess.h:89:2: note: in expansion of macro '__get_user_check'
89 | __get_user_check((x), (ptr), sizeof(*(ptr)))
| ^~~~~~~~~~~~~~~~
>> arch/powerpc/kernel/vecemu.c:267:6: note: in expansion of macro 'get_user'
267 | if (get_user(instr, (unsigned int __user *) regs->nip))
| ^~~~~~~~
--
In file included from include/linux/uaccess.h:11,
from arch/powerpc/include/asm/sections.h:7,
from include/linux/interrupt.h:20,
from arch/powerpc/include/asm/kvm_host.h:14,
from include/linux/kvm_host.h:36,
from arch/powerpc/kvm/../../../virt/kvm/vfio.c:11:
arch/powerpc/kvm/../../../virt/kvm/vfio.c: In function 'kvm_vfio_set_group':
arch/powerpc/include/asm/uaccess.h:328:2: warning: statement with no effect [-Wunused-value]
328 | __gu_err; \
| ^~~~~~~~
>> arch/powerpc/include/asm/uaccess.h:89:2: note: in expansion of macro '__get_user_check'
89 | __get_user_check((x), (ptr), sizeof(*(ptr)))
| ^~~~~~~~~~~~~~~~
>> arch/powerpc/kvm/../../../virt/kvm/vfio.c:196:7: note: in expansion of macro 'get_user'
196 | if (get_user(fd, argp))
| ^~~~~~~~
arch/powerpc/include/asm/uaccess.h:328:2: warning: statement with no effect [-Wunused-value]
328 | __gu_err; \
| ^~~~~~~~
>> arch/powerpc/include/asm/uaccess.h:89:2: note: in expansion of macro '__get_user_check'
89 | __get_user_check((x), (ptr), sizeof(*(ptr)))
| ^~~~~~~~~~~~~~~~
arch/powerpc/kvm/../../../virt/kvm/vfio.c:240:7: note: in expansion of macro 'get_user'
240 | if (get_user(fd, argp))
| ^~~~~~~~
..
vim +/__get_user_check +89 arch/powerpc/include/asm/uaccess.h
2df5e8bcca53e5 include/asm-powerpc/uaccess.h Stephen Rothwell 2005-10-29 64
96d4f267e40f95 arch/powerpc/include/asm/uaccess.h Linus Torvalds 2019-01-03 65 #define access_ok(addr, size) \
4caf4ebfe4cf0e arch/powerpc/include/asm/uaccess.h Linus Torvalds 2019-01-04 66 (__chk_user_ptr(addr), \
2df5e8bcca53e5 include/asm-powerpc/uaccess.h Stephen Rothwell 2005-10-29 67 __access_ok((__force unsigned long)(addr), (size), get_fs()))
2df5e8bcca53e5 include/asm-powerpc/uaccess.h Stephen Rothwell 2005-10-29 68
2df5e8bcca53e5 include/asm-powerpc/uaccess.h Stephen Rothwell 2005-10-29 69 /*
2df5e8bcca53e5 include/asm-powerpc/uaccess.h Stephen Rothwell 2005-10-29 70 * These are the main single-value transfer routines. They automatically
2df5e8bcca53e5 include/asm-powerpc/uaccess.h Stephen Rothwell 2005-10-29 71 * use the right size if we just have the right pointer type.
2df5e8bcca53e5 include/asm-powerpc/uaccess.h Stephen Rothwell 2005-10-29 72 *
2df5e8bcca53e5 include/asm-powerpc/uaccess.h Stephen Rothwell 2005-10-29 73 * This gets kind of ugly. We want to return _two_ values in "get_user()"
2df5e8bcca53e5 include/asm-powerpc/uaccess.h Stephen Rothwell 2005-10-29 74 * and yet we don't want to do any pointers, because that is too much
2df5e8bcca53e5 include/asm-powerpc/uaccess.h Stephen Rothwell 2005-10-29 75 * of a performance impact. Thus we have a few rather ugly macros here,
2df5e8bcca53e5 include/asm-powerpc/uaccess.h Stephen Rothwell 2005-10-29 76 * and hide all the ugliness from the user.
2df5e8bcca53e5 include/asm-powerpc/uaccess.h Stephen Rothwell 2005-10-29 77 *
2df5e8bcca53e5 include/asm-powerpc/uaccess.h Stephen Rothwell 2005-10-29 78 * The "__xxx" versions of the user access functions are versions that
2df5e8bcca53e5 include/asm-powerpc/uaccess.h Stephen Rothwell 2005-10-29 79 * do not verify the address space, that must have been done previously
2df5e8bcca53e5 include/asm-powerpc/uaccess.h Stephen Rothwell 2005-10-29 80 * with a separate "access_ok()" call (this is used when we do multiple
2df5e8bcca53e5 include/asm-powerpc/uaccess.h Stephen Rothwell 2005-10-29 81 * accesses to the same area of user memory).
2df5e8bcca53e5 include/asm-powerpc/uaccess.h Stephen Rothwell 2005-10-29 82 *
2df5e8bcca53e5 include/asm-powerpc/uaccess.h Stephen Rothwell 2005-10-29 83 * As we use the same address space for kernel and user data on the
2df5e8bcca53e5 include/asm-powerpc/uaccess.h Stephen Rothwell 2005-10-29 84 * PowerPC, we can just do these as direct assignments. (Of course, the
2df5e8bcca53e5 include/asm-powerpc/uaccess.h Stephen Rothwell 2005-10-29 85 * exception handling means that it's no longer "just"...)
2df5e8bcca53e5 include/asm-powerpc/uaccess.h Stephen Rothwell 2005-10-29 86 *
2df5e8bcca53e5 include/asm-powerpc/uaccess.h Stephen Rothwell 2005-10-29 87 */
2df5e8bcca53e5 include/asm-powerpc/uaccess.h Stephen Rothwell 2005-10-29 88 #define get_user(x, ptr) \
2df5e8bcca53e5 include/asm-powerpc/uaccess.h Stephen Rothwell 2005-10-29 @89 __get_user_check((x), (ptr), sizeof(*(ptr)))
2df5e8bcca53e5 include/asm-powerpc/uaccess.h Stephen Rothwell 2005-10-29 90 #define put_user(x, ptr) \
2df5e8bcca53e5 include/asm-powerpc/uaccess.h Stephen Rothwell 2005-10-29 @91 __put_user_check((__typeof__(*(ptr)))(x), (ptr), sizeof(*(ptr)))
2df5e8bcca53e5 include/asm-powerpc/uaccess.h Stephen Rothwell 2005-10-29 92
2df5e8bcca53e5 include/asm-powerpc/uaccess.h Stephen Rothwell 2005-10-29 93 #define __get_user(x, ptr) \
5cd623333e7cf4 arch/powerpc/include/asm/uaccess.h Christophe Leroy 2020-01-24 94 __get_user_nocheck((x), (ptr), sizeof(*(ptr)), true)
2df5e8bcca53e5 include/asm-powerpc/uaccess.h Stephen Rothwell 2005-10-29 95 #define __put_user(x, ptr) \
5cd623333e7cf4 arch/powerpc/include/asm/uaccess.h Christophe Leroy 2020-01-24 96 __put_user_nocheck((__typeof__(*(ptr)))(x), (ptr), sizeof(*(ptr)), true)
5cd623333e7cf4 arch/powerpc/include/asm/uaccess.h Christophe Leroy 2020-01-24 97
5cd623333e7cf4 arch/powerpc/include/asm/uaccess.h Christophe Leroy 2020-01-24 98 #define __get_user_allowed(x, ptr) \
5cd623333e7cf4 arch/powerpc/include/asm/uaccess.h Christophe Leroy 2020-01-24 99 __get_user_nocheck((x), (ptr), sizeof(*(ptr)), false)
5cd623333e7cf4 arch/powerpc/include/asm/uaccess.h Christophe Leroy 2020-01-24 100 #define __put_user_allowed(x, ptr) \
5cd623333e7cf4 arch/powerpc/include/asm/uaccess.h Christophe Leroy 2020-01-24 101 __put_user_nocheck((__typeof__(*(ptr)))(x), (ptr), sizeof(*(ptr)), false)
e68c825bb01670 include/asm-powerpc/uaccess.h Benjamin Herrenschmidt 2007-04-11 102
e68c825bb01670 include/asm-powerpc/uaccess.h Benjamin Herrenschmidt 2007-04-11 103 #define __get_user_inatomic(x, ptr) \
e68c825bb01670 include/asm-powerpc/uaccess.h Benjamin Herrenschmidt 2007-04-11 104 __get_user_nosleep((x), (ptr), sizeof(*(ptr)))
e68c825bb01670 include/asm-powerpc/uaccess.h Benjamin Herrenschmidt 2007-04-11 105 #define __put_user_inatomic(x, ptr) \
e68c825bb01670 include/asm-powerpc/uaccess.h Benjamin Herrenschmidt 2007-04-11 106 __put_user_nosleep((__typeof__(*(ptr)))(x), (ptr), sizeof(*(ptr)))
e68c825bb01670 include/asm-powerpc/uaccess.h Benjamin Herrenschmidt 2007-04-11 107
2df5e8bcca53e5 include/asm-powerpc/uaccess.h Stephen Rothwell 2005-10-29 108 extern long __put_user_bad(void);
2df5e8bcca53e5 include/asm-powerpc/uaccess.h Stephen Rothwell 2005-10-29 109
2df5e8bcca53e5 include/asm-powerpc/uaccess.h Stephen Rothwell 2005-10-29 110 /*
2df5e8bcca53e5 include/asm-powerpc/uaccess.h Stephen Rothwell 2005-10-29 111 * We don't tell gcc that we are accessing memory, but this is OK
2df5e8bcca53e5 include/asm-powerpc/uaccess.h Stephen Rothwell 2005-10-29 112 * because we do not write to any memory gcc knows about, so there
2df5e8bcca53e5 include/asm-powerpc/uaccess.h Stephen Rothwell 2005-10-29 113 * are no aliasing issues.
2df5e8bcca53e5 include/asm-powerpc/uaccess.h Stephen Rothwell 2005-10-29 114 */
2df5e8bcca53e5 include/asm-powerpc/uaccess.h Stephen Rothwell 2005-10-29 115 #define __put_user_asm(x, addr, err, op) \
2df5e8bcca53e5 include/asm-powerpc/uaccess.h Stephen Rothwell 2005-10-29 116 __asm__ __volatile__( \
2df5e8bcca53e5 include/asm-powerpc/uaccess.h Stephen Rothwell 2005-10-29 117 "1: " op " %1,0(%2) # put_user\n" \
2df5e8bcca53e5 include/asm-powerpc/uaccess.h Stephen Rothwell 2005-10-29 118 "2:\n" \
2df5e8bcca53e5 include/asm-powerpc/uaccess.h Stephen Rothwell 2005-10-29 119 ".section .fixup,\"ax\"\n" \
2df5e8bcca53e5 include/asm-powerpc/uaccess.h Stephen Rothwell 2005-10-29 120 "3: li %0,%3\n" \
2df5e8bcca53e5 include/asm-powerpc/uaccess.h Stephen Rothwell 2005-10-29 121 " b 2b\n" \
2df5e8bcca53e5 include/asm-powerpc/uaccess.h Stephen Rothwell 2005-10-29 122 ".previous\n" \
24bfa6a9e0d4fe arch/powerpc/include/asm/uaccess.h Nicholas Piggin 2016-10-13 123 EX_TABLE(1b, 3b) \
2df5e8bcca53e5 include/asm-powerpc/uaccess.h Stephen Rothwell 2005-10-29 124 : "=r" (err) \
551c3c04b478b9 include/asm-powerpc/uaccess.h Michael Ellerman 2008-07-17 125 : "r" (x), "b" (addr), "i" (-EFAULT), "0" (err))
2df5e8bcca53e5 include/asm-powerpc/uaccess.h Stephen Rothwell 2005-10-29 126
5015b49448cbe5 include/asm-powerpc/uaccess.h Stephen Rothwell 2005-10-31 127 #ifdef __powerpc64__
5015b49448cbe5 include/asm-powerpc/uaccess.h Stephen Rothwell 2005-10-31 128 #define __put_user_asm2(x, ptr, retval) \
5015b49448cbe5 include/asm-powerpc/uaccess.h Stephen Rothwell 2005-10-31 129 __put_user_asm(x, ptr, retval, "std")
5015b49448cbe5 include/asm-powerpc/uaccess.h Stephen Rothwell 2005-10-31 130 #else /* __powerpc64__ */
2df5e8bcca53e5 include/asm-powerpc/uaccess.h Stephen Rothwell 2005-10-29 131 #define __put_user_asm2(x, addr, err) \
2df5e8bcca53e5 include/asm-powerpc/uaccess.h Stephen Rothwell 2005-10-29 132 __asm__ __volatile__( \
2df5e8bcca53e5 include/asm-powerpc/uaccess.h Stephen Rothwell 2005-10-29 133 "1: stw %1,0(%2)\n" \
2df5e8bcca53e5 include/asm-powerpc/uaccess.h Stephen Rothwell 2005-10-29 134 "2: stw %1+1,4(%2)\n" \
2df5e8bcca53e5 include/asm-powerpc/uaccess.h Stephen Rothwell 2005-10-29 135 "3:\n" \
2df5e8bcca53e5 include/asm-powerpc/uaccess.h Stephen Rothwell 2005-10-29 136 ".section .fixup,\"ax\"\n" \
2df5e8bcca53e5 include/asm-powerpc/uaccess.h Stephen Rothwell 2005-10-29 137 "4: li %0,%3\n" \
2df5e8bcca53e5 include/asm-powerpc/uaccess.h Stephen Rothwell 2005-10-29 138 " b 3b\n" \
2df5e8bcca53e5 include/asm-powerpc/uaccess.h Stephen Rothwell 2005-10-29 139 ".previous\n" \
24bfa6a9e0d4fe arch/powerpc/include/asm/uaccess.h Nicholas Piggin 2016-10-13 140 EX_TABLE(1b, 4b) \
24bfa6a9e0d4fe arch/powerpc/include/asm/uaccess.h Nicholas Piggin 2016-10-13 141 EX_TABLE(2b, 4b) \
2df5e8bcca53e5 include/asm-powerpc/uaccess.h Stephen Rothwell 2005-10-29 142 : "=r" (err) \
551c3c04b478b9 include/asm-powerpc/uaccess.h Michael Ellerman 2008-07-17 143 : "r" (x), "b" (addr), "i" (-EFAULT), "0" (err))
2df5e8bcca53e5 include/asm-powerpc/uaccess.h Stephen Rothwell 2005-10-29 144 #endif /* __powerpc64__ */
2df5e8bcca53e5 include/asm-powerpc/uaccess.h Stephen Rothwell 2005-10-29 145
5cd623333e7cf4 arch/powerpc/include/asm/uaccess.h Christophe Leroy 2020-01-24 146 #define __put_user_size_allowed(x, ptr, size, retval) \
2df5e8bcca53e5 include/asm-powerpc/uaccess.h Stephen Rothwell 2005-10-29 147 do { \
2df5e8bcca53e5 include/asm-powerpc/uaccess.h Stephen Rothwell 2005-10-29 148 retval = 0; \
2df5e8bcca53e5 include/asm-powerpc/uaccess.h Stephen Rothwell 2005-10-29 149 switch (size) { \
2df5e8bcca53e5 include/asm-powerpc/uaccess.h Stephen Rothwell 2005-10-29 150 case 1: __put_user_asm(x, ptr, retval, "stb"); break; \
2df5e8bcca53e5 include/asm-powerpc/uaccess.h Stephen Rothwell 2005-10-29 151 case 2: __put_user_asm(x, ptr, retval, "sth"); break; \
2df5e8bcca53e5 include/asm-powerpc/uaccess.h Stephen Rothwell 2005-10-29 152 case 4: __put_user_asm(x, ptr, retval, "stw"); break; \
2df5e8bcca53e5 include/asm-powerpc/uaccess.h Stephen Rothwell 2005-10-29 153 case 8: __put_user_asm2(x, ptr, retval); break; \
2df5e8bcca53e5 include/asm-powerpc/uaccess.h Stephen Rothwell 2005-10-29 154 default: __put_user_bad(); \
2df5e8bcca53e5 include/asm-powerpc/uaccess.h Stephen Rothwell 2005-10-29 155 } \
5cd623333e7cf4 arch/powerpc/include/asm/uaccess.h Christophe Leroy 2020-01-24 156 } while (0)
5cd623333e7cf4 arch/powerpc/include/asm/uaccess.h Christophe Leroy 2020-01-24 157
5cd623333e7cf4 arch/powerpc/include/asm/uaccess.h Christophe Leroy 2020-01-24 158 #define __put_user_size(x, ptr, size, retval) \
5cd623333e7cf4 arch/powerpc/include/asm/uaccess.h Christophe Leroy 2020-01-24 159 do { \
5cd623333e7cf4 arch/powerpc/include/asm/uaccess.h Christophe Leroy 2020-01-24 160 allow_write_to_user(ptr, size); \
5cd623333e7cf4 arch/powerpc/include/asm/uaccess.h Christophe Leroy 2020-01-24 161 __put_user_size_allowed(x, ptr, size, retval); \
de78a9c42a7900 arch/powerpc/include/asm/uaccess.h Christophe Leroy 2019-04-18 162 prevent_write_to_user(ptr, size); \
2df5e8bcca53e5 include/asm-powerpc/uaccess.h Stephen Rothwell 2005-10-29 163 } while (0)
2df5e8bcca53e5 include/asm-powerpc/uaccess.h Stephen Rothwell 2005-10-29 164
5cd623333e7cf4 arch/powerpc/include/asm/uaccess.h Christophe Leroy 2020-01-24 165 #define __put_user_nocheck(x, ptr, size, do_allow) \
2df5e8bcca53e5 include/asm-powerpc/uaccess.h Stephen Rothwell 2005-10-29 166 ({ \
2df5e8bcca53e5 include/asm-powerpc/uaccess.h Stephen Rothwell 2005-10-29 167 long __pu_err; \
6bfd93c32a5065 include/asm-powerpc/uaccess.h Paul Mackerras 2006-05-03 168 __typeof__(*(ptr)) __user *__pu_addr = (ptr); \
fea87fb7a00046 arch/powerpc/include/asm/uaccess.h Nicholas Piggin 2020-04-03 169 __typeof__(*(ptr)) __pu_val = (x); \
fea87fb7a00046 arch/powerpc/include/asm/uaccess.h Nicholas Piggin 2020-04-03 170 __typeof__(size) __pu_size = (size); \
fea87fb7a00046 arch/powerpc/include/asm/uaccess.h Nicholas Piggin 2020-04-03 171 \
6bfd93c32a5065 include/asm-powerpc/uaccess.h Paul Mackerras 2006-05-03 172 if (!is_kernel_addr((unsigned long)__pu_addr)) \
1af1717dbf96eb arch/powerpc/include/asm/uaccess.h Michael S. Tsirkin 2013-05-26 173 might_fault(); \
fea87fb7a00046 arch/powerpc/include/asm/uaccess.h Nicholas Piggin 2020-04-03 174 __chk_user_ptr(__pu_addr); \
5cd623333e7cf4 arch/powerpc/include/asm/uaccess.h Christophe Leroy 2020-01-24 175 if (do_allow) \
fea87fb7a00046 arch/powerpc/include/asm/uaccess.h Nicholas Piggin 2020-04-03 176 __put_user_size(__pu_val, __pu_addr, __pu_size, __pu_err); \
5cd623333e7cf4 arch/powerpc/include/asm/uaccess.h Christophe Leroy 2020-01-24 177 else \
fea87fb7a00046 arch/powerpc/include/asm/uaccess.h Nicholas Piggin 2020-04-03 178 __put_user_size_allowed(__pu_val, __pu_addr, __pu_size, __pu_err); \
fea87fb7a00046 arch/powerpc/include/asm/uaccess.h Nicholas Piggin 2020-04-03 179 \
958106a072021d arch/powerpc/include/asm/uaccess.h Nicholas Piggin 2020-04-03 180 __builtin_expect(__pu_err, 0); \
2df5e8bcca53e5 include/asm-powerpc/uaccess.h Stephen Rothwell 2005-10-29 181 })
2df5e8bcca53e5 include/asm-powerpc/uaccess.h Stephen Rothwell 2005-10-29 182
2df5e8bcca53e5 include/asm-powerpc/uaccess.h Stephen Rothwell 2005-10-29 183 #define __put_user_check(x, ptr, size) \
2df5e8bcca53e5 include/asm-powerpc/uaccess.h Stephen Rothwell 2005-10-29 184 ({ \
2df5e8bcca53e5 include/asm-powerpc/uaccess.h Stephen Rothwell 2005-10-29 185 long __pu_err = -EFAULT; \
2df5e8bcca53e5 include/asm-powerpc/uaccess.h Stephen Rothwell 2005-10-29 186 __typeof__(*(ptr)) __user *__pu_addr = (ptr); \
fea87fb7a00046 arch/powerpc/include/asm/uaccess.h Nicholas Piggin 2020-04-03 187 __typeof__(*(ptr)) __pu_val = (x); \
fea87fb7a00046 arch/powerpc/include/asm/uaccess.h Nicholas Piggin 2020-04-03 188 __typeof__(size) __pu_size = (size); \
fea87fb7a00046 arch/powerpc/include/asm/uaccess.h Nicholas Piggin 2020-04-03 189 \
1af1717dbf96eb arch/powerpc/include/asm/uaccess.h Michael S. Tsirkin 2013-05-26 190 might_fault(); \
fea87fb7a00046 arch/powerpc/include/asm/uaccess.h Nicholas Piggin 2020-04-03 191 if (access_ok(__pu_addr, __pu_size)) \
fea87fb7a00046 arch/powerpc/include/asm/uaccess.h Nicholas Piggin 2020-04-03 192 __put_user_size(__pu_val, __pu_addr, __pu_size, __pu_err); \
fea87fb7a00046 arch/powerpc/include/asm/uaccess.h Nicholas Piggin 2020-04-03 193 \
2df5e8bcca53e5 include/asm-powerpc/uaccess.h Stephen Rothwell 2005-10-29 @194 __pu_err; \
958106a072021d arch/powerpc/include/asm/uaccess.h Nicholas Piggin 2020-04-03 195 __builtin_expect(__pu_err, 0); \
2df5e8bcca53e5 include/asm-powerpc/uaccess.h Stephen Rothwell 2005-10-29 196 })
2df5e8bcca53e5 include/asm-powerpc/uaccess.h Stephen Rothwell 2005-10-29 197
:::::: The code at line 89 was first introduced by commit
:::::: 2df5e8bcca53e528a78ee0e3b114d0d21dd6d043 powerpc: merge uaccess.h
:::::: TO: Stephen Rothwell <sfr@canb.auug.org.au>
:::::: CC: Stephen Rothwell <sfr@canb.auug.org.au>
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 15339 bytes --]
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v2 1/4] powerpc/64s: implement probe_kernel_read/write without touching AMR
2020-04-03 11:05 ` Nicholas Piggin
@ 2020-04-07 4:01 ` Nicholas Piggin
2020-04-07 9:09 ` Daniel Borkmann
0 siblings, 1 reply; 12+ messages in thread
From: Nicholas Piggin @ 2020-04-07 4:01 UTC (permalink / raw)
To: Christophe Leroy, linuxppc-dev
Cc: linux-arch, Daniel Borkmann, x86, Alexei Starovoitov,
linux-kernel, Masami Hiramatsu, Linus Torvalds
Nicholas Piggin's on April 3, 2020 9:05 pm:
> Christophe Leroy's on April 3, 2020 8:31 pm:
>>
>>
>> Le 03/04/2020 à 11:35, Nicholas Piggin a écrit :
>>> There is no need to allow user accesses when probing kernel addresses.
>>
>> I just discovered the following commit
>> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=75a1a607bb7e6d918be3aca11ec2214a275392f4
>>
>> This commit adds probe_kernel_read_strict() and probe_kernel_write_strict().
>>
>> When reading the commit log, I understand that probe_kernel_read() may
>> be used to access some user memory. Which will not work anymore with
>> your patch.
>
> Hmm, I looked at _strict but obviously not hard enough. Good catch.
>
> I don't think probe_kernel_read() should ever access user memory,
> the comment certainly says it doesn't, but that patch sort of implies
> that they do.
>
> I think it's wrong. The non-_strict maybe could return userspace data to
> you if you did pass a user address? I don't see why that shouldn't just
> be disallowed always though.
>
> And if the _strict version is required to be safe, then it seems like a
> bug or security issue to just allow everyone that doesn't explicitly
> override it to use the default implementation.
>
> Also, the way the weak linkage is done in that patch, means parisc and
> um archs that were previously overriding probe_kernel_read() now get
> the default probe_kernel_read_strict(), which would be wrong for them.
The changelog in commit 75a1a607bb7 makes it a bit clearer. If the
non-_strict variant is used on non-kernel addresses, then it might not
return -EFAULT or it might cause a kernel warning. The _strict variant
is supposed to be usable with any address and it will return -EFAULT if
it was not a valid and mapped kernel address.
The non-_strict variant can not portably access user memory because it
uses KERNEL_DS, and its documentation says its only for kernel pointers.
So powerpc should be fine to run that under KUAP AFAIKS.
I don't know why the _strict behaviour is not just made default, but
the implementation of it does seem to be broken on the archs that
override the non-_strict variant.
Thanks,
Nick
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v2 1/4] powerpc/64s: implement probe_kernel_read/write without touching AMR
2020-04-07 4:01 ` Nicholas Piggin
@ 2020-04-07 9:09 ` Daniel Borkmann
0 siblings, 0 replies; 12+ messages in thread
From: Daniel Borkmann @ 2020-04-07 9:09 UTC (permalink / raw)
To: Nicholas Piggin, Christophe Leroy, linuxppc-dev
Cc: linux-arch, x86, Alexei Starovoitov, linux-kernel,
Masami Hiramatsu, Linus Torvalds
Hey Nicholas,
On 4/7/20 6:01 AM, Nicholas Piggin wrote:
> Nicholas Piggin's on April 3, 2020 9:05 pm:
>> Christophe Leroy's on April 3, 2020 8:31 pm:
>>> Le 03/04/2020 à 11:35, Nicholas Piggin a écrit :
>>>> There is no need to allow user accesses when probing kernel addresses.
>>>
>>> I just discovered the following commit
>>> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=75a1a607bb7e6d918be3aca11ec2214a275392f4
>>>
>>> This commit adds probe_kernel_read_strict() and probe_kernel_write_strict().
>>>
>>> When reading the commit log, I understand that probe_kernel_read() may
>>> be used to access some user memory. Which will not work anymore with
>>> your patch.
>>
>> Hmm, I looked at _strict but obviously not hard enough. Good catch.
>>
>> I don't think probe_kernel_read() should ever access user memory,
>> the comment certainly says it doesn't, but that patch sort of implies
>> that they do.
>>
>> I think it's wrong. The non-_strict maybe could return userspace data to
>> you if you did pass a user address? I don't see why that shouldn't just
>> be disallowed always though.
>>
>> And if the _strict version is required to be safe, then it seems like a
>> bug or security issue to just allow everyone that doesn't explicitly
>> override it to use the default implementation.
>>
>> Also, the way the weak linkage is done in that patch, means parisc and
>> um archs that were previously overriding probe_kernel_read() now get
>> the default probe_kernel_read_strict(), which would be wrong for them.
>
> The changelog in commit 75a1a607bb7 makes it a bit clearer. If the
> non-_strict variant is used on non-kernel addresses, then it might not
> return -EFAULT or it might cause a kernel warning. The _strict variant
> is supposed to be usable with any address and it will return -EFAULT if
> it was not a valid and mapped kernel address.
>
> The non-_strict variant can not portably access user memory because it
> uses KERNEL_DS, and its documentation says its only for kernel pointers.
> So powerpc should be fine to run that under KUAP AFAIKS.
>
> I don't know why the _strict behaviour is not just made default, but
> the implementation of it does seem to be broken on the archs that
> override the non-_strict variant.
Yeah, we should make it default and only add a "opt out" for the old legacy
cases; there was also same discussion started over here just recently [0].
Thanks,
Daniel
[0] https://lore.kernel.org/lkml/20200403133533.GA3424@infradead.org/T/
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v2 1/4] powerpc/64s: implement probe_kernel_read/write without touching AMR
2020-04-03 9:35 [PATCH v2 1/4] powerpc/64s: implement probe_kernel_read/write without touching AMR Nicholas Piggin
` (3 preceding siblings ...)
2020-04-03 10:31 ` [PATCH v2 1/4] powerpc/64s: implement probe_kernel_read/write without touching AMR Christophe Leroy
@ 2020-06-10 12:41 ` Christophe Leroy
2020-06-14 9:28 ` Nicholas Piggin
4 siblings, 1 reply; 12+ messages in thread
From: Christophe Leroy @ 2020-06-10 12:41 UTC (permalink / raw)
To: Nicholas Piggin, linuxppc-dev
Hi Nick
Le 03/04/2020 à 11:35, Nicholas Piggin a écrit :
> There is no need to allow user accesses when probing kernel addresses.
You should have a look at
https://github.com/torvalds/linux/commit/fa94111d94354de76c47fea6e1187d1ee91e23a7
At seems to implement a generic way of achieving what you are trying to
do here.
Christophe
>
> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
> ---
> v2:
> - Enable for all powerpc (suggested by Christophe)
> - Fold helper function together (Christophe)
> - Rename uaccess.c to maccess.c to match kernel/maccess.c.
>
> arch/powerpc/include/asm/uaccess.h | 25 +++++++++++++++-------
> arch/powerpc/lib/Makefile | 2 +-
> arch/powerpc/lib/maccess.c | 34 ++++++++++++++++++++++++++++++
> 3 files changed, 52 insertions(+), 9 deletions(-)
> create mode 100644 arch/powerpc/lib/maccess.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..77af10ad0b0d 100644
> --- a/arch/powerpc/lib/Makefile
> +++ b/arch/powerpc/lib/Makefile
> @@ -16,7 +16,7 @@ CFLAGS_code-patching.o += -DDISABLE_BRANCH_PROFILING
> CFLAGS_feature-fixups.o += -DDISABLE_BRANCH_PROFILING
> endif
>
> -obj-y += alloc.o code-patching.o feature-fixups.o pmem.o
> +obj-y += alloc.o code-patching.o feature-fixups.o pmem.o maccess.o
>
> ifndef CONFIG_KASAN
> obj-y += string.o memcmp_$(BITS).o
> diff --git a/arch/powerpc/lib/maccess.c b/arch/powerpc/lib/maccess.c
> new file mode 100644
> index 000000000000..ce5465db1e2d
> --- /dev/null
> +++ b/arch/powerpc/lib/maccess.c
> @@ -0,0 +1,34 @@
> +#include <linux/mm.h>
> +#include <linux/uaccess.h>
> +
> +/*
> + * Override the generic weak linkage functions to avoid changing KUP state via
> + * the generic user access functions, as this is accessing kernel addresses.
> + */
> +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);
> + pagefault_disable();
> + ret = raw_copy_from_user_allowed(dst, (__force const void __user *)src, size);
> + pagefault_enable();
> + set_fs(old_fs);
> +
> + return ret ? -EFAULT : 0;
> +}
> +
> +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);
> + pagefault_disable();
> + ret = raw_copy_to_user_allowed((__force void __user *)dst, src, size);
> + pagefault_enable();
> + set_fs(old_fs);
> +
> + return ret ? -EFAULT : 0;
> +}
>
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v2 1/4] powerpc/64s: implement probe_kernel_read/write without touching AMR
2020-06-10 12:41 ` Christophe Leroy
@ 2020-06-14 9:28 ` Nicholas Piggin
0 siblings, 0 replies; 12+ messages in thread
From: Nicholas Piggin @ 2020-06-14 9:28 UTC (permalink / raw)
To: Christophe Leroy, linuxppc-dev
Excerpts from Christophe Leroy's message of June 10, 2020 10:41 pm:
> Hi Nick
>
> Le 03/04/2020 à 11:35, Nicholas Piggin a écrit :
>> There is no need to allow user accesses when probing kernel addresses.
>
> You should have a look at
> https://github.com/torvalds/linux/commit/fa94111d94354de76c47fea6e1187d1ee91e23a7
>
> At seems to implement a generic way of achieving what you are trying to
> do here.
Yep thanks for that, I'll rebase this series on upstream now.
Thanks,
Nick
^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2020-06-14 9:29 UTC | newest]
Thread overview: 12+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2020-04-03 9:35 [PATCH v2 1/4] powerpc/64s: implement probe_kernel_read/write without touching AMR Nicholas Piggin
2020-04-03 9:35 ` [PATCH v2 2/4] powerpc/64s: use mmu_has_feature in set_kuap() and get_kuap() Nicholas Piggin
2020-04-03 9:35 ` [PATCH v2 3/4] powerpc/uaccess: evaluate macro arguments once, before user access is allowed Nicholas Piggin
2020-04-03 9:35 ` [PATCH v2 4/4] powerpc/uaccess: add more __builtin_expect annotations Nicholas Piggin
2020-04-03 10:35 ` Nicholas Piggin
2020-04-04 14:56 ` kbuild test robot
2020-04-03 10:31 ` [PATCH v2 1/4] powerpc/64s: implement probe_kernel_read/write without touching AMR Christophe Leroy
2020-04-03 11:05 ` Nicholas Piggin
2020-04-07 4:01 ` Nicholas Piggin
2020-04-07 9:09 ` Daniel Borkmann
2020-06-10 12:41 ` Christophe Leroy
2020-06-14 9:28 ` Nicholas Piggin
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).