Re: [RFC PATCH for 4.18 3/5] rseq: uapi: declare rseq_cs field as union, update includes

[Mathieu Desnoyers <mathieu.desnoyers@efficios.com>]

----- On Jul 5, 2018, at 2:05 PM, Mathieu Desnoyers mathieu.desnoyers@efficios.com wrote:

> Declaring the rseq_cs field as a union between __u64 and two __u32
> allows both 32-bit and 64-bit kernels to read the full __u64, and
> therefore validate that a 32-bit user-space cleared the upper 32
> bits, thus ensuring a consistent behavior between native 32-bit
> kernels and 32-bit compat tasks on 64-bit kernels.
> 
> Check that the rseq_cs value read is < TASK_SIZE.
> 
> The asm/byteorder.h header needs to be included by rseq.h, now
> that it is not using linux/types_32_64.h anymore.
> 
> Considering that only __32 and __u64 types are declared in linux/rseq.h,
> the linux/types.h header should always be included for both kernel and
> user-space code: including stdint.h is just for u64 and u32, which are
> not used in this header at all.

The 0-day bot noticed that __get_user() is unimplemented for 64-bit
values on arm32 (although get_user() is implemented).

The following diff fixes this discrepancy, and allows this rseq patch
to build on arm32:

commit dde99f3310c76acb0a160c0572f40b6aa279594c
Author: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
Date:   Fri Jul 6 11:29:39 2018 -0400

    arm: implement 64-bit __get_user
    
    get_user() is implemented on arm32 for 64-bit user-space values, but
    not its __get_user() counterpart.
    
    Implement __get_user() as two __get_user_asm_word().
    
    Signed-off-by: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
    CC: "Paul E. McKenney" <paulmck@linux.vnet.ibm.com>
    CC: Peter Zijlstra <peterz@infradead.org>
    CC: Paul Turner <pjt@google.com>
    CC: Thomas Gleixner <tglx@linutronix.de>
    CC: Andy Lutomirski <luto@amacapital.net>
    CC: Andi Kleen <andi@firstfloor.org>
    CC: Dave Watson <davejwatson@fb.com>
    CC: Chris Lameter <cl@linux.com>
    CC: Ingo Molnar <mingo@redhat.com>
    CC: "H. Peter Anvin" <hpa@zytor.com>
    CC: Ben Maurer <bmaurer@fb.com>
    CC: Steven Rostedt <rostedt@goodmis.org>
    CC: Josh Triplett <josh@joshtriplett.org>
    CC: Linus Torvalds <torvalds@linux-foundation.org>
    CC: Andrew Morton <akpm@linux-foundation.org>
    CC: Russell King <linux@arm.linux.org.uk>
    CC: Catalin Marinas <catalin.marinas@arm.com>
    CC: Will Deacon <will.deacon@arm.com>
    CC: Michael Kerrisk <mtk.manpages@gmail.com>
    CC: Boqun Feng <boqun.feng@gmail.com>
    CC: linux-api@vger.kernel.org

diff --git a/arch/arm/include/asm/uaccess.h b/arch/arm/include/asm/uaccess.h
index 3d614e9..38659c6 100644
--- a/arch/arm/include/asm/uaccess.h
+++ b/arch/arm/include/asm/uaccess.h
@@ -265,10 +265,16 @@ static inline void set_fs(mm_segment_t fs)
        (void) 0;                                                       \
 })
 
+union __gu_u64 {
+       u64 val64;
+       u32 word[2];
+};
+
 #define __get_user_err(x, ptr, err)                                    \
 do {                                                                   \
        unsigned long __gu_addr = (unsigned long)(ptr);                 \
        unsigned long __gu_val;                                         \
+       union __gu_u64 __gu_tmp64;                                      \
        unsigned int __ua_flags;                                        \
        __chk_user_ptr(ptr);                                            \
        might_fault();                                                  \
@@ -277,10 +283,28 @@ static inline void set_fs(mm_segment_t fs)
        case 1: __get_user_asm_byte(__gu_val, __gu_addr, err);  break;  \
        case 2: __get_user_asm_half(__gu_val, __gu_addr, err);  break;  \
        case 4: __get_user_asm_word(__gu_val, __gu_addr, err);  break;  \
+       case 8:                                                         \
+       {                                                               \
+               union __gu_u64 __user *__gu_addr64 =                    \
+                       (union __gu_u64 __user *)__gu_addr;             \
+               __get_user_asm_word(__gu_tmp64.word[0],                 \
+                        &__gu_addr64->word[0], err);                   \
+               if (err)                                                \
+                       break;                                          \
+               __get_user_asm_word(__gu_tmp64.word[1],                 \
+                        &__gu_addr64->word[1], err);                   \
+               break;                                                  \
+       };                                                              \
        default: (__gu_val) = __get_user_bad();                         \
        }                                                               \
        uaccess_restore(__ua_flags);                                    \
-       (x) = (__typeof__(*(ptr)))__gu_val;                             \
+       switch (sizeof(*(ptr))) {                                       \
+       case 1:                                                         \
+       case 2:                                                         \
+       case 4:                                                         \
+       default: (x) = (__typeof__(*(ptr)))__gu_val; break;             \
+       case 8: (x) = (__typeof__(*(ptr)))__gu_tmp64.val64; break;      \
+       }                                                               \
 } while (0)
 
 #define __get_user_asm(x, addr, err, instr)                    \


