Message-ID: <20240531-cough-yearling-bdfd49244885@spud>
Date: Fri, 31 May 2024 22:36:46 +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 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.

"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.

> > 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`.

Thanks,
Conor.

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

Powered by blists - more mailing lists