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

[Clément Léger]

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