Re: [PATCH v0] RISC-V: Use Zkr to seed KASLR base address

[Charlie Jenkins]

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!

- Charlie

> 
> Thanks,
> Conor.