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: <20c12aac-193e-43ae-9418-39db1af4ede9@sirena.org.uk>
Date: Tue, 10 Dec 2024 15:44:29 +0000
From: Mark Brown <broonie@...nel.org>
To: Mark Rutland <mark.rutland@....com>
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 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.

Download attachment "signature.asc" of type "application/pgp-signature" (489 bytes)