linux-kernel - Re: [PATCH] arm64/signal: Silence spurious sparse warning storing GCSPR

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <Z1ht7X2LRw34pMJK@J2N7QTR9R3>
Date: Tue, 10 Dec 2024 16:35:57 +0000
From: Mark Rutland <mark.rutland@....com>
To: Mark Brown <broonie@...nel.org>
Cc: Catalin Marinas <catalin.marinas@....com>,
	Will Deacon <will@...nel.org>, linux-arm-kernel@...ts.infradead.org,
	linux-kernel@...r.kernel.org, kernel test robot <lkp@...el.com>
Subject: Re: [PATCH] arm64/signal: Silence spurious sparse warning storing
 GCSPR_EL0

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;