| Message ID | 20241210-arm64-gcs-signal-sparse-v1-1-26888bcd6f89@kernel.org (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | arm64/signal: Silence spurious sparse warning storing GCSPR_EL0 | expand |
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?
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.
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> > >
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.
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;
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.
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.
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.
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; }
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(-) --- base-commit: fac04efc5c793dccbd07e2d59af9f90b7fc0dca4 change-id: 20241209-arm64-gcs-signal-sparse-53fa9cad67f7 Best regards,