* arch/arm64/kernel/signal.c:1046:36: sparse: sparse: cast removes address space '__user' of expression
@ 2024-12-09 4:47 kernel test robot
2024-12-09 15:37 ` Catalin Marinas
0 siblings, 1 reply; 6+ messages in thread
From: kernel test robot @ 2024-12-09 4:47 UTC (permalink / raw)
To: Mark Brown
Cc: oe-kbuild-all, linux-kernel, Catalin Marinas,
Thiago Jung Bauermann
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
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: arch/arm64/kernel/signal.c:1046:36: sparse: sparse: cast removes address space '__user' of expression
2024-12-09 4:47 arch/arm64/kernel/signal.c:1046:36: sparse: sparse: cast removes address space '__user' of expression kernel test robot
@ 2024-12-09 15:37 ` Catalin Marinas
2024-12-09 16:11 ` Mark Brown
0 siblings, 1 reply; 6+ messages in thread
From: Catalin Marinas @ 2024-12-09 15:37 UTC (permalink / raw)
To: kernel test robot
Cc: Mark Brown, oe-kbuild-all, linux-kernel, Thiago Jung Bauermann
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
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: arch/arm64/kernel/signal.c:1046:36: sparse: sparse: cast removes address space '__user' of expression
2024-12-09 15:37 ` Catalin Marinas
@ 2024-12-09 16:11 ` Mark Brown
2024-12-10 11:11 ` Catalin Marinas
0 siblings, 1 reply; 6+ messages in thread
From: Mark Brown @ 2024-12-09 16:11 UTC (permalink / raw)
To: Catalin Marinas
Cc: kernel test robot, oe-kbuild-all, linux-kernel,
Thiago Jung Bauermann
[-- 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 --]
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: arch/arm64/kernel/signal.c:1046:36: sparse: sparse: cast removes address space '__user' of expression
2024-12-09 16:11 ` Mark Brown
@ 2024-12-10 11:11 ` Catalin Marinas
2024-12-10 12:33 ` Mark Brown
0 siblings, 1 reply; 6+ messages in thread
From: Catalin Marinas @ 2024-12-10 11:11 UTC (permalink / raw)
To: Mark Brown
Cc: kernel test robot, oe-kbuild-all, linux-kernel,
Thiago Jung Bauermann
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: arch/arm64/kernel/signal.c:1046:36: sparse: sparse: cast removes address space '__user' of expression
2024-12-10 11:11 ` Catalin Marinas
@ 2024-12-10 12:33 ` Mark Brown
2024-12-10 12:49 ` Mark Brown
0 siblings, 1 reply; 6+ messages in thread
From: Mark Brown @ 2024-12-10 12:33 UTC (permalink / raw)
To: Catalin Marinas
Cc: kernel test robot, oe-kbuild-all, linux-kernel,
Thiago Jung Bauermann
[-- 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 --]
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: arch/arm64/kernel/signal.c:1046:36: sparse: sparse: cast removes address space '__user' of expression
2024-12-10 12:33 ` Mark Brown
@ 2024-12-10 12:49 ` Mark Brown
0 siblings, 0 replies; 6+ messages in thread
From: Mark Brown @ 2024-12-10 12:49 UTC (permalink / raw)
To: Catalin Marinas
Cc: kernel test robot, oe-kbuild-all, linux-kernel,
Thiago Jung Bauermann
[-- 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 --]
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2024-12-10 12:49 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-12-09 4:47 arch/arm64/kernel/signal.c:1046:36: sparse: sparse: cast removes address space '__user' of expression kernel test robot
2024-12-09 15:37 ` Catalin Marinas
2024-12-09 16:11 ` Mark Brown
2024-12-10 11:11 ` Catalin Marinas
2024-12-10 12:33 ` Mark Brown
2024-12-10 12:49 ` Mark Brown
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox