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

[Palmer Dabbelt]

On Wed, 12 Jun 2024 00:15:26 PDT (-0700), cleger@xxxxxxxxxxxx wrote:
> On 11/06/2024 17:32, Deepak Gupta wrote:
> > On Mon, Jun 10, 2024 at 10:56:35PM +0100, Conor Dooley wrote:
> > > On Mon, Jun 10, 2024 at 02:06:50PM -0700, Deepak Gupta wrote:
> > > > On Mon, Jun 10, 2024 at 11:16:42AM +0200, Clément Léger wrote:
> > > > > On 10/06/2024 11:02, Conor Dooley wrote:
> > > > > > On Mon, Jun 10, 2024 at 10:33:34AM +0200, Clément Léger wrote:
> > > > > > > On 07/06/2024 20:51, Deepak Gupta wrote:
> > > > > > > > On Mon, Jun 03, 2024 at 01:47:40PM +0100, Conor Dooley wrote:
> > > > > > > > > On Mon, Jun 03, 2024 at 11:14:49AM +0200, Alexandre Ghiti
> > > > wrote:
> > > > > > > > I don't know all the details but on first glance it seems
> > > > like instead
> > > > > > > > of ACPI,
> > > > > > > > may be FWFT is a better place for discovery ?
> > > > > > > >
> > > > https://lists.riscv.org/g/tech-prs/topic/patch_v12_add_firmware/106479571
> > > > > > >
> > > > > > > IMHO, doing discovery in FWFT is not the goal of this
> > > > extension. I think
> > > > > > > the "real" solution would be to wait for the unified discovery
> > > > task
> > > > > > > group to come up with something for that (which is their goal I
> > > > think) [1]
> > > >
> > > > Yeah I understand the conundrum here.
> > > >
> > > > > >
> > > > > > I'm curious to see how that works out. The proposal documents an
> > > > m-mode
> > > > > > csr, so we'd have to smuggle the information to s-mode somehow...
> > > > >
> > > > > Ahem, yeah, I spoke a bit too fast. Looked at the proposal and the
> > > > > mconfigptr CSR will be accessible by M-mode only so I guess we will
> > > > have
> > > > > to find another way...
> > > >
> > > > That's not the only problem. Even if you get mconfigptr access,
> > > > parsing the format
> > > > is another thing that has to be done. This is early in boot. Although
> > > > its strictly (pun
> > > > intended) not a firmware feature extension, I think it's much easier
> > > > to ask underlying
> > > > firmware if platform is `Sstrict`. or may be expose something like below
> > > >
> > > > `ENABLE_SSTRICT`.
> > > > Platforms which support `Sstrict` can return success for this while
> > > > platforms which don't
> > > > have `Sstrict` can return error code (not supported or not implemented).
> > > > This way its not feature discovery.
> > >
> > > I mean, it's feature discovery in all but name. You're calling it
> > > enable, but the behaviour you describe is feature discovery - not that I
> > > am against this being feature discovery since it gets us out of an
> > > annoying bind.
> >
> > Yes I know it's cheating but at least for this case, it seems like easy
> > solution which
> > doesn't break anything. Neither I see it creating any future problems
> > (except FWFT becoming
> > to look like discovery mechanism :-) and Clement/Atish hating me for that)
>
> Ahah no worries;) Thinking a bit more about it, if we need only a few
> extensions to be discoverable, it seems manageable (ie add a "locked"
> feature that report the extension availability itself, get only will
> work, set will return SBI_EDENIED). But if need more of them, then a
> dedicated mechanism should probably be designed.
>
> Clément
>
> > Another solution to this could be introducing a riscv config
> > `CONFIG_RISCV_SSTRICT`.
> > By default always select `CONFIG_RISCV_SSTRICT` and any platform owner
> > who are not
> > sstrict, they can build their own.
> > I expect distro (ubuntu, red hat, etc) would want by default
> > `CONFIG_RISCV_SSTRICT`.

IMO things like Sstrict is going to end up being useless.  It's just 
another way to kick the can down the road on actually having 
compatibility.  We've already seen this with the extensions and the 
profiles, I don't see how Sstrict is any different.

The RISC-V foundation needs to just actually do compatibility, making up 
more names isn't going to change anything.

> > > I forget which extension Alex and I discussed previously, but there's
> > > some mm-related things that you're gonna have to probe super early and
> > > we need to figure out if we can get that info from ACPI or not. I know
> > > we discussed it w.r.t. one of the T-Head vendor extensions, but I think
> > > another standard one got mentioned too.
> > >
> > > > It seems like arm64 parses fdt and it reads certain CSRs too
> > > > (`arch/arm64/kernel/pi/kaslr_early.c`). Although it doesn't look like
> > > > it has to do any
> > > > discovery for them.
> > >
> > > A decree from the Palmer that we don't support things that do not conform
> > > in this manner would allow us to do what arm64 does. I brought this up
> > > originally because it's been discussed before that we cannot rely on
> > > conformance because we want to support people's platforms, whether they
> > > comply or not. I'd be wary of making this an exception now, and then
> > > later on someone makes a platform we want to support that doesn't
> > > conform and hey presto, we regress KASLR support - even if I think it is
> > > pretty unlikely that someone is going to repurpose the Zkr CSRs.

Ya, I think that's fine.  Let's just blindly read the CSR without 
probing, if some hardware has destructive side effects on the CSR read 
then we can figure out what to do with it later.

> > > One of the problems with only supporting conforming platforms is that
> > > the definition of conforming changes over time! This has happened with
> > > the Andes PMU for example, which I believe uses an interrupt number that
> > > was later co-opted by AIA spec. That conformed at the time, but doesn't
> > > anymore - do they get to mark themselves as Sstrict?
> > >
> > > Maybe we can do this on a case-by-case basis, but it's up to Palmer
> > > whether or not we can do that.

Yep.  Trying to stick to only supporting what counts as conforming today 
isn't going to work, RISC-V just isn't managed that way.  That's why we 
have all these strict rules about avoiding breaking what used to work in 
software land, if we didn't have those then we be breaking users every 
release.