Re: [PATCH 07/14] uaccess: generalize access_ok()
On Mon, Feb 14, 2022 at 05:34:45PM +0100, Arnd Bergmann wrote:
> From: Arnd Bergmann
>
> There are many different ways that access_ok() is defined across
> architectures, but in the end, they all just compare against the
> user_addr_max() value or they accept anything.
>
> Provide one definition that works for most architectures, checking
> against TASK_SIZE_MAX for user processes or skipping the check inside
> of uaccess_kernel() sections.
>
> For architectures without CONFIG_SET_FS(), this should be the fastest
> check, as it comes down to a single comparison of a pointer against a
> compile-time constant, while the architecture specific versions tend to
> do something more complex for historic reasons or get something wrong.
>
> Type checking for __user annotations is handled inconsistently across
> architectures, but this is easily simplified as well by using an inline
> function that takes a 'const void __user *' argument. A handful of
> callers need an extra __user annotation for this.
>
> Some architectures had trick to use 33-bit or 65-bit arithmetic on the
> addresses to calculate the overflow, however this simpler version uses
> fewer registers, which means it can produce better object code in the
> end despite needing a second (statically predicted) branch.
>
> Signed-off-by: Arnd Bergmann
As discussed over IRC, the generic sequence looks good to me, and likewise for
the arm64 change, so:
Acked-by: Mark Rutland [arm64, asm-generic]
Thanks,
Mark.
> ---
> arch/alpha/include/asm/uaccess.h | 34 +++
> arch/arc/include/asm/uaccess.h| 29 -
> arch/arm/include/asm/uaccess.h| 20 +
> arch/arm/kernel/swp_emulate.c | 2 +-
> arch/arm/kernel/traps.c | 2 +-
> arch/arm64/include/asm/uaccess.h | 5 ++-
> arch/csky/include/asm/uaccess.h | 8
> arch/csky/kernel/signal.c | 2 +-
> arch/hexagon/include/asm/uaccess.h| 25
> arch/ia64/include/asm/uaccess.h | 5 +--
> arch/m68k/include/asm/uaccess.h | 5 ++-
> arch/microblaze/include/asm/uaccess.h | 8 +---
> arch/mips/include/asm/uaccess.h | 29 +
> arch/nds32/include/asm/uaccess.h | 7 +---
> arch/nios2/include/asm/uaccess.h | 11 +
> arch/nios2/kernel/signal.c| 20 +
> arch/openrisc/include/asm/uaccess.h | 19 +
> arch/parisc/include/asm/uaccess.h | 10 +++--
> arch/powerpc/include/asm/uaccess.h| 11 +
> arch/powerpc/lib/sstep.c | 4 +-
> arch/riscv/include/asm/uaccess.h | 31 +-
> arch/riscv/kernel/perf_callchain.c| 2 +-
> arch/s390/include/asm/uaccess.h | 11 ++---
> arch/sh/include/asm/uaccess.h | 22 +-
> arch/sparc/include/asm/uaccess.h | 3 --
> arch/sparc/include/asm/uaccess_32.h | 18 ++--
> arch/sparc/include/asm/uaccess_64.h | 35
> arch/sparc/kernel/signal_32.c | 2 +-
> arch/um/include/asm/uaccess.h | 5 ++-
> arch/x86/include/asm/uaccess.h| 14 +--
> arch/xtensa/include/asm/uaccess.h | 10 +
> include/asm-generic/access_ok.h | 59 +++
> include/asm-generic/uaccess.h | 21 +-
> include/linux/uaccess.h | 7
> 34 files changed, 130 insertions(+), 366 deletions(-)
> create mode 100644 include/asm-generic/access_ok.h
>
> diff --git a/arch/alpha/include/asm/uaccess.h
> b/arch/alpha/include/asm/uaccess.h
> index 1b6f25efa247..82c5743fc9cd 100644
> --- a/arch/alpha/include/asm/uaccess.h
> +++ b/arch/alpha/include/asm/uaccess.h
> @@ -20,28 +20,7 @@
> #define get_fs() (current_thread_info()->addr_limit)
> #define set_fs(x) (current_thread_info()->addr_limit = (x))
>
> -#define uaccess_kernel() (get_fs().seg == KERNEL_DS.seg)
> -
> -/*
> - * Is a address valid? This does a straightforward calculation rather
> - * than tests.
> - *
> - * Address valid if:
> - * - "addr" doesn't have any high-bits set
> - * - AND "size" doesn't have any high-bits set
> - * - AND "addr+size-(size != 0)" doesn't have any high-bits set
> - * - OR we are in kernel mode.
> - */
> -#define __access_ok(addr, size) ({ \
> - unsigned long __ao_a = (addr), __ao_b = (size); \
> - unsigned long __ao_end = __ao_a + __ao_b - !!__ao_b;\
> - (get_fs().seg & (__ao_a | __ao_b | __ao_end)) == 0; })
> -
> -#define access_ok(addr, size)\
> -({ \
> - __chk_user_ptr(addr); \
> - __access_ok(((unsigned long)(addr)), (size)); \
> -})
> +#include
>
> /*
> * These are the main single-value transfer routines. They automatically
> @@ -105,7 +84,7 @@ extern void __get_user_unknown(void);
> long __gu_err = -EFAULT;\
> unsigned long __gu_val = 0;
Re: [PATCH 07/14] uaccess: generalize access_ok()
On Mon, Feb 14, 2022 at 6:15 PM Al Viro wrote:
>
> On Mon, Feb 14, 2022 at 05:34:45PM +0100, Arnd Bergmann wrote:
>
> > diff --git a/arch/csky/kernel/signal.c b/arch/csky/kernel/signal.c
> > index c7b763d2f526..8867ddf3e6c7 100644
> > --- a/arch/csky/kernel/signal.c
> > +++ b/arch/csky/kernel/signal.c
> > @@ -136,7 +136,7 @@ static inline void __user *get_sigframe(struct ksignal
> > *ksig,
> > static int
> > setup_rt_frame(struct ksignal *ksig, sigset_t *set, struct pt_regs *regs)
> > {
> > - struct rt_sigframe *frame;
> > + struct rt_sigframe __user *frame;
> > int err = 0;
> >
> > frame = get_sigframe(ksig, regs, sizeof(*frame));
>
> Minor nit: might make sense to separate annotations (here, on nios2, etc.)
> from the rest...
Done.
> > -}
> > -
> > -static inline int access_ok(const void __user * addr, unsigned long size)
> > -{
> > - return 1;
> > -}
> > +#define __range_not_ok(addr, size, limit) (!__access_ok(addr, size))
>
> is really wrong. For sparc64, access_ok() should always be true.
> This __range_not_ok() thing is used *only* for valid_user_frame() in
> arch/sparc/kernel/perf_event.c - it's not a part of normal access_ok()
> there.
>
> sparc64 has separate address spaces for kernel and for userland; access_ok()
> had never been useful there.
Ok, fixed as well now. I had the access_ok() bit right, the definition just
moved around here so it comes before the #include, but I missed the
bit about __range_not_ok(), which I have now reverted back to the
correct version in my tree.
Arnd
Re: [PATCH 07/14] uaccess: generalize access_ok()
On Mon, Feb 14, 2022 at 05:34:45PM +0100, Arnd Bergmann wrote:
> diff --git a/arch/csky/kernel/signal.c b/arch/csky/kernel/signal.c
> index c7b763d2f526..8867ddf3e6c7 100644
> --- a/arch/csky/kernel/signal.c
> +++ b/arch/csky/kernel/signal.c
> @@ -136,7 +136,7 @@ static inline void __user *get_sigframe(struct ksignal
> *ksig,
> static int
> setup_rt_frame(struct ksignal *ksig, sigset_t *set, struct pt_regs *regs)
> {
> - struct rt_sigframe *frame;
> + struct rt_sigframe __user *frame;
> int err = 0;
>
> frame = get_sigframe(ksig, regs, sizeof(*frame));
Minor nit: might make sense to separate annotations (here, on nios2, etc.) from
the rest...
This, OTOH,
> diff --git a/arch/sparc/include/asm/uaccess_64.h
> b/arch/sparc/include/asm/uaccess_64.h
> index 5c12fb46bc61..000bac67cf31 100644
> --- a/arch/sparc/include/asm/uaccess_64.h
> +++ b/arch/sparc/include/asm/uaccess_64.h
...
> -static inline bool __chk_range_not_ok(unsigned long addr, unsigned long
> size, unsigned long limit)
> -{
> - if (__builtin_constant_p(size))
> - return addr > limit - size;
> -
> - addr += size;
> - if (addr < size)
> - return true;
> -
> - return addr > limit;
> -}
> -
> -#define __range_not_ok(addr, size, limit) \
> -({ \
> - __chk_user_ptr(addr); \
> - __chk_range_not_ok((unsigned long __force)(addr), size, limit); \
> -})
> -
> -static inline int __access_ok(const void __user * addr, unsigned long size)
> -{
> - return 1;
> -}
> -
> -static inline int access_ok(const void __user * addr, unsigned long size)
> -{
> - return 1;
> -}
> +#define __range_not_ok(addr, size, limit) (!__access_ok(addr, size))
is really wrong. For sparc64, access_ok() should always be true.
This __range_not_ok() thing is used *only* for valid_user_frame() in
arch/sparc/kernel/perf_event.c - it's not a part of normal access_ok()
there.
sparc64 has separate address spaces for kernel and for userland; access_ok()
had never been useful there.
Re: [PATCH 07/14] uaccess: generalize access_ok()
Looks good, Reviewed-by: Christoph Hellwig
[PATCH 07/14] uaccess: generalize access_ok()
From: Arnd Bergmann
There are many different ways that access_ok() is defined across
architectures, but in the end, they all just compare against the
user_addr_max() value or they accept anything.
Provide one definition that works for most architectures, checking
against TASK_SIZE_MAX for user processes or skipping the check inside
of uaccess_kernel() sections.
For architectures without CONFIG_SET_FS(), this should be the fastest
check, as it comes down to a single comparison of a pointer against a
compile-time constant, while the architecture specific versions tend to
do something more complex for historic reasons or get something wrong.
Type checking for __user annotations is handled inconsistently across
architectures, but this is easily simplified as well by using an inline
function that takes a 'const void __user *' argument. A handful of
callers need an extra __user annotation for this.
Some architectures had trick to use 33-bit or 65-bit arithmetic on the
addresses to calculate the overflow, however this simpler version uses
fewer registers, which means it can produce better object code in the
end despite needing a second (statically predicted) branch.
Signed-off-by: Arnd Bergmann
---
arch/alpha/include/asm/uaccess.h | 34 +++
arch/arc/include/asm/uaccess.h| 29 -
arch/arm/include/asm/uaccess.h| 20 +
arch/arm/kernel/swp_emulate.c | 2 +-
arch/arm/kernel/traps.c | 2 +-
arch/arm64/include/asm/uaccess.h | 5 ++-
arch/csky/include/asm/uaccess.h | 8
arch/csky/kernel/signal.c | 2 +-
arch/hexagon/include/asm/uaccess.h| 25
arch/ia64/include/asm/uaccess.h | 5 +--
arch/m68k/include/asm/uaccess.h | 5 ++-
arch/microblaze/include/asm/uaccess.h | 8 +---
arch/mips/include/asm/uaccess.h | 29 +
arch/nds32/include/asm/uaccess.h | 7 +---
arch/nios2/include/asm/uaccess.h | 11 +
arch/nios2/kernel/signal.c| 20 +
arch/openrisc/include/asm/uaccess.h | 19 +
arch/parisc/include/asm/uaccess.h | 10 +++--
arch/powerpc/include/asm/uaccess.h| 11 +
arch/powerpc/lib/sstep.c | 4 +-
arch/riscv/include/asm/uaccess.h | 31 +-
arch/riscv/kernel/perf_callchain.c| 2 +-
arch/s390/include/asm/uaccess.h | 11 ++---
arch/sh/include/asm/uaccess.h | 22 +-
arch/sparc/include/asm/uaccess.h | 3 --
arch/sparc/include/asm/uaccess_32.h | 18 ++--
arch/sparc/include/asm/uaccess_64.h | 35
arch/sparc/kernel/signal_32.c | 2 +-
arch/um/include/asm/uaccess.h | 5 ++-
arch/x86/include/asm/uaccess.h| 14 +--
arch/xtensa/include/asm/uaccess.h | 10 +
include/asm-generic/access_ok.h | 59 +++
include/asm-generic/uaccess.h | 21 +-
include/linux/uaccess.h | 7
34 files changed, 130 insertions(+), 366 deletions(-)
create mode 100644 include/asm-generic/access_ok.h
diff --git a/arch/alpha/include/asm/uaccess.h b/arch/alpha/include/asm/uaccess.h
index 1b6f25efa247..82c5743fc9cd 100644
--- a/arch/alpha/include/asm/uaccess.h
+++ b/arch/alpha/include/asm/uaccess.h
@@ -20,28 +20,7 @@
#define get_fs() (current_thread_info()->addr_limit)
#define set_fs(x) (current_thread_info()->addr_limit = (x))
-#define uaccess_kernel() (get_fs().seg == KERNEL_DS.seg)
-
-/*
- * Is a address valid? This does a straightforward calculation rather
- * than tests.
- *
- * Address valid if:
- * - "addr" doesn't have any high-bits set
- * - AND "size" doesn't have any high-bits set
- * - AND "addr+size-(size != 0)" doesn't have any high-bits set
- * - OR we are in kernel mode.
- */
-#define __access_ok(addr, size) ({ \
- unsigned long __ao_a = (addr), __ao_b = (size); \
- unsigned long __ao_end = __ao_a + __ao_b - !!__ao_b;\
- (get_fs().seg & (__ao_a | __ao_b | __ao_end)) == 0; })
-
-#define access_ok(addr, size) \
-({ \
- __chk_user_ptr(addr); \
- __access_ok(((unsigned long)(addr)), (size)); \
-})
+#include
/*
* These are the main single-value transfer routines. They automatically
@@ -105,7 +84,7 @@ extern void __get_user_unknown(void);
long __gu_err = -EFAULT;\
unsigned long __gu_val = 0; \
const __typeof__(*(ptr)) __user *__gu_addr = (ptr); \
- if (__access_ok((unsigned long)__gu_addr, size)) { \
+ if (__access_ok(__gu_addr, size)) { \
__gu_err = 0; \
switch (size) { \
case 1: __get_user_8(__gu_addr); break; \
@@ -200,7
