2022-02-16 13:35:24

by Arnd Bergmann

[permalink] [raw]
Subject: [PATCH v2 07/18] nios2: drop access_ok() check from __put_user()

From: Arnd Bergmann <[email protected]>

Unlike other architectures, the nios2 version of __put_user() has an
extra check for access_ok(), preventing it from being used to implement
__put_kernel_nofault().

Split up put_user() along the same lines as __get_user()/get_user()

Signed-off-by: Arnd Bergmann <[email protected]>
---
arch/nios2/include/asm/uaccess.h | 56 +++++++++++++++++++-------------
1 file changed, 33 insertions(+), 23 deletions(-)

diff --git a/arch/nios2/include/asm/uaccess.h b/arch/nios2/include/asm/uaccess.h
index ca9285a915ef..a5cbe07cf0da 100644
--- a/arch/nios2/include/asm/uaccess.h
+++ b/arch/nios2/include/asm/uaccess.h
@@ -167,34 +167,44 @@ do { \
: "r" (val), "r" (ptr), "i" (-EFAULT)); \
}

-#define put_user(x, ptr) \
+#define __put_user_common(__pu_val, __pu_ptr) \
({ \
long __pu_err = -EFAULT; \
- __typeof__(*(ptr)) __user *__pu_ptr = (ptr); \
- __typeof__(*(ptr)) __pu_val = (__typeof(*ptr))(x); \
- if (access_ok(__pu_ptr, sizeof(*__pu_ptr))) { \
- switch (sizeof(*__pu_ptr)) { \
- case 1: \
- __put_user_asm(__pu_val, "stb", __pu_ptr, __pu_err); \
- break; \
- case 2: \
- __put_user_asm(__pu_val, "sth", __pu_ptr, __pu_err); \
- break; \
- case 4: \
- __put_user_asm(__pu_val, "stw", __pu_ptr, __pu_err); \
- break; \
- default: \
- /* XXX: This looks wrong... */ \
- __pu_err = 0; \
- if (copy_to_user(__pu_ptr, &(__pu_val), \
- sizeof(*__pu_ptr))) \
- __pu_err = -EFAULT; \
- break; \
- } \
+ switch (sizeof(*__pu_ptr)) { \
+ case 1: \
+ __put_user_asm(__pu_val, "stb", __pu_ptr, __pu_err); \
+ break; \
+ case 2: \
+ __put_user_asm(__pu_val, "sth", __pu_ptr, __pu_err); \
+ break; \
+ case 4: \
+ __put_user_asm(__pu_val, "stw", __pu_ptr, __pu_err); \
+ break; \
+ default: \
+ /* XXX: This looks wrong... */ \
+ __pu_err = 0; \
+ if (__copy_to_user(__pu_ptr, &(__pu_val), \
+ sizeof(*__pu_ptr))) \
+ __pu_err = -EFAULT; \
+ break; \
} \
__pu_err; \
})

-#define __put_user(x, ptr) put_user(x, ptr)
+#define __put_user(x, ptr) \
+({ \
+ __auto_type __pu_ptr = (ptr); \
+ typeof(*__pu_ptr) __pu_val = (typeof(*__pu_ptr))(x); \
+ __put_user_common(__pu_val, __pu_ptr); \
+})
+
+#define put_user(x, ptr) \
+({ \
+ __auto_type __pu_ptr = (ptr); \
+ typeof(*__pu_ptr) __pu_val = (typeof(*__pu_ptr))(x); \
+ access_ok(__pu_ptr, sizeof(*__pu_ptr)) ? \
+ __put_user_common(__pu_val, __pu_ptr) : \
+ -EFAULT; \
+})

#endif /* _ASM_NIOS2_UACCESS_H */
--
2.29.2


2022-02-18 06:46:53

by Christoph Hellwig

[permalink] [raw]
Subject: Re: [PATCH v2 07/18] nios2: drop access_ok() check from __put_user()

On Wed, Feb 16, 2022 at 02:13:21PM +0100, Arnd Bergmann wrote:
> From: Arnd Bergmann <[email protected]>
>
> Unlike other architectures, the nios2 version of __put_user() has an
> extra check for access_ok(), preventing it from being used to implement
> __put_kernel_nofault().
>
> Split up put_user() along the same lines as __get_user()/get_user()

Looks good:

Reviewed-by: Christoph Hellwig <[email protected]>

2022-02-24 02:00:30

by Dinh Nguyen

[permalink] [raw]
Subject: Re: [PATCH v2 07/18] nios2: drop access_ok() check from __put_user()



On 2/16/22 07:13, Arnd Bergmann wrote:
> From: Arnd Bergmann <[email protected]>
>
> Unlike other architectures, the nios2 version of __put_user() has an
> extra check for access_ok(), preventing it from being used to implement
> __put_kernel_nofault().
>
> Split up put_user() along the same lines as __get_user()/get_user()
>
> Signed-off-by: Arnd Bergmann <[email protected]>
> ---
> arch/nios2/include/asm/uaccess.h | 56 +++++++++++++++++++-------------
> 1 file changed, 33 insertions(+), 23 deletions(-)
>
> diff --git a/arch/nios2/include/asm/uaccess.h b/arch/nios2/include/asm/uaccess.h
> index ca9285a915ef..a5cbe07cf0da 100644
> --- a/arch/nios2/include/asm/uaccess.h
> +++ b/arch/nios2/include/asm/uaccess.h
> @@ -167,34 +167,44 @@ do { \
> : "r" (val), "r" (ptr), "i" (-EFAULT)); \
> }
>
> -#define put_user(x, ptr) \
> +#define __put_user_common(__pu_val, __pu_ptr) \
> ({ \
> long __pu_err = -EFAULT; \
> - __typeof__(*(ptr)) __user *__pu_ptr = (ptr); \
> - __typeof__(*(ptr)) __pu_val = (__typeof(*ptr))(x); \
> - if (access_ok(__pu_ptr, sizeof(*__pu_ptr))) { \
> - switch (sizeof(*__pu_ptr)) { \
> - case 1: \
> - __put_user_asm(__pu_val, "stb", __pu_ptr, __pu_err); \
> - break; \
> - case 2: \
> - __put_user_asm(__pu_val, "sth", __pu_ptr, __pu_err); \
> - break; \
> - case 4: \
> - __put_user_asm(__pu_val, "stw", __pu_ptr, __pu_err); \
> - break; \
> - default: \
> - /* XXX: This looks wrong... */ \
> - __pu_err = 0; \
> - if (copy_to_user(__pu_ptr, &(__pu_val), \
> - sizeof(*__pu_ptr))) \
> - __pu_err = -EFAULT; \
> - break; \
> - } \
> + switch (sizeof(*__pu_ptr)) { \
> + case 1: \
> + __put_user_asm(__pu_val, "stb", __pu_ptr, __pu_err); \
> + break; \
> + case 2: \
> + __put_user_asm(__pu_val, "sth", __pu_ptr, __pu_err); \
> + break; \
> + case 4: \
> + __put_user_asm(__pu_val, "stw", __pu_ptr, __pu_err); \
> + break; \
> + default: \
> + /* XXX: This looks wrong... */ \
> + __pu_err = 0; \
> + if (__copy_to_user(__pu_ptr, &(__pu_val), \
> + sizeof(*__pu_ptr))) \
> + __pu_err = -EFAULT; \
> + break; \
> } \
> __pu_err; \
> })
>
> -#define __put_user(x, ptr) put_user(x, ptr)
> +#define __put_user(x, ptr) \
> +({ \
> + __auto_type __pu_ptr = (ptr); \
> + typeof(*__pu_ptr) __pu_val = (typeof(*__pu_ptr))(x); \
> + __put_user_common(__pu_val, __pu_ptr); \
> +})
> +
> +#define put_user(x, ptr) \
> +({ \
> + __auto_type __pu_ptr = (ptr); \
> + typeof(*__pu_ptr) __pu_val = (typeof(*__pu_ptr))(x); \
> + access_ok(__pu_ptr, sizeof(*__pu_ptr)) ? \
> + __put_user_common(__pu_val, __pu_ptr) : \
> + -EFAULT; \
> +})
>
> #endif /* _ASM_NIOS2_UACCESS_H */

Acked-by: Dinh Nguyen <[email protected]>

2022-02-24 07:09:31

by Arnd Bergmann

[permalink] [raw]
Subject: Re: [PATCH v2 07/18] nios2: drop access_ok() check from __put_user()

On Thu, Feb 24, 2022 at 12:30 AM Dinh Nguyen <[email protected]> wrote:
> On 2/16/22 07:13, Arnd Bergmann wrote: From: Arnd Bergmann <[email protected]>
> >
> > Unlike other architectures, the nios2 version of __put_user() has an
> > extra check for access_ok(), preventing it from being used to implement
> > __put_kernel_nofault().
> >
> > Split up put_user() along the same lines as __get_user()/get_user()
> >
> > Signed-off-by: Arnd Bergmann <[email protected]>
>
> Acked-by: Dinh Nguyen <[email protected]>

Thanks! Could you also have a look at patch 2 (uaccess: fix nios2 and
microblaze get_user_8)? That one is actually more critical, and should
be backported to stable kernels.

Arnd