> 
> Signed-off-by: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
> CC: "Paul E. McKenney" <paulmck@linux.vnet.ibm.com>
> CC: Peter Zijlstra <peterz@infradead.org>
> CC: Paul Turner <pjt@google.com>
> CC: Thomas Gleixner <tglx@linutronix.de>
> CC: Andy Lutomirski <luto@amacapital.net>
> CC: Andi Kleen <andi@firstfloor.org>
> CC: Dave Watson <davejwatson@fb.com>
> CC: Chris Lameter <cl@linux.com>
> CC: Ingo Molnar <mingo@redhat.com>
> CC: "H. Peter Anvin" <hpa@zytor.com>
> CC: Ben Maurer <bmaurer@fb.com>
> CC: Steven Rostedt <rostedt@goodmis.org>
> CC: Josh Triplett <josh@joshtriplett.org>
> CC: Linus Torvalds <torvalds@linux-foundation.org>
> CC: Andrew Morton <akpm@linux-foundation.org>
> CC: Russell King <linux@arm.linux.org.uk>
> CC: Catalin Marinas <catalin.marinas@arm.com>
> CC: Will Deacon <will.deacon@arm.com>
> CC: Michael Kerrisk <mtk.manpages@gmail.com>
> CC: Boqun Feng <boqun.feng@gmail.com>
> CC: linux-api@vger.kernel.org
> ---
> include/uapi/linux/rseq.h           | 27 +++++++++++++++++++--------
> kernel/rseq.c                       | 12 +++++++-----
> tools/testing/selftests/rseq/rseq.h | 11 ++++++++++-
> 3 files changed, 36 insertions(+), 14 deletions(-)
> 
> diff --git a/include/uapi/linux/rseq.h b/include/uapi/linux/rseq.h
> index bf4188c13bec..9a402fdb60e9 100644
> --- a/include/uapi/linux/rseq.h
> +++ b/include/uapi/linux/rseq.h
> @@ -10,13 +10,8 @@
>  * Copyright (c) 2015-2018 Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
>  */
> 
> -#ifdef __KERNEL__
> -# include <linux/types.h>
> -#else
> -# include <stdint.h>
> -#endif
> -
> -#include <linux/types_32_64.h>
> +#include <linux/types.h>
> +#include <asm/byteorder.h>
> 
> enum rseq_cpu_id_state {
> 	RSEQ_CPU_ID_UNINITIALIZED		= -1,
> @@ -111,7 +106,23 @@ struct rseq {
> 	 * atomicity semantics. This field should only be updated by the
> 	 * thread which registered this data structure. Aligned on 64-bit.
> 	 */
> -	LINUX_FIELD_u32_u64(rseq_cs);
> +	union {
> +		__u64 ptr64;
> +#ifdef __LP64__
> +		__u64 ptr;
> +#else
> +		struct {
> +#if (defined(__BYTE_ORDER) && (__BYTE_ORDER == __BIG_ENDIAN)) ||
> defined(__BIG_ENDIAN)
> +			__u32 padding;		/* Initialized to zero. */
> +			__u32 ptr32;
> +#else /* LITTLE */
> +			__u32 ptr32;
> +			__u32 padding;		/* Initialized to zero. */
> +#endif /* ENDIAN */
> +		} ptr;
> +#endif
> +	} rseq_cs;
> +
> 	/*
> 	 * Restartable sequences flags field.
> 	 *
> diff --git a/kernel/rseq.c b/kernel/rseq.c
> index 16b38c5342f9..3081e6783cce 100644
> --- a/kernel/rseq.c
> +++ b/kernel/rseq.c
> @@ -115,19 +115,21 @@ static int rseq_reset_rseq_cpu_id(struct task_struct *t)
> static int rseq_get_rseq_cs(struct task_struct *t, struct rseq_cs *rseq_cs)
> {
> 	struct rseq_cs __user *urseq_cs;
> -	unsigned long ptr;
> +	u64 ptr;
> 	u32 __user *usig;
> 	u32 sig;
> 	int ret;
> 
> -	ret = __get_user(ptr, &t->rseq->rseq_cs);
> +	ret = __get_user(ptr, &t->rseq->rseq_cs.ptr64);
> 	if (ret)
> 		return ret;
> 	if (!ptr) {
> 		memset(rseq_cs, 0, sizeof(*rseq_cs));
> 		return 0;
> 	}
> -	urseq_cs = (struct rseq_cs __user *)ptr;
> +	if (ptr >= TASK_SIZE)
> +		return -EINVAL;
> +	urseq_cs = (struct rseq_cs __user *)(unsigned long)ptr;
> 	if (copy_from_user(rseq_cs, urseq_cs, sizeof(*rseq_cs)))
> 		return -EFAULT;
> 
> @@ -201,9 +203,9 @@ static int clear_rseq_cs(struct task_struct *t)
> 	 * of code outside of the rseq assembly block. This performs
> 	 * a lazy clear of the rseq_cs field.
> 	 *
> -	 * Set rseq_cs to NULL with single-copy atomicity.
> +	 * Set rseq_cs to NULL.
> 	 */
> -	return __put_user(0UL, &t->rseq->rseq_cs);
> +	return __put_user(0ULL, &t->rseq->rseq_cs.ptr64);
> }
> 
> /*
> diff --git a/tools/testing/selftests/rseq/rseq.h
> b/tools/testing/selftests/rseq/rseq.h
> index a4684112676c..f2073cfa4448 100644
> --- a/tools/testing/selftests/rseq/rseq.h
> +++ b/tools/testing/selftests/rseq/rseq.h
> @@ -133,6 +133,15 @@ static inline uint32_t rseq_current_cpu(void)
> 	return cpu;
> }
> 
> +static inline void rseq_clear_rseq_cs(void)
> +{
> +#ifdef __LP64__
> +	__rseq_abi.rseq_cs.ptr = 0;
> +#else
> +	__rseq_abi.rseq_cs.ptr.ptr32 = 0;
> +#endif
> +}
> +
> /*
>  * rseq_prepare_unload() should be invoked by each thread using rseq_finish*()
>  * at least once between their last rseq_finish*() and library unload of the
> @@ -143,7 +152,7 @@ static inline uint32_t rseq_current_cpu(void)
>  */
> static inline void rseq_prepare_unload(void)
> {
> -	__rseq_abi.rseq_cs = 0;
> +	rseq_clear_rseq_cs();
> }
> 
> #endif  /* RSEQ_H_ */
> --
> 2.11.0

-- 
Mathieu Desnoyers
EfficiOS Inc.
http://www.efficios.com