Re: [PATCH] arm64/signal: Silence spurious sparse warning storing GCSPR_EL0

[Mark Rutland <mark.rutland@arm.com>]

On Tue, Dec 10, 2024 at 12:42:53AM +0000, Mark Brown wrote:
> We are seeing a false postive sparse warning in gcs_restore_signal()
> 
> arch/arm64/kernel/signal.c:1054:9: sparse: sparse: cast removes address space '__user' of expression

This isn't a false positive; this is a cross-address space cast that
sparse is accurately warning about. That might be *benign*, but the tool
is doing exactly what it is supposed to.

> when storing the final GCSPR_EL0 value back into the register, caused by
> the fact that write_sysreg_s() casts the value it writes to a u64 which
> sparse sees as discarding the __userness of the pointer. The magic for
> handling such situations with sparse is to cast the value to an unsigned
> long which sparse sees as a valid thing to do with __user pointers so add
> such a cast.

As a general note, casting to/from unsigned long or uintptr_t is a
special-case in sparse, and in general you should use __force to cast
across address spaces or to/from bitwise types, e.g.

	some_type *foo = ...;
	some_type __user *user_foo;

	user_foo = (__force some_type __user *)foo;

> While we're at it also remove spurious casts of the gcspr_el0 value as we
> manipulate it which were the result of bitrot as the code was tweaked in
> the long period it was out of tree.
> 
> Reported-by: kernel test robot <lkp@intel.com>
> Closes: https://lore.kernel.org/oe-kbuild-all/202412082005.OBJ0BbWs-lkp@intel.com/
> Signed-off-by: Mark Brown <broonie@kernel.org>
> ---
>  arch/arm64/kernel/signal.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/arch/arm64/kernel/signal.c b/arch/arm64/kernel/signal.c
> index 14ac6fdb872b9672e4b16a097f1b577aae8dec50..83ea7e5fd2b54566c6649b82b8570657a5711dd4 100644
> --- a/arch/arm64/kernel/signal.c
> +++ b/arch/arm64/kernel/signal.c
> @@ -39,7 +39,7 @@
>  #ifdef CONFIG_ARM64_GCS
>  #define GCS_SIGNAL_CAP(addr) (((unsigned long)addr) & GCS_CAP_ADDR_MASK)
>  
> -static bool gcs_signal_cap_valid(u64 addr, u64 val)
> +static bool gcs_signal_cap_valid(unsigned long __user *addr, u64 val)
>  {
>  	return val == GCS_SIGNAL_CAP(addr);
>  }
> @@ -1094,15 +1094,15 @@ static int gcs_restore_signal(void)
>  	/*
>  	 * Check that the cap is the actual GCS before replacing it.
>  	 */
> -	if (!gcs_signal_cap_valid((u64)gcspr_el0, cap))
> +	if (!gcs_signal_cap_valid(gcspr_el0, cap))
>  		return -EINVAL;
>  
>  	/* Invalidate the token to prevent reuse */
> -	put_user_gcs(0, (__user void*)gcspr_el0, &ret);
> +	put_user_gcs(0, gcspr_el0, &ret);
>  	if (ret != 0)
>  		return -EFAULT;
>  
> -	write_sysreg_s(gcspr_el0 + 1, SYS_GCSPR_EL0);
> +	write_sysreg_s((unsigned long)(gcspr_el0 + 1), SYS_GCSPR_EL0);

Only one line here wants a __user pointer, so wouldn't it be simpler to
pass 'gcspr_el0' as an integer type, and cast it at the point it's used
as an actual pointer, rather than the other way around?

Then you could also simplify gcs_restore_signal(), etc.

Similarly in map_shadow_stack(), it'd be simpler to treat cap_ptr as an
integer type.

Mark.

>  
>  	return 0;
>  }
> 
> ---
> base-commit: fac04efc5c793dccbd07e2d59af9f90b7fc0dca4
> change-id: 20241209-arm64-gcs-signal-sparse-53fa9cad67f7
> 
> Best regards,
> -- 
> Mark Brown <broonie@kernel.org>
> 
>