Re: [PATCH] arm64: uaccess: simplify uaccess_mask_ptr()

[Will Deacon <will@kernel.org>]

On Thu, Sep 22, 2022 at 04:10:53PM +0100, Mark Rutland wrote:
> We introduced uaccess pointer masking for arm64 in commit:
> 
>   4d8efc2d5ee4c9cc ("arm64: Use pointer masking to limit uaccess speculation")
> 
> Which was intended to prevent speculative uaccesses to kernel memory on
> CPUs where access permissions were not respected under speculation.
> 
> At the time, the uaccess primitives were occasionally used to access
> kernel memory, with the maximum permitted address held in
> thread_info::addr_limit. Consequently, the address masking needed to
> take this dynamic limit into account.
> 
> Subsequently the uaccess primitives were reworked such that they are
> only used for user memory, and as of commit:
> 
>   3d2403fd10a1dbb3 ("arm64: uaccess: remove set_fs()")
> 
> ... the address limit was made a compile-time constant, but the logic
> was otherwise unchanged.
> 
> Regardless of the configured VA size or whether TBI is in use, the
> address space can be divided into three ranges:
> 
> * The TTBR0 VA range, for which any valid pointer has bit 55 *clear*,
>   and any non-tag bits [63-56] must match bit 55 (i.e. must be clear).
> 
> * The TTBR1 VA range, for which any valid pointer has bit 55 *set*, and
>   any non-tag bits [63-56] must match bit 55 (i.e. must be set).
> 
> * The gap between the TTBR0 and TTBR1 ranges, where bit 55 may be set or
>   clear, but any access will result in a fault.
> 
> As the uaccess primitives are now only used for user memory in the TTBR0
> VA range, we can prevent generation of TTBR1 addresses by clearing bit
> 55, which will either result in a TTBR0 address or a faulting address
> between the TTBR VA ranges.
> 
> This is beneficial for code generation as:
> 
> * We no longer clobber the condition codes.
> 
> * We no longer burn a register on (TASK_SIZE_MAX - 1).
> 
> * We no longer need to consume the untagged pointer.
> 
> When building a defconfig v6.0-rc3 with GCC 12.1.0, this change makes
> the resulting Image 64KiB smaller.
> 
> Signed-off-by: Mark Rutland <mark.rutland@arm.com>
> Cc: Catalin Marinas <catalin.marinas@arm.com>
> Cc: James Morse <james.morse@arm.com>
> Cc: Robin Murphy <robin.murphy@arm.com>
> Cc: Will Deacon <will@kernel.org>
> ---
>  arch/arm64/include/asm/uaccess.h | 19 ++++++++++---------
>  1 file changed, 10 insertions(+), 9 deletions(-)
> 
> diff --git a/arch/arm64/include/asm/uaccess.h b/arch/arm64/include/asm/uaccess.h
> index 2fc9f0861769a..e69559826cb8c 100644
> --- a/arch/arm64/include/asm/uaccess.h
> +++ b/arch/arm64/include/asm/uaccess.h
> @@ -203,9 +203,11 @@ static inline void uaccess_enable_privileged(void)
>  }
>  
>  /*
> - * Sanitise a uaccess pointer such that it becomes NULL if above the maximum
> - * user address. In case the pointer is tagged (has the top byte set), untag
> - * the pointer before checking.
> + * Sanitize a uaccess pointer such that it cannot reach any kernel address.
> + *
> + * Clearing bit 55 ensures the pointer cannot address any portion of the TTBR1
> + * address range (i.e. any kernel address), and either the pointer falls within
> + * the TTBR0 address range or must cause a fault.
>   */
>  #define uaccess_mask_ptr(ptr) (__typeof__(ptr))__uaccess_mask_ptr(ptr)
>  static inline void __user *__uaccess_mask_ptr(const void __user *ptr)
> @@ -213,12 +215,11 @@ static inline void __user *__uaccess_mask_ptr(const void __user *ptr)
>  	void __user *safe_ptr;
>  
>  	asm volatile(
> -	"	bics	xzr, %3, %2\n"
> -	"	csel	%0, %1, xzr, eq\n"
> -	: "=&r" (safe_ptr)
> -	: "r" (ptr), "r" (TASK_SIZE_MAX - 1),
> -	  "r" (untagged_addr(ptr))
> -	: "cc");
> +	"	bic	%0, %1, %2\n"
> +	: "=r" (safe_ptr)
> +	: "r" (ptr),
> +	  "i" (BIT(55))
> +	);
>  
>  	csdb();

Why do we still need the CSDB after your change?

Will

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel