Message-ID: <20251228223430.43f14d51@pumpkin>
Date: Sun, 28 Dec 2025 22:34:30 +0000
From: David Laight <david.laight.linux@...il.com>
To: Deepak Gupta <debug@...osinc.com>
Cc: Lukas Gerlach <lukas.gerlach@...pa.de>, linux-riscv@...ts.infradead.org,
 palmer@...belt.com, pjw@...nel.org, aou@...s.berkeley.edu, alex@...ti.fr,
 linux-kernel@...r.kernel.org, daniel.weber@...pa.de,
 michael.schwarz@...pa.de, marton.bognar@...euven.be,
 jo.vanbulck@...euven.be
Subject: Re: [PATCH 1/2] riscv: Use pointer masking to limit uaccess
 speculation

On Sat, 27 Dec 2025 17:59:38 -0800
Deepak Gupta <debug@...osinc.com> wrote:

> On Sat, Dec 27, 2025 at 09:28:59PM +0000, David Laight wrote:
> >On Fri, 19 Dec 2025 16:44:11 -0800
> >Deepak Gupta <debug@...osinc.com> wrote:
> >  
> >> On Thu, Dec 18, 2025 at 08:13:31PM +0100, Lukas Gerlach wrote:  
> >> >Similarly to x86 and arm64, mitigate speculation past an access_ok()
> >> >check by masking the pointer before use.
> >> >
> >> >On RISC-V, user addresses have the MSB clear while kernel addresses
> >> >have the MSB set. The uaccess_mask_ptr() function clears the MSB,
> >> >ensuring any kernel pointer becomes invalid and will fault, while
> >> >valid user pointers remain unchanged. This prevents speculative
> >> >access to kernel memory via user copy functions.
> >> >
> >> >The masking is applied to __get_user, __put_user, raw_copy_from_user,
> >> >raw_copy_to_user, clear_user, and the unsafe_* variants.
> >> >
> >> >Signed-off-by: Lukas Gerlach <lukas.gerlach@...pa.de>
> >> >---
> >> > arch/riscv/include/asm/uaccess.h | 41 +++++++++++++++++++++++++-------
> >> > 1 file changed, 32 insertions(+), 9 deletions(-)
> >> >
> >> >diff --git a/arch/riscv/include/asm/uaccess.h b/arch/riscv/include/asm/uaccess.h
> >> >index 36bba6720c26..ceee1d62ff9b 100644
> >> >--- a/arch/riscv/include/asm/uaccess.h
> >> >+++ b/arch/riscv/include/asm/uaccess.h
> >> >@@ -74,6 +74,23 @@ static inline unsigned long __untagged_addr_remote(struct mm_struct *mm, unsigne
> >> > #define __typefits(x, type, not) \
> >> > 	__builtin_choose_expr(sizeof(x) <= sizeof(type), (unsigned type)0, not)
> >> >
> >> >+/*
> >> >+ * Sanitize a uaccess pointer such that it cannot reach any kernel address.
> >> >+ *
> >> >+ * On RISC-V, virtual addresses are sign-extended from the top implemented bit.
> >> >+ * User addresses have the MSB clear; kernel addresses have the MSB set.
> >> >+ * Clearing the MSB ensures any kernel pointer becomes non-canonical and will
> >> >+ * fault, while valid user pointers remain unchanged.
> >> >+ */
> >> >+#define uaccess_mask_ptr(ptr) ((__typeof__(ptr))__uaccess_mask_ptr(ptr))
> >> >+static inline void __user *__uaccess_mask_ptr(const void __user *ptr)
> >> >+{
> >> >+	unsigned long val = (unsigned long)ptr;
> >> >+
> >> >+	val = (val << 1) >> 1;
> >> >+	return (void __user *)val;  
> >>
> >> This is only clearing b63 which is what we don't need here.  
> >
> >It is also entirely the wrong operation.
> >A kernel address needs converting into an address that is guaranteed
> >to fault - not a user address that might be valid.  
> 
> This is about speculative accesses and not actual accesses. Due to some
> speculation it is possible that in speculative path a wrong address is
> generated with MSB=1. This simply ensures that bit is cleared for agen
> even in speculative path.

You said you were following what x86 did - this isn't what is does.
Avoiding the conditional branch in access_ok() is actually a big win.
That is true even without the issues with speculative accesses to kernel
memory.
The 'address masking' (badly named - it isn't just masking) is a replacement
for access_ok(), it changes kernel addresses to invalid ones so that the
access faults and the trap/fault fixup code detects the error.

	David

> 
> >
> >You need a guard page of address space between valid user addresses
> >and valid kernel addresses that is guaranteed to not be used.  
> 
> IIUC, risc-v does that already (last user page is unmapped and first
> kernel page is unmapped).
> 
> >Typically this is the top page of the user address space.
> >All kernel addresses need converting to the base of the guard page
> >using ALU operations (to avoid speculative accesses).
> >So: ptr = min(ptr, guard_page); easiest done (as in x86-64) with
> >a compare and conditional move.
> >The ppc version is more complex because there isn't a usable conditional
> >move instruction.
> >I think this would work for x86-32:
> >	offset = ptr + -guard_page;
> >	mask = sbb(x,x); // ~0u if the add set the carry flag.
> >	ptr -= offset & mask;
> >You need to find something that works for riscv.  
> 
> I believe what you're describing is `access_ok` in x86. `__access_ok` in
> `asm-generic/access_ok.h` should cover same behavior for risc-v.
> 
> >  
> >>
> >> You should be clearing b47 (if bit indexing starts at 0) on Sv48 and b56
> >> on Sv57 system.
> >>
> >> Anything above b47/b56 isn't going to be used anyways in indexing into
> >> page tables and will be ignored if pointer masking is enabled at S.  
> >
> >Gah more broken hardware...
> >Ignoring the high address bits doesn't work and is really a bad idea.
> >Arm did it as well.
> >Trying to do 'user pointer masking' for uaccess validation is a PITA
> >if you have to allow for non-zero values in the high bits it makes
> >life even more complicated.  
> 
> Pointer masking and preventing speculative accesses to kernel addresses
> are two different things (although they're related because they impact
> address generation part). 
> 
> Kernel may not have pointer masking enabled and in that case, it'll just
> trap if high unused bits don't match b38/47/56. To accomodate that, there
> already are patches in riscv to remove the tag in high unused bits before
> access is made by the kernel.
> 
> >
> >And 'pointer masking' is so broken unless it is done at the page level
> >and enforced by the hardware.
> >What it does is make random/invalid addresses much more likely to be valid.
> >An interpreter can use them internally, but they are so slow software
> >masking  (following the validation) shouldn't be a real issue.
> >
> >	David
> >
> >  

Powered by blists - more mailing lists