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: <6d839dfb-0a85-44c5-90cc-2b2426353a5f@sirena.org.uk>
Date: Tue, 10 Dec 2024 16:52:49 +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 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.

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