Re: [PATCH v2 13/18] uaccess: generalize access_ok()
On Wed, Feb 16, 2022 at 02:13:27PM +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.
>
> Reviewed-by: Christoph Hellwig
> Acked-by: Mark Rutland [arm64, asm-generic]
> Signed-off-by: Arnd Bergmann
> ---
...
> arch/openrisc/include/asm/uaccess.h | 19 +
...
> include/asm-generic/access_ok.h | 59 +++
> include/asm-generic/uaccess.h | 21 +-
> include/linux/uaccess.h | 7
> 32 files changed, 109 insertions(+), 366 deletions(-)
>
...
> diff --git a/arch/openrisc/include/asm/uaccess.h
> b/arch/openrisc/include/asm/uaccess.h
> index 120f5005461b..8f049ec99b3e 100644
> --- a/arch/openrisc/include/asm/uaccess.h
> +++ b/arch/openrisc/include/asm/uaccess.h
> @@ -45,21 +45,7 @@
>
> #define uaccess_kernel() (get_fs() == KERNEL_DS)
>
> -/* Ensure that the range from addr to addr+size is all within the process'
> - * address space
> - */
> -static inline int __range_ok(unsigned long addr, unsigned long size)
> -{
> - const mm_segment_t fs = get_fs();
> -
> - return size <= fs && addr <= (fs - size);
> -}
> -
> -#define access_ok(addr, size)
> \
> -({ \
> - __chk_user_ptr(addr); \
> - __range_ok((unsigned long)(addr), (size)); \
> -})
> +#include
I was going to ask why we are missing __chk_user_ptr in the generic version.
But this is basically now a no-op so I think its OK.
> /*
> * These are the main single-value transfer routines. They automatically
> @@ -268,9 +254,6 @@ clear_user(void __user *addr, unsigned long size)
> return size;
> }
>
> -#define user_addr_max() \
> - (uaccess_kernel() ? ~0UL : TASK_SIZE)
> -
> extern long strncpy_from_user(char *dest, const char __user *src, long
> count);
>
> extern __must_check long strnlen_user(const char __user *str, long n);
...
> diff --git a/include/asm-generic/access_ok.h b/include/asm-generic/access_ok.h
> new file mode 100644
> index ..1aad8964d2ed
> --- /dev/null
> +++ b/include/asm-generic/access_ok.h
> @@ -0,0 +1,59 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +#ifndef __ASM_GENERIC_ACCESS_OK_H__
> +#define __ASM_GENERIC_ACCESS_OK_H__
> +
> +/*
> + * Checking whether a pointer is valid for user space access.
> + * These definitions work on most architectures, but overrides can
> + * be used where necessary.
> + */
> +
> +/*
> + * architectures with compat tasks have a variable TASK_SIZE and should
> + * override this to a constant.
> + */
> +#ifndef TASK_SIZE_MAX
> +#define TASK_SIZE_MAXTASK_SIZE
> +#endif
> +
> +#ifndef uaccess_kernel
> +#ifdef CONFIG_SET_FS
> +#define uaccess_kernel() (get_fs().seg == KERNEL_DS.seg)
> +#else
> +#define uaccess_kernel() (0)
> +#endif
> +#endif
> +
> +#ifndef user_addr_max
> +#define user_addr_max() (uaccess_kernel() ? ~0UL :
> TASK_SIZE_MAX)
> +#endif
> +
> +#ifndef __access_ok
> +/*
> + * 'size' is a compile-time constant for most callers, so optimize for
> + * this case to turn the check into a single comparison against a constant
> + * limit and catch all possible overflows.
> + * On architectures with separate user address space (m68k, s390, parisc,
> + * sparc64) or those without an MMU, this should always return true.
> + *
> + * This version was originally contributed by Jonas Bonn for the
> + * OpenRISC architecture, and was found to be the most efficient
> + * for constant 'size' and 'limit' values.
> + */
> +static inline int __access_ok(const void __user *ptr, unsigned long size)
> +{
> + unsigned long limit = user_addr_max();
> + unsigned
Re: [PATCH v2 13/18] uaccess: generalize access_ok()
On Thu, Feb 24, 2022 at 9:29 AM Stafford Horne wrote:
> > -
> > -#define access_ok(addr, size)
> > \
> > -({ \
> > - __chk_user_ptr(addr); \
> > - __range_ok((unsigned long)(addr), (size)); \
> > -})
> > +#include
>
> I was going to ask why we are missing __chk_user_ptr in the generic version.
> But this is basically now a no-op so I think its OK.
Correct, the type checking is implied by making __access_ok() an inline
function that takes a __user pointer.
> Acked-by: Stafford Horne [openrisc, asm-generic]
Thanks!
Arnd
___
linux-snps-arc mailing list
linux-snps-arc@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-snps-arc
Re: [PATCH v2 18/18] uaccess: drop maining CONFIG_SET_FS users
On Wed, Feb 16, 2022 at 02:13:32PM +0100, Arnd Bergmann wrote:
> From: Arnd Bergmann
>
> There are no remaining callers of set_fs(), so CONFIG_SET_FS
> can be removed globally, along with the thread_info field and
> any references to it.
>
> This turns access_ok() into a cheaper check against TASK_SIZE_MAX.
>
> With CONFIG_SET_FS gone, so drop all remaining references to
> set_fs()/get_fs(), mm_segment_t and uaccess_kernel().
>
> Signed-off-by: Arnd Bergmann
> ---
...
> arch/openrisc/Kconfig | 1 -
> arch/openrisc/include/asm/thread_info.h | 7 ---
> arch/openrisc/include/asm/uaccess.h | 23
...
> fs/exec.c | 6 --
> include/asm-generic/access_ok.h | 10 +---
> include/asm-generic/uaccess.h | 25 +---
> include/linux/syscalls.h | 4 --
> include/linux/uaccess.h | 33 ---
> include/rdma/ib.h | 2 +-
> kernel/events/callchain.c | 4 --
> kernel/events/core.c | 3 -
> kernel/exit.c | 14 -
> kernel/kthread.c | 5 --
> kernel/stacktrace.c | 3 -
> kernel/trace/bpf_trace.c | 4 --
> mm/maccess.c | 11
> mm/memory.c | 8 ---
> net/bpfilter/bpfilter_kern.c | 2 +-
> 72 files changed, 10 insertions(+), 522 deletions(-)
> delete mode 100644 arch/arc/include/asm/segment.h
> delete mode 100644 arch/csky/include/asm/segment.h
> delete mode 100644 arch/h8300/include/asm/segment.h
>
> diff --git a/arch/Kconfig b/arch/Kconfig
> index fa5db36bda67..99349547afed 100644
> --- a/arch/Kconfig
> +++ b/arch/Kconfig
> @@ -24,9 +24,6 @@ config KEXEC_ELF
> config HAVE_IMA_KEXEC
> bool
>
> -config SET_FS
> - bool
> -
> config HOTPLUG_SMT
> bool
>
...
> diff --git a/arch/openrisc/Kconfig b/arch/openrisc/Kconfig
> index f724b3f1aeed..0d68adf6e02b 100644
> --- a/arch/openrisc/Kconfig
> +++ b/arch/openrisc/Kconfig
> @@ -36,7 +36,6 @@ config OPENRISC
> select ARCH_WANT_FRAME_POINTERS
> select GENERIC_IRQ_MULTI_HANDLER
> select MMU_GATHER_NO_RANGE if MMU
> - select SET_FS
> select TRACE_IRQFLAGS_SUPPORT
>
> config CPU_BIG_ENDIAN
> diff --git a/arch/openrisc/include/asm/thread_info.h
> b/arch/openrisc/include/asm/thread_info.h
> index 659834ab87fa..4af3049c34c2 100644
> --- a/arch/openrisc/include/asm/thread_info.h
> +++ b/arch/openrisc/include/asm/thread_info.h
> @@ -40,18 +40,12 @@
> */
> #ifndef __ASSEMBLY__
>
> -typedef unsigned long mm_segment_t;
> -
> struct thread_info {
> struct task_struct *task; /* main task structure */
> unsigned long flags; /* low level flags */
> __u32 cpu;/* current CPU */
> __s32 preempt_count; /* 0 => preemptable, <0 => BUG */
>
> - mm_segment_taddr_limit; /* thread address space:
> -0-0x7FFF for user-thead
> -0-0x for kernel-thread
> - */
> __u8supervisor_stack[0];
>
> /* saved context data */
> @@ -71,7 +65,6 @@ struct thread_info {
> .flags = 0,\
> .cpu= 0,\
> .preempt_count = INIT_PREEMPT_COUNT, \
> - .addr_limit = KERNEL_DS,\
> .ksp= 0,\
> }
>
> diff --git a/arch/openrisc/include/asm/uaccess.h
> b/arch/openrisc/include/asm/uaccess.h
> index 8f049ec99b3e..d6500a374e18 100644
> --- a/arch/openrisc/include/asm/uaccess.h
> +++ b/arch/openrisc/include/asm/uaccess.h
> @@ -22,29 +22,6 @@
> #include
> #include
> #include
> -
> -/*
> - * The fs value determines whether argument validity checking should be
> - * performed or not. If get_fs() == USER_DS, checking is performed, with
> - * get_fs() == KERNEL_DS, checking is bypassed.
> - *
> - * For historical reasons, these macros are grossly misnamed.
> - */
> -
> -/* addr_limit is the maximum accessible address for the task. we misuse
> - * the KERNEL_DS and USER_DS values to both assign and compare the
> - * addr_limit values through the equally misnamed get/set_fs macros.
> - * (see above)
> - */
> -
> -#define KERNEL_DS(~0UL)
> -
> -#define USER_DS (TASK_SIZE)
> -#define get_fs() (current_thread_info()->addr_limit)
> -#define set_fs(x)(current_thread_info()->addr_limit = (x))
> -
> -#define uaccess_kernel() (get_fs() == KERNEL_DS)
> -
> #include
>
> /*
...
> diff --git a/fs/exec.c b/fs/exec.c
> index 79f2c9483302..bc68a0c089ac 100644
> --- a/fs/exec.c
> +++ b/fs/exec.c
> @@ -1303,12 +1303,6 @@ int
Re: [PATCH v2 02/18] uaccess: fix nios2 and microblaze get_user_8()
On 2/16/22 07:13, Arnd Bergmann wrote:
From: Arnd Bergmann
These two architectures implement 8-byte get_user() through
a memcpy() into a four-byte variable, which won't fit.
Use a temporary 64-bit variable instead here, and use a double
cast the way that risc-v and openrisc do to avoid compile-time
warnings.
Fixes: 6a090e97972d ("arch/microblaze: support get_user() of size 8 bytes")
Fixes: 5ccc6af5e88e ("nios2: Memory management")
Signed-off-by: Arnd Bergmann
---
arch/microblaze/include/asm/uaccess.h | 18 +-
arch/nios2/include/asm/uaccess.h | 26 --
2 files changed, 25 insertions(+), 19 deletions(-)
diff --git a/arch/microblaze/include/asm/uaccess.h
b/arch/microblaze/include/asm/uaccess.h
index 5b6e0e7788f4..3fe96979d2c6 100644
--- a/arch/microblaze/include/asm/uaccess.h
+++ b/arch/microblaze/include/asm/uaccess.h
@@ -130,27 +130,27 @@ extern long __user_bad(void);
#define __get_user(x, ptr) \
({\
- unsigned long __gu_val = 0; \
long __gu_err; \
switch (sizeof(*(ptr))) { \
case 1: \
- __get_user_asm("lbu", (ptr), __gu_val, __gu_err); \
+ __get_user_asm("lbu", (ptr), x, __gu_err);\
break; \
case 2: \
- __get_user_asm("lhu", (ptr), __gu_val, __gu_err); \
+ __get_user_asm("lhu", (ptr), x, __gu_err);\
break; \
case 4: \
- __get_user_asm("lw", (ptr), __gu_val, __gu_err); \
+ __get_user_asm("lw", (ptr), x, __gu_err); \
break; \
- case 8: \
- __gu_err = __copy_from_user(&__gu_val, ptr, 8); \
- if (__gu_err) \
- __gu_err = -EFAULT; \
+ case 8: { \
+ __u64 __x = 0; \
+ __gu_err = raw_copy_from_user(&__x, ptr, 8) ? \
+ -EFAULT : 0;\
+ (x) = (typeof(x))(typeof((x) - (x)))__x;\
break; \
+ } \
default:\
/* __gu_val = 0; __gu_err = -EINVAL;*/ __gu_err = __user_bad();\
} \
- x = (__force __typeof__(*(ptr))) __gu_val; \
__gu_err; \
})
diff --git a/arch/nios2/include/asm/uaccess.h b/arch/nios2/include/asm/uaccess.h
index ba9340e96fd4..ca9285a915ef 100644
--- a/arch/nios2/include/asm/uaccess.h
+++ b/arch/nios2/include/asm/uaccess.h
@@ -88,6 +88,7 @@ extern __must_check long strnlen_user(const char __user *s,
long n);
/* Optimized macros */
#define __get_user_asm(val, insn, addr, err) \
{ \
+ unsigned long __gu_val; \
__asm__ __volatile__( \
" movi%0, %3\n" \
"1: " insn " %1, 0(%2)\n" \
@@ -96,14 +97,20 @@ extern __must_check long strnlen_user(const char __user *s,
long n);
" .section __ex_table,\"a\"\n"\
" .word 1b, 2b\n" \
" .previous"\
- : "=&r" (err), "=r" (val) \
+ : "=&r" (err), "=r" (__gu_val) \
: "r" (addr), "i" (-EFAULT)); \
+ val = (__force __typeof__(*(addr)))__gu_val;\
}
-#define __get_user_unknown(val, size, ptr, err) do { \
+extern void __get_user_unknown(void);
+
+#define __get_user_8(val, ptr, err) do { \
+ u64 __val = 0; \
err =
Re: [PATCH v2 12/18] uaccess: fix type mismatch warnings from access_ok()
On 2/16/22 07:13, Arnd Bergmann wrote: From: Arnd Bergmann On some architectures, access_ok() does not do any argument type checking, so replacing the definition with a generic one causes a few warnings for harmless issues that were never caught before. Fix the ones that I found either through my own test builds or that were reported by the 0-day bot. Reported-by: kernel test robot Signed-off-by: Arnd Bergmann --- arch/arc/kernel/process.c | 2 +- arch/arm/kernel/swp_emulate.c | 2 +- arch/arm/kernel/traps.c | 2 +- arch/csky/kernel/signal.c | 2 +- arch/mips/sibyte/common/sb_tbprof.c | 6 +++--- arch/nios2/kernel/signal.c | 20 +++- Acked-by: Dinh Nguyen ___ linux-snps-arc mailing list linux-snps-arc@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-snps-arc
Re: [PATCH v2 13/18] uaccess: generalize access_ok()
On 2/16/22 07:13, 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. Reviewed-by: Christoph Hellwig Acked-by: Mark Rutland [arm64, asm-generic] Signed-off-by: Arnd Bergmann --- arch/Kconfig | 7 arch/alpha/include/asm/uaccess.h | 34 +++ arch/arc/include/asm/uaccess.h| 29 - arch/arm/include/asm/uaccess.h| 20 + arch/arm64/include/asm/uaccess.h | 11 ++--- arch/csky/include/asm/uaccess.h | 8 arch/hexagon/include/asm/uaccess.h| 25 arch/ia64/include/asm/uaccess.h | 5 +-- arch/m68k/Kconfig.cpu | 1 + arch/m68k/include/asm/uaccess.h | 19 + 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 + Acked-by: Dinh Nguyen ___ linux-snps-arc mailing list linux-snps-arc@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-snps-arc
Re: [PATCH v2 18/18] uaccess: drop maining CONFIG_SET_FS users
On 2/16/22 07:13, Arnd Bergmann wrote: From: Arnd Bergmann There are no remaining callers of set_fs(), so CONFIG_SET_FS can be removed globally, along with the thread_info field and any references to it. This turns access_ok() into a cheaper check against TASK_SIZE_MAX. With CONFIG_SET_FS gone, so drop all remaining references to set_fs()/get_fs(), mm_segment_t and uaccess_kernel(). Signed-off-by: Arnd Bergmann --- arch/Kconfig | 3 - arch/alpha/Kconfig| 1 - arch/alpha/include/asm/processor.h| 4 -- arch/alpha/include/asm/thread_info.h | 2 - arch/alpha/include/asm/uaccess.h | 19 -- arch/arc/Kconfig | 1 - arch/arc/include/asm/segment.h| 20 --- arch/arc/include/asm/thread_info.h| 3 - arch/arc/include/asm/uaccess.h| 1 - arch/arm/lib/uaccess_with_memcpy.c| 10 arch/csky/Kconfig | 1 - arch/csky/include/asm/processor.h | 2 - arch/csky/include/asm/segment.h | 10 arch/csky/include/asm/thread_info.h | 2 - arch/csky/include/asm/uaccess.h | 3 - arch/csky/kernel/asm-offsets.c| 1 - arch/h8300/Kconfig| 1 - arch/h8300/include/asm/processor.h| 1 - arch/h8300/include/asm/segment.h | 40 - arch/h8300/include/asm/thread_info.h | 3 - arch/h8300/kernel/entry.S | 1 - arch/h8300/kernel/head_ram.S | 1 - arch/h8300/mm/init.c | 6 -- arch/h8300/mm/memory.c| 1 - arch/hexagon/Kconfig | 1 - arch/hexagon/include/asm/thread_info.h| 6 -- arch/hexagon/kernel/process.c | 1 - arch/microblaze/Kconfig | 1 - arch/microblaze/include/asm/thread_info.h | 6 -- arch/microblaze/include/asm/uaccess.h | 24 arch/microblaze/kernel/asm-offsets.c | 1 - arch/microblaze/kernel/process.c | 1 - arch/nds32/Kconfig| 1 - arch/nds32/include/asm/thread_info.h | 4 -- arch/nds32/include/asm/uaccess.h | 15 + arch/nds32/kernel/process.c | 5 +- arch/nds32/mm/alignment.c | 3 - arch/nios2/Kconfig| 1 - arch/nios2/include/asm/thread_info.h | 9 --- arch/nios2/include/asm/uaccess.h | 12 For NIOS2: Acked-by: Dinh Nguyen ___ linux-snps-arc mailing list linux-snps-arc@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-snps-arc
