linux-kernel - Re: arch/arm64/kernel/signal.c:1046:36: sparse: sparse: cast removes address space '_

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 for Android: free password hash cracker in your pocket

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

Message-ID: <Z1gh809HUGRZI258@arm.com>
Date: Tue, 10 Dec 2024 11:11:47 +0000
From: Catalin Marinas <catalin.marinas@....com>
To: Mark Brown <broonie@...nel.org>
Cc: kernel test robot <lkp@...el.com>, oe-kbuild-all@...ts.linux.dev,
	linux-kernel@...r.kernel.org,
	Thiago Jung Bauermann <thiago.bauermann@...aro.org>
Subject: Re: arch/arm64/kernel/signal.c:1046:36: sparse: sparse: cast removes
 address space '__user' of expression

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