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

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

[Mark Brown]

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

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.

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);
 
 	return 0;
 }

---
base-commit: fac04efc5c793dccbd07e2d59af9f90b7fc0dca4
change-id: 20241209-arm64-gcs-signal-sparse-53fa9cad67f7

Best regards,
-- 
Mark Brown <broonie@kernel.org>

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

[Catalin Marinas]

On Tue, Dec 10, 2024 at 12:42:53AM +0000, Mark Brown wrote:
> 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);

Would we now get a warning here?

-- 
Catalin

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

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 546 bytes --]

On Tue, Dec 10, 2024 at 02:17:58PM +0000, Catalin Marinas wrote:
> On Tue, Dec 10, 2024 at 12:42:53AM +0000, Mark Brown wrote:

> > -	write_sysreg_s(gcspr_el0 + 1, SYS_GCSPR_EL0);
> > +	write_sysreg_s((unsigned long)(gcspr_el0 + 1), SYS_GCSPR_EL0);

> Would we now get a warning here?

No, like the changelog mentions converting to an unsigned long is
regarded by sparse as a (the AFAICT) valid way of discarding the
__userness of a pointer so it's that change that silences the warning.
It does the right thing with whatever sparse I have here.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

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

[Mark Rutland]

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>
> 
> 

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

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1398 bytes --]

On Tue, Dec 10, 2024 at 02:48:48PM +0000, Mark Rutland wrote:
> 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.

The spuriousness is arguable, from my point of view it's spurious in
that we don't have the type of the system register we're writing to.

> > +	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.

I find it both safer and clearer to keep values which are userspace
pointers as userspace pointers rather than working with them as
integers, using integers just sets off alarm bells.  

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

With map_shadow_stack() it's a bit of an issue with letting users
specify a size but yeah, we could do better there.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

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

[Mark Rutland]

On Tue, Dec 10, 2024 at 03:44:29PM +0000, Mark Brown wrote:
> On Tue, Dec 10, 2024 at 02:48:48PM +0000, Mark Rutland wrote:
> > 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.
> 
> The spuriousness is arguable, from my point of view it's spurious in
> that we don't have the type of the system register we're writing to.

All that I'm asking for here is a trivial rewording; make the title say
something like:

  arm64/signal: Avoid sparse warning when manipulating GCSPR_EL0

... and in the commit message, say something like:

  Sparse complains about the manipulation of the GCSPR_EL0 value in
  gcs_restore_signal(), because we cast to/from the __user address space
  without a __force cast. Silence this warning by ${DOING_THING}.

... which clearly explains what's actually going wrong, rather than
making spurious complaints about the tool that may mislead a reader of
the commit message.

> > > +	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.
> 
> I find it both safer and clearer to keep values which are userspace
> pointers as userspace pointers rather than working with them as
> integers, using integers just sets off alarm bells.  

Having casts strewn throughout the code sets off more alarm bells for
me.

> > Similarly in map_shadow_stack(), it'd be simpler to treat cap_ptr as an
> > integer type.
> 
> With map_shadow_stack() it's a bit of an issue with letting users
> specify a size but yeah, we could do better there.

I don't follow. The only place where size interacts with cap_ptr is when
we initialize cap_ptr, and there we're adding size to an integer type:

	cap_ptr = (unsigned long __user *)(addr + size -
					   (cap_offset * sizeof(unsigned long)));

I was suggesting something along the lines of the diff below.

Mark.

