Message-ID: <20240601-powdering-herbicide-7bfe85717536@spud>
Date: Sat, 1 Jun 2024 14:22:56 +0100
From: Conor Dooley <conor@...nel.org>
To: Charlie Jenkins <charlie@...osinc.com>
Cc: Jesse Taube <jesse@...osinc.com>, linux-riscv@...ts.infradead.org,
	linux-kernel@...r.kernel.org, llvm@...ts.linux.dev,
	Alexandre Ghiti <alexghiti@...osinc.com>,
	Palmer Dabbelt <palmer@...belt.com>,
	Albert Ou <aou@...s.berkeley.edu>,
	Björn Töpel <bjorn@...osinc.com>,
	Paul Walmsley <paul.walmsley@...ive.com>,
	Nathan Chancellor <nathan@...nel.org>,
	Nick Desaulniers <ndesaulniers@...gle.com>,
	Masahiro Yamada <masahiroy@...nel.org>
Subject: Re: [PATCH v0] RISC-V: Use Zkr to seed KASLR base address

On Fri, May 31, 2024 at 02:40:05PM -0700, Charlie Jenkins wrote:
> On Fri, May 31, 2024 at 10:36:46PM +0100, Conor Dooley wrote:
> > On Fri, May 31, 2024 at 01:19:14PM -0700, Charlie Jenkins wrote:
> > > On Fri, May 31, 2024 at 06:31:09PM +0100, Conor Dooley wrote:
> > > > On Fri, May 31, 2024 at 12:23:27PM -0400, Jesse Taube wrote:
> > > > > Dectect the Zkr extension and use it to seed the kernel base address.
> > > > > 
> > > > > Detection of the extension can not be done in the typical fashion, as
> > > > > this is very early in the boot process. Instead, add a trap handler
> > > > > and run it to see if the extension is present.
> > > > 
> > > > You can't rely on the lack of a trap meaning that Zkr is present unless
> > > > you know that the platform implements Ssstrict. The CSR with that number
> > > > could do anything if not Ssstrict compliant, so this approach gets a
> > > > nak from me. Unfortunately, Ssstrict doesn't provide a way to detect
> > > > it, so you're stuck with getting that information from firmware.
> > > 
> > > The Scalar Cryptography v1.0.1 spec says "If Zkr is not implemented, the
> > > [s,u]seed bits must be hardwired to zero". It also says "Without the
> > > corresponding access control bit set to 1, any attempted access to seed
> > > from U, S, or HS modes will raise an illegal instruction exception."
> > > 
> > > There is a slight nuance here as the definition of Ssstrict is:
> > > 
> > > "No non-conforming extensions are present. Attempts to execute
> > > unimplemented opcodes or access unimplemented CSRs in the standard or
> > > reserved encoding spaces raises an illegal instruction exception that
> > > results in a contained trap to the supervisor-mode trap handler."
> > > 
> > > The trap that Jesse is relying on in the code here is related to access
> > > bits and not related to the CSR being unimplemented. Since the access
> > > bits are required to be 0 on an implementation without Zkr, it is
> > > required to trap if seed is accessed, regardless of Ssstrict.
> > > 
> > > The situation here is slightly odd because the spec is defining behavior
> > > for what to do if the extension is not supported, and requires
> > > implementations to follow this aspect of the Scalar Cryptography spec
> > > even though they may not implement any of the instructions in the spec.
> > 
> > Firstly, you absolutely cannot rely on the behaviour defined by a new
> > extension by systems that implement a version of the ISA that predates
> > it. Secondly, we're talking about non-conforming implementations that
> > use a reserved CSR number for other purposes, you cannot rely on the
> > behaviour that the Scalar Crypto spec prescribes for access to the
> > register.
> 
> Yes that is definitely a slippery slope.
> 
> > 
> > "Non-conforming" is also a moving target btw - the Andes PMU (I think
> > it's that) uses an interrupt number that was moved from "platform
> > specific use" to "reserved" by the AIA spec. If you only looked the
> > current specs, the Andes PMU is a "non-conforming extension" but at the
> > time that it was created it was compliant. I think we're gonna have a
> > fun conversation defining what "Ssstrict" actually means when someone
> > actually tries to document that.
> 
> Sounds fun ;)
> 
> > 
> > > > For DT systems, you can actually parse the DT in the pi, we do it to get
> > > > the kaslr seed if present, so you can actually check for Zkr. With ACPI
> > > > I have no idea how you can get that information, I amn't an ACPI-ist.
> > > 
> > > It is feasible to check if Zkr is present in the device tree at this
> > > stage in boot, but at first glance does not seem feasible to read the
> > > ACPI tables this early.
> > > 
> > > The CSR being read is just for entropy so even if the seed CSR doesn't
> > > trap and provides an arbitrary value on an implementation that does not
> > > support Zkr, it can still be used as a source of entropy. If the
> > > implementation does trap, the entropy will be set to 0 which is just a
> > > different hard-coded arbitrary value. 
> > 
> > Right. I can see value in doing something that may contain entropy, and
> > is at worst no better than the 0 we can currently get. But the patch
> > we're talking about here mentions nothing of the sort, it presents itself
> > as detection of Zkr and an actually random number - but all it actually
> > detects is whether or not the CSR at CSR_SEED traps.
> > 
> > To be acceptable, the patch would need to stop claiming that it is a valid
> > way to detect Zkr. The commit message, and a comment, must also explain
> > what may happen on systems that do not implement Zkr as you have done
> > here.
> > For example, `if (!riscv_has_zkr()) return 0` would have to be something
> > like `if (riscv_csr_seed_traps()) return 0`.
> 
> That is reasonable, thank you for your input!

Another thing to consider is that writing a zero to CSR_SEED may have
side effects on a non-conforming implementation.

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

Powered by blists - more mailing lists