Re: [PATCH v2 3/3] RISC-V: Use Zkr to seed KASLR base address

[Conor Dooley <conor@kernel.org>]

[-- Attachment #1: Type: text/plain, Size: 4320 bytes --]

On Thu, Jun 27, 2024 at 01:55:19PM -0400, Jesse Taube wrote:
> On 6/27/24 07:45, Conor Dooley wrote:
> > On Wed, Jun 26, 2024 at 01:16:52PM -0400, Jesse Taube wrote:

> > > +/* Based off of fdt_stringlist_contains */
> > > +static int isa_string_contains(const char *strlist, int listlen, const char *str)
> > > +{
> > > +	int len = strlen(str);
> > > +	const char *p;
> > > +
> > > +	while (listlen >= len) {
> > > +		if (strncasecmp(str, strlist, len) == 0)
> > > +			return 1;
> > 
> > How does this handle searching a devicetree containing "rv64ima_zksed_zkr"
> > for the extension zks? Hint: https://godbolt.org/z/YfhTqe54e
> > I think this works for fdt_stringlist_contains() because it also
> > compares the null chars - which you're not doing so I think this also
> > brakes for something like riscv,isa-extensions = "rv64ima\0zksed\0zkr"
> > while searching for zks.
> > 
> > > +		p = memchr(strlist, '_', listlen);
> > 
> > Or how does this handle searching "rv64imafdczkr" for zkr? It's gonna
> > run right off the end of the string without finding anything, right?
> 
> Yes...
> 
> Is that a valid isa,string?

It is. I wish I had just not allowed it, but I was more naive then and
figured we should allow whatever the spec did. Technically versioning of
the extension isn't allowed, but in the wild people do put it in, so I
believe that a parser shouldn't break when it encounters versioning,
even if the regex for the property doesn't permit them:
^rv(?:64|32)imaf?d?q?c?b?k?j?p?v?h?(?:[hsxz](?:[0-9a-z])+)?(?:_[hsxz](?:[0-9a-z])+)*$

> I will try to copy how cpufeature.c as close as
> posible.

The comments there should be fairly understandable, but that parser has
a different goal to the one here, so you should be able to make things
simpler. I hope at least.

> > Handling "riscv,isa" is not trivial, but at least the search for extension
> > approach here skips dealing with some of what has to be done in the "real"
> > parser with the version numbers...
> > 
> > Maybe we just say screw "riscv,isa", as it's deprecated anyway, and only.
> I think it's important to have.
> 
> > add this new feature for "riscv,isa-extensions" which is far simpler to
> > parse and can be done using off-the-shelf fdt functions?
> > 
> > If not, then I think we should use fdt_stringlist_contains verbatim for
> > "riscv,isa-extensions".
> 
> Ok I had a notion that riscv,isa-extensions could be upercase they
> cant/wont. I will use fdt_stringlist_contains.
> 
> > and introduce a custom function for "riscv,isa"
> > only.
> 
> That was my original thought I will do that.
> 
> > 
> > > +		if (!p)
> > > +			p = memchr(strlist, '\0', listlen);
> > > +		if (!p)
> > > +			return 0; /* malformed strlist.. */
> > > +		listlen -= (p - strlist) + 1;
> > > +		strlist = p + 1;
> > > +	}
> > > +
> > > +	return 0;
> > > +}
> > > +
> > > +/* Based off of fdt_nodename_eq_ */
> > 
> > Why can't we just use fdt_nodename_eq?
> 
> Because fdt_nodename_eq_ is static.
> I will change the comment to "copy of fdt_nodename_eq_".
> Oddly there is `of_node_name_eq` but not `fdt_nodename_eq`

of_node_name_eq comes from the kernel, fdt_nodename_eq comes from
libfdt. I figure the former cannot be used this early since we've not
extracted the dtb and parsed it yet...

> > > +/*
> > > + * Returns true if the extension is in the isa string on all cpus
> > 
> > Shouldn't we only be checking CPUs that are not disabled or reserved,
> > rather than all CPUs?
> 
> Its way easier to just check all the cpus rather then make sure we are
> runing on one thats has the extention. I will add a continue for
> dissabled/reserved cpus.
> 
> > To use Zkr for KASLR this is kinda irrelevant
> > since really we only care about whether or not the boot CPU has Zkr,
> > but in general we only want to consider CPUs that we can actually use.
> > For example, if you did this for FPU support with mpfs.dtsi, you'd get
> > told that the F/D extensions were not present cos hart 0
> 
> Can we assume that the boot hart is always 0?

Nah, You cannot assume that the boot hart is hart 0, in the example I gave
here hart 0 is not available to Linux.

> > doesn't have
> > them, even though it's disabled and will not be used by Linux.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]