arch/arm64/kernel/signal.c:1046:36: sparse: sparse: cast removes address space '__user' of expression

arch/arm64/kernel/signal.c:1046:36: sparse: sparse: cast removes address space '__user' of expression

[kernel test robot]

tree:   https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git master
head:   7503345ac5f5e82fd9a36d6e6b447c016376403a
commit: 5d8b172e7005c6b42c16a0952c1d8873051d68ae arm64: Add Kconfig for Guarded Control Stack (GCS)
date:   9 weeks ago
config: arm64-randconfig-r133-20241208 (https://download.01.org/0day-ci/archive/20241208/202412082005.OBJ0BbWs-lkp@intel.com/config)
compiler: clang version 20.0.0git (https://github.com/llvm/llvm-project 592c0fe55f6d9a811028b5f3507be91458ab2713)
reproduce: (https://download.01.org/0day-ci/archive/20241208/202412082005.OBJ0BbWs-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202412082005.OBJ0BbWs-lkp@intel.com/

sparse warnings: (new ones prefixed by >>)
>> arch/arm64/kernel/signal.c:1046:36: sparse: sparse: cast removes address space '__user' of expression
   arch/arm64/kernel/signal.c:1054:9: sparse: sparse: cast removes address space '__user' of expression
   arch/arm64/kernel/signal.c:1461:26: sparse: sparse: incorrect type in assignment (different address spaces) @@     expected void ( [noderef] [usertype] __user *[assigned] [usertype] sigtramp )( ... ) @@     got void * @@
   arch/arm64/kernel/signal.c:1461:26: sparse:     expected void ( [noderef] [usertype] __user *[assigned] [usertype] sigtramp )( ... )
   arch/arm64/kernel/signal.c:1461:26: sparse:     got void *

vim +/__user +1046 arch/arm64/kernel/signal.c

2c020ed8d148f7 Catalin Marinas 2012-03-05  1010  
eaf62ce1563b85 Mark Brown      2024-10-01  1011  #ifdef CONFIG_ARM64_GCS
eaf62ce1563b85 Mark Brown      2024-10-01  1012  static int gcs_restore_signal(void)
eaf62ce1563b85 Mark Brown      2024-10-01  1013  {
eaf62ce1563b85 Mark Brown      2024-10-01  1014  	unsigned long __user *gcspr_el0;
eaf62ce1563b85 Mark Brown      2024-10-01  1015  	u64 cap;
eaf62ce1563b85 Mark Brown      2024-10-01  1016  	int ret;
eaf62ce1563b85 Mark Brown      2024-10-01  1017  
eaf62ce1563b85 Mark Brown      2024-10-01  1018  	if (!system_supports_gcs())
eaf62ce1563b85 Mark Brown      2024-10-01  1019  		return 0;
eaf62ce1563b85 Mark Brown      2024-10-01  1020  
eaf62ce1563b85 Mark Brown      2024-10-01  1021  	if (!(current->thread.gcs_el0_mode & PR_SHADOW_STACK_ENABLE))
eaf62ce1563b85 Mark Brown      2024-10-01  1022  		return 0;
eaf62ce1563b85 Mark Brown      2024-10-01  1023  
eaf62ce1563b85 Mark Brown      2024-10-01  1024  	gcspr_el0 = (unsigned long __user *)read_sysreg_s(SYS_GCSPR_EL0);
eaf62ce1563b85 Mark Brown      2024-10-01  1025  
eaf62ce1563b85 Mark Brown      2024-10-01  1026  	/*
eaf62ce1563b85 Mark Brown      2024-10-01  1027  	 * Ensure that any changes to the GCS done via GCS operations
eaf62ce1563b85 Mark Brown      2024-10-01  1028  	 * are visible to the normal reads we do to validate the
eaf62ce1563b85 Mark Brown      2024-10-01  1029  	 * token.
eaf62ce1563b85 Mark Brown      2024-10-01  1030  	 */
eaf62ce1563b85 Mark Brown      2024-10-01  1031  	gcsb_dsync();
eaf62ce1563b85 Mark Brown      2024-10-01  1032  
eaf62ce1563b85 Mark Brown      2024-10-01  1033  	/*
eaf62ce1563b85 Mark Brown      2024-10-01  1034  	 * GCSPR_EL0 should be pointing at a capped GCS, read the cap.
eaf62ce1563b85 Mark Brown      2024-10-01  1035  	 * We don't enforce that this is in a GCS page, if it is not
eaf62ce1563b85 Mark Brown      2024-10-01  1036  	 * then faults will be generated on GCS operations - the main
eaf62ce1563b85 Mark Brown      2024-10-01  1037  	 * concern is to protect GCS pages.
eaf62ce1563b85 Mark Brown      2024-10-01  1038  	 */
eaf62ce1563b85 Mark Brown      2024-10-01  1039  	ret = copy_from_user(&cap, gcspr_el0, sizeof(cap));
eaf62ce1563b85 Mark Brown      2024-10-01  1040  	if (ret)
eaf62ce1563b85 Mark Brown      2024-10-01  1041  		return -EFAULT;
eaf62ce1563b85 Mark Brown      2024-10-01  1042  
eaf62ce1563b85 Mark Brown      2024-10-01  1043  	/*
eaf62ce1563b85 Mark Brown      2024-10-01  1044  	 * Check that the cap is the actual GCS before replacing it.
eaf62ce1563b85 Mark Brown      2024-10-01  1045  	 */
eaf62ce1563b85 Mark Brown      2024-10-01 @1046  	if (!gcs_signal_cap_valid((u64)gcspr_el0, cap))
eaf62ce1563b85 Mark Brown      2024-10-01  1047  		return -EINVAL;
eaf62ce1563b85 Mark Brown      2024-10-01  1048  
eaf62ce1563b85 Mark Brown      2024-10-01  1049  	/* Invalidate the token to prevent reuse */
eaf62ce1563b85 Mark Brown      2024-10-01  1050  	put_user_gcs(0, (__user void*)gcspr_el0, &ret);
eaf62ce1563b85 Mark Brown      2024-10-01  1051  	if (ret != 0)
eaf62ce1563b85 Mark Brown      2024-10-01  1052  		return -EFAULT;
eaf62ce1563b85 Mark Brown      2024-10-01  1053  
eaf62ce1563b85 Mark Brown      2024-10-01  1054  	write_sysreg_s(gcspr_el0 + 1, SYS_GCSPR_EL0);
eaf62ce1563b85 Mark Brown      2024-10-01  1055  
eaf62ce1563b85 Mark Brown      2024-10-01  1056  	return 0;
eaf62ce1563b85 Mark Brown      2024-10-01  1057  }
eaf62ce1563b85 Mark Brown      2024-10-01  1058  

:::::: The code at line 1046 was first introduced by commit
:::::: eaf62ce1563b8557e3550acb97d5086120168750 arm64/signal: Set up and restore the GCS context for signal handlers

:::::: TO: Mark Brown <broonie@kernel.org>
:::::: CC: Catalin Marinas <catalin.marinas@arm.com>

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: arch/arm64/kernel/signal.c:1046:36: sparse: sparse: cast removes address space '__user' of expression

[Catalin Marinas]

On Mon, Dec 09, 2024 at 12:47:33PM +0800, kernel test robot wrote:
> sparse warnings: (new ones prefixed by >>)
> >> arch/arm64/kernel/signal.c:1046:36: sparse: sparse: cast removes address space '__user' of expression
>    arch/arm64/kernel/signal.c:1054:9: sparse: sparse: cast removes address space '__user' of expression
>    arch/arm64/kernel/signal.c:1461:26: sparse: sparse: incorrect type in assignment (different address spaces) @@     expected void ( [noderef] [usertype] __user *[assigned] [usertype] sigtramp )( ... ) @@     got void * @@
>    arch/arm64/kernel/signal.c:1461:26: sparse:     expected void ( [noderef] [usertype] __user *[assigned] [usertype] sigtramp )( ... )
>    arch/arm64/kernel/signal.c:1461:26: sparse:     got void *
> 
> vim +/__user +1046 arch/arm64/kernel/signal.c

Thanks for this, the warning is valid.

> 2c020ed8d148f7 Catalin Marinas 2012-03-05  1010  
> eaf62ce1563b85 Mark Brown      2024-10-01  1011  #ifdef CONFIG_ARM64_GCS
> eaf62ce1563b85 Mark Brown      2024-10-01  1012  static int gcs_restore_signal(void)
> eaf62ce1563b85 Mark Brown      2024-10-01  1013  {
> eaf62ce1563b85 Mark Brown      2024-10-01  1014  	unsigned long __user *gcspr_el0;

I think we should keep this as u64 since it's a sysreg.

> eaf62ce1563b85 Mark Brown      2024-10-01  1015  	u64 cap;
> eaf62ce1563b85 Mark Brown      2024-10-01  1016  	int ret;
> eaf62ce1563b85 Mark Brown      2024-10-01  1017  
> eaf62ce1563b85 Mark Brown      2024-10-01  1018  	if (!system_supports_gcs())
> eaf62ce1563b85 Mark Brown      2024-10-01  1019  		return 0;
> eaf62ce1563b85 Mark Brown      2024-10-01  1020  
> eaf62ce1563b85 Mark Brown      2024-10-01  1021  	if (!(current->thread.gcs_el0_mode & PR_SHADOW_STACK_ENABLE))
> eaf62ce1563b85 Mark Brown      2024-10-01  1022  		return 0;
> eaf62ce1563b85 Mark Brown      2024-10-01  1023  
> eaf62ce1563b85 Mark Brown      2024-10-01  1024  	gcspr_el0 = (unsigned long __user *)read_sysreg_s(SYS_GCSPR_EL0);

Remove the cast here.

> eaf62ce1563b85 Mark Brown      2024-10-01  1025  
> eaf62ce1563b85 Mark Brown      2024-10-01  1026  	/*
> eaf62ce1563b85 Mark Brown      2024-10-01  1027  	 * Ensure that any changes to the GCS done via GCS operations
> eaf62ce1563b85 Mark Brown      2024-10-01  1028  	 * are visible to the normal reads we do to validate the
> eaf62ce1563b85 Mark Brown      2024-10-01  1029  	 * token.
> eaf62ce1563b85 Mark Brown      2024-10-01  1030  	 */
> eaf62ce1563b85 Mark Brown      2024-10-01  1031  	gcsb_dsync();
> eaf62ce1563b85 Mark Brown      2024-10-01  1032  
> eaf62ce1563b85 Mark Brown      2024-10-01  1033  	/*
> eaf62ce1563b85 Mark Brown      2024-10-01  1034  	 * GCSPR_EL0 should be pointing at a capped GCS, read the cap.
> eaf62ce1563b85 Mark Brown      2024-10-01  1035  	 * We don't enforce that this is in a GCS page, if it is not
> eaf62ce1563b85 Mark Brown      2024-10-01  1036  	 * then faults will be generated on GCS operations - the main
> eaf62ce1563b85 Mark Brown      2024-10-01  1037  	 * concern is to protect GCS pages.
> eaf62ce1563b85 Mark Brown      2024-10-01  1038  	 */
> eaf62ce1563b85 Mark Brown      2024-10-01  1039  	ret = copy_from_user(&cap, gcspr_el0, sizeof(cap));

Add one here.

> eaf62ce1563b85 Mark Brown      2024-10-01  1040  	if (ret)
> eaf62ce1563b85 Mark Brown      2024-10-01  1041  		return -EFAULT;
> eaf62ce1563b85 Mark Brown      2024-10-01  1042  
> eaf62ce1563b85 Mark Brown      2024-10-01  1043  	/*
> eaf62ce1563b85 Mark Brown      2024-10-01  1044  	 * Check that the cap is the actual GCS before replacing it.
> eaf62ce1563b85 Mark Brown      2024-10-01  1045  	 */
> eaf62ce1563b85 Mark Brown      2024-10-01 @1046  	if (!gcs_signal_cap_valid((u64)gcspr_el0, cap))

Drop the cast here.

> eaf62ce1563b85 Mark Brown      2024-10-01  1047  		return -EINVAL;
> eaf62ce1563b85 Mark Brown      2024-10-01  1048  
> eaf62ce1563b85 Mark Brown      2024-10-01  1049  	/* Invalidate the token to prevent reuse */
> eaf62ce1563b85 Mark Brown      2024-10-01  1050  	put_user_gcs(0, (__user void*)gcspr_el0, &ret);

We need a cast here if we are to go with u64 gcspr_el0 (it wasn't needed
before, not sure why it was cast to void *).

> eaf62ce1563b85 Mark Brown      2024-10-01  1051  	if (ret != 0)
> eaf62ce1563b85 Mark Brown      2024-10-01  1052  		return -EFAULT;
> eaf62ce1563b85 Mark Brown      2024-10-01  1053  
> eaf62ce1563b85 Mark Brown      2024-10-01  1054  	write_sysreg_s(gcspr_el0 + 1, SYS_GCSPR_EL0);

And this would be +8 I guess.

> eaf62ce1563b85 Mark Brown      2024-10-01  1055  
> eaf62ce1563b85 Mark Brown      2024-10-01  1056  	return 0;
> eaf62ce1563b85 Mark Brown      2024-10-01  1057  }
> eaf62ce1563b85 Mark Brown      2024-10-01  1058  

-- 
Catalin

Re: arch/arm64/kernel/signal.c:1046:36: sparse: sparse: cast removes address space '__user' of expression

[Mark Brown]

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

On Mon, Dec 09, 2024 at 03:37:23PM +0000, Catalin Marinas wrote:
> On Mon, Dec 09, 2024 at 12:47:33PM +0800, kernel test robot wrote:

> > eaf62ce1563b85 Mark Brown      2024-10-01  1014       unsigned long __user *gcspr_el0;
> 
> I think we should keep this as u64 since it's a sysreg.

Do you mean pointer to u64 or plain u64?  The value we get from the
sysreg is a pointer so it makes the uses of the value clearer if we keep
it as a pointer in C code, it seems to be defeating the point of doing
static analysis to discard the pointerness to make it happier.

> > eaf62ce1563b85 Mark Brown      2024-10-01  1050  	put_user_gcs(0, (__user void*)gcspr_el0, &ret);

> We need a cast here if we are to go with u64 gcspr_el0 (it wasn't needed
> before, not sure why it was cast to void *).

It'll have been cast to void * to add the __user at some point before
the __user annotation got added to the variable declaration.

> > eaf62ce1563b85 Mark Brown      2024-10-01  1051  	if (ret != 0)
> > eaf62ce1563b85 Mark Brown      2024-10-01  1052  		return -EFAULT;
> > eaf62ce1563b85 Mark Brown      2024-10-01  1053  
> > eaf62ce1563b85 Mark Brown      2024-10-01  1054  	write_sysreg_s(gcspr_el0 + 1, SYS_GCSPR_EL0);

> And this would be +8 I guess.

The variable is a pointer so we're doing pointer arithmetic here not
working directly with the value, unless we change the value to be purely
a u64 with no pointer in which case we would need the case above.  The
whole shambles with u64 vs unsigned long and pointer vs absolute numbers
in all the code that deals with userspace is really unhelpful :(

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

Re: arch/arm64/kernel/signal.c:1046:36: sparse: sparse: cast removes address space '__user' of expression

[Catalin Marinas]

On Mon, Dec 09, 2024 at 04:11:00PM +0000, Mark Brown wrote:
> On Mon, Dec 09, 2024 at 03:37:23PM +0000, Catalin Marinas wrote:
> > On Mon, Dec 09, 2024 at 12:47:33PM +0800, kernel test robot wrote:
> > > eaf62ce1563b85 Mark Brown      2024-10-01  1014       unsigned long __user *gcspr_el0;
> > 
> > I think we should keep this as u64 since it's a sysreg.
> 
> Do you mean pointer to u64 or plain u64? 

Plain u64.

> The value we get from the
> sysreg is a pointer so it makes the uses of the value clearer if we keep
> it as a pointer in C code, it seems to be defeating the point of doing
> static analysis to discard the pointerness to make it happier.

We have other cases where we treat a reg as u64 and convert it to
pointer as needed. While not a sysreg, the pt_regs::sp is u64 and we end
up treating it as a pointer eventually for writing the signal stack.
Another case is user_insn_read(). It's bit of bikeshedding around the
primary use in this function, do we need more conversions one way or the
other? In general I'd consider a sysreg read to be u64, especially as
the architecture has a habit of adding bits around the actual address
occasionally.

> > > eaf62ce1563b85 Mark Brown      2024-10-01  1051  	if (ret != 0)
> > > eaf62ce1563b85 Mark Brown      2024-10-01  1052  		return -EFAULT;
> > > eaf62ce1563b85 Mark Brown      2024-10-01  1053  
> > > eaf62ce1563b85 Mark Brown      2024-10-01  1054  	write_sysreg_s(gcspr_el0 + 1, SYS_GCSPR_EL0);
> 
> > And this would be +8 I guess.
> 
> The variable is a pointer so we're doing pointer arithmetic here not
> working directly with the value, unless we change the value to be purely
> a u64 with no pointer in which case we would need the case above.

That's what I meant, if we go for u64 we'll need a +8 here.

Anyway, I'd like to silence sparse on this. The u64 (non-pointer) has
some precedence in the arm64 code but, if you want, I'm happy to keep it
as a pointer (and maybe just rename it to shadow_stack or something that
does not imply a sysreg). I think for the actual warning, we can
probably fix it with a __force to silence sparse on conversion to u64.

-- 
Catalin

Re: arch/arm64/kernel/signal.c:1046:36: sparse: sparse: cast removes address space '__user' of expression

[Mark Brown]

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

On Tue, Dec 10, 2024 at 11:11:47AM +0000, Catalin Marinas wrote:
> On Mon, Dec 09, 2024 at 04:11:00PM +0000, Mark Brown wrote:

> > The value we get from the
> > sysreg is a pointer so it makes the uses of the value clearer if we keep
> > it as a pointer in C code, it seems to be defeating the point of doing
> > static analysis to discard the pointerness to make it happier.

> We have other cases where we treat a reg as u64 and convert it to
> pointer as needed. While not a sysreg, the pt_regs::sp is u64 and we end
> up treating it as a pointer eventually for writing the signal stack.
> Another case is user_insn_read(). It's bit of bikeshedding around the
> primary use in this function, do we need more conversions one way or the
> other? In general I'd consider a sysreg read to be u64, especially as
> the architecture has a habit of adding bits around the actual address
> occasionally.

The sysreg read is a u64, but we then interpret it as a pointer and do
pointer things with it for the remainder of the function until we need
to store it back in the register (which is where the warning comes from,
for some reason there's a cast in write_sysreg_s()).

> Anyway, I'd like to silence sparse on this. The u64 (non-pointer) has
> some precedence in the arm64 code but, if you want, I'm happy to keep it
> as a pointer (and maybe just rename it to shadow_stack or something that
> does not imply a sysreg). I think for the actual warning, we can
> probably fix it with a __force to silence sparse on conversion to u64.

Apparently the rune for this with sparse is to cast the value to an
unsigned long (type safety for the win!).  I wrote a patch last night,
should be done testing by now.  Given sparse we should probably have
that cast in write_sysreg_s() be to unsigned long.

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

Re: arch/arm64/kernel/signal.c:1046:36: sparse: sparse: cast removes address space '__user' of expression

[Mark Brown]

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

On Tue, Dec 10, 2024 at 12:33:42PM +0000, Mark Brown wrote:

> Apparently the rune for this with sparse is to cast the value to an
> unsigned long (type safety for the win!).  I wrote a patch last night,
> should be done testing by now.  Given sparse we should probably have
> that cast in write_sysreg_s() be to unsigned long.

Actually the patch was sent last night - it was another patch I was
waiting for:

   https://lore.kernel.org/r/20241210-arm64-gcs-signal-sparse-v1-1-26888bcd6f89@kernel.org

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