diff --git a/arch/arm64/mm/gcs.c b/arch/arm64/mm/gcs.c
index 5c46ec527b1cd..096add5f2ddb2 100644
--- a/arch/arm64/mm/gcs.c
+++ b/arch/arm64/mm/gcs.c
@@ -71,10 +71,7 @@ unsigned long gcs_alloc_thread_stack(struct task_struct *tsk,
 SYSCALL_DEFINE3(map_shadow_stack, unsigned long, addr, unsigned long, size, unsigned int, flags)
 {
        unsigned long alloc_size;
-       unsigned long __user *cap_ptr;
-       unsigned long cap_val;
        int ret = 0;
-       int cap_offset;
 
        if (!system_supports_gcs())
                return -EOPNOTSUPP;
@@ -106,17 +103,16 @@ SYSCALL_DEFINE3(map_shadow_stack, unsigned long, addr, unsigned long, size, unsi
         * can be switched to.
         */
        if (flags & SHADOW_STACK_SET_TOKEN) {
+               unsigned long cap_addr = addr + size - sizeof(unsigned long);
+               unsigned long cap_val;
+
                /* Leave an extra empty frame as a top of stack marker? */
                if (flags & SHADOW_STACK_SET_MARKER)
-                       cap_offset = 2;
-               else
-                       cap_offset = 1;
+                       cap_addr -= sizeof(unsigned long)
 
-               cap_ptr = (unsigned long __user *)(addr + size -
-                                                  (cap_offset * sizeof(unsigned long)));
-               cap_val = GCS_CAP(cap_ptr);
+               cap_val = GCS_CAP(cap_addr);
 
-               put_user_gcs(cap_val, cap_ptr, &ret);
+               put_user_gcs(cap_val, (unsigned long __user *)cap_addr, &ret);
                if (ret != 0) {
                        vm_munmap(addr, size);
                        return -EFAULT;

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

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1685 bytes --]

On Tue, Dec 10, 2024 at 04:35:57PM +0000, Mark Rutland wrote:
> On Tue, Dec 10, 2024 at 03:44:29PM +0000, Mark Brown wrote:

> > The spuriousness is arguable, from my point of view it's spurious in
> > that we don't have the type of the system register we're writing to.

> All that I'm asking for here is a trivial rewording; make the title say
> something like:

Yes, I had already removed the references to spurious and false positive
locally and changed the unsigned long cast to a __force u64 cast.

> > I find it both safer and clearer to keep values which are userspace
> > pointers as userspace pointers rather than working with them as
> > integers, using integers just sets off alarm bells.  

> Having casts strewn throughout the code sets off more alarm bells for
> me.

With the new code there's only a cast when we store the value to the
register, which is the point where we're discarding the type safety.

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

> > With map_shadow_stack() it's a bit of an issue with letting users
> > specify a size but yeah, we could do better there.

> I don't follow. The only place where size interacts with cap_ptr is when
> we initialize cap_ptr, and there we're adding size to an integer type:

> 	cap_ptr = (unsigned long __user *)(addr + size -
> 					   (cap_offset * sizeof(unsigned long)));

Ugh, addr is also not a pointer which I'd not noticed but still.  My
main thought there was to move the cap_offset change to a second step so
it was done type safely.

> I was suggesting something along the lines of the diff below.

Yes, I know.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

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

[Catalin Marinas]

On Tue, Dec 10, 2024 at 04:52:49PM +0000, Mark Brown wrote:
> On Tue, Dec 10, 2024 at 04:35:57PM +0000, Mark Rutland wrote:
> > On Tue, Dec 10, 2024 at 03:44:29PM +0000, Mark Brown wrote:
> > > The spuriousness is arguable, from my point of view it's spurious in
> > > that we don't have the type of the system register we're writing to.
> 
> > All that I'm asking for here is a trivial rewording; make the title say
> > something like:
> 
> Yes, I had already removed the references to spurious and false positive
> locally and changed the unsigned long cast to a __force u64 cast.

I still have a slight preference for treating a sysreg value as an
integer and cast it to pointer when needed rather than using __force.

> > > With map_shadow_stack() it's a bit of an issue with letting users
> > > specify a size but yeah, we could do better there.
> 
> > I don't follow. The only place where size interacts with cap_ptr is when
> > we initialize cap_ptr, and there we're adding size to an integer type:
> 
> > 	cap_ptr = (unsigned long __user *)(addr + size -
> > 					   (cap_offset * sizeof(unsigned long)));
> 
> Ugh, addr is also not a pointer which I'd not noticed but still.

'addr' should stay as long in map_shadow_stack(). This matches mmap(),
mremap(), brk() etc. as they handle address space layout manipulation
(these functions also do not accept tagged pointers). map_shadow_stack()
does write a cap to memory but its primary functionality is not user
buffer access but rather memory layout management.

-- 
Catalin

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

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 602 bytes --]

On Fri, Dec 13, 2024 at 04:17:48PM +0000, Catalin Marinas wrote:
> On Tue, Dec 10, 2024 at 04:52:49PM +0000, Mark Brown wrote:

> > Yes, I had already removed the references to spurious and false positive
> > locally and changed the unsigned long cast to a __force u64 cast.

> I still have a slight preference for treating a sysreg value as an
> integer and cast it to pointer when needed rather than using __force.

I've now changed to that and will send it later - TBH I'm having a very
hard time seeing it as an improvement, especially with the update to
gcs_signal_entry() where we do two writes.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]