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

Commit Message

Jesse Taube June 26, 2024, 5:16 p.m. UTC

Parse the device tree for Zkr in isa string.
If Zkr is present, use it to seed the kernel base address.

Signed-off-by: Jesse Taube <[e-mail]>
---
 arch/riscv/kernel/pi/Makefile           |  2 +-
 arch/riscv/kernel/pi/archrandom_early.c | 30 ++++++++
 arch/riscv/kernel/pi/fdt_early.c        | 94 +++++++++++++++++++++++++
 arch/riscv/kernel/pi/pi.h               |  3 +
 arch/riscv/mm/init.c                    |  5 +-
 5 files changed, 132 insertions(+), 2 deletions(-)
 create mode 100644 arch/riscv/kernel/pi/archrandom_early.c

Comments

Conor Dooley June 27, 2024, 11:45 a.m. UTC | #1

Hey Jesse,

On Wed, Jun 26, 2024 at 01:16:52PM -0400, Jesse Taube wrote:
> Parse the device tree for Zkr in isa string.
> If Zkr is present, use it to seed the kernel base address.
> 
> Signed-off-by: Jesse Taube <[e-mail]>
> ---
>  arch/riscv/kernel/pi/Makefile           |  2 +-
>  arch/riscv/kernel/pi/archrandom_early.c | 30 ++++++++
>  arch/riscv/kernel/pi/fdt_early.c        | 94 +++++++++++++++++++++++++
>  arch/riscv/kernel/pi/pi.h               |  3 +
>  arch/riscv/mm/init.c                    |  5 +-
>  5 files changed, 132 insertions(+), 2 deletions(-)
>  create mode 100644 arch/riscv/kernel/pi/archrandom_early.c
> 
> diff --git a/arch/riscv/kernel/pi/Makefile b/arch/riscv/kernel/pi/Makefile
> index 1ef7584be0c3..dba902f2a538 100644
> --- a/arch/riscv/kernel/pi/Makefile
> +++ b/arch/riscv/kernel/pi/Makefile
> @@ -33,5 +33,5 @@ $(obj)/string.o: $(srctree)/lib/string.c FORCE
>  $(obj)/ctype.o: $(srctree)/lib/ctype.c FORCE
>  	$(call if_changed_rule,cc_o_c)
>  
> -obj-y		:= cmdline_early.pi.o fdt_early.pi.o string.pi.o ctype.pi.o lib-fdt.pi.o lib-fdt_ro.pi.o
> +obj-y		:= cmdline_early.pi.o fdt_early.pi.o string.pi.o ctype.pi.o lib-fdt.pi.o lib-fdt_ro.pi.o archrandom_early.pi.o
>  extra-y		:= $(patsubst %.pi.o,%.o,$(obj-y))
> diff --git a/arch/riscv/kernel/pi/archrandom_early.c b/arch/riscv/kernel/pi/archrandom_early.c
> new file mode 100644
> index 000000000000..c6261165e8a6
> --- /dev/null
> +++ b/arch/riscv/kernel/pi/archrandom_early.c
> @@ -0,0 +1,30 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +
> +#include <asm/csr.h>
> +#include <linux/processor.h>
> +
> +#include "pi.h"
> +
> +/*
> + * To avoid rewriting code include asm/archrandom.h and create macros
> + * for the functions that won't be included.
> + */
> +#undef riscv_has_extension_unlikely
> +#define riscv_has_extension_likely(...) false
> +#undef pr_err_once
> +#define pr_err_once(...)
> +
> +#include <asm/archrandom.h>
> +
> +u64 get_kaslr_seed_zkr(const uintptr_t dtb_pa)
> +{
> +	unsigned long seed = 0;
> +
> +	if (!early_isa_str((const void *)dtb_pa, "zkr"))
> +		return 0;
> +
> +	if (!csr_seed_long(&seed))
> +		return 0;
> +
> +	return seed;
> +}
> diff --git a/arch/riscv/kernel/pi/fdt_early.c b/arch/riscv/kernel/pi/fdt_early.c
> index 40ee299702bf..ba76197b44d1 100644
> --- a/arch/riscv/kernel/pi/fdt_early.c
> +++ b/arch/riscv/kernel/pi/fdt_early.c
> @@ -23,3 +23,97 @@ u64 get_kaslr_seed(uintptr_t dtb_pa)
>  	*prop = 0;
>  	return ret;
>  }
> +
> +/* Based off of fdt_stringlist_contains */
> +static int isa_string_contains(const char *strlist, int listlen, const char *str)

The variable names here are needlessly confusing IMO. The function also
returns a bool, not an int.

> +{
> +	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?

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
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" and introduce a custom function for "riscv,isa"
only.

> +		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?

> +static int fdt_node_name_eq(const void *fdt, int offset,
> +			    const char *s)
> +{
> +	int olen;
> +	int len = strlen(s);
> +	const char *p = fdt_get_name(fdt, offset, &olen);
> +
> +	if (!p || olen < len)
> +		/* short match */
> +		return 0;
> +
> +	if (memcmp(p, s, len) != 0)
> +		return 0;
> +
> +	if (p[len] == '\0')
> +		return 1;
> +	else if (!memchr(s, '@', len) && (p[len] == '@'))
> +		return 1;
> +	else
> +		return 0;
> +}
> +
> +/*
> + * Returns true if the extension is in the isa string
> + * Returns false if the extension is not found
> + */
> +static bool get_ext_named(const void *fdt, int node, const char *name)

Could you rename this function please? Having something named "get" that
returns a bool, and not an "ext_named" is odd - and it'd be self
explanatory in that case. Maybe it can just be moved into the sole caller
and isn't needed?

> +{
> +	const void *prop;
> +	int len;
> +
> +	prop = fdt_getprop(fdt, node, "riscv,isa-base", &len);
> +	if (prop && isa_string_contains(prop, len, name))
> +		return true;

This shouldn't be here, there'll not be an extension in this property.

> +	prop = fdt_getprop(fdt, node, "riscv,isa-extensions", &len);
> +	if (prop && isa_string_contains(prop, len, name))
> +		return true;
> +
> +	prop = fdt_getprop(fdt, node, "riscv,isa", &len);
> +	if (prop && isa_string_contains(prop, len, name))
> +		return true;
> +
> +	return false;
> +}
> +
> +/*
> + * 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? 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 doesn't have
them, even though it's disabled and will not be used by Linux.

> + * Returns false if the extension is not found
> + */
> +bool early_isa_str(const void *fdt, const char *ext_name)

Could you try to match the naming of the stuff used outside of pi?
Maybe early_isa_ext_available()?

Thanks for the update,
Conor.

> +{
> +	int node, parent;
> +	bool ret = false;
> +
> +	parent = fdt_path_offset(fdt, "/cpus");
> +	if (parent < 0)
> +		return false;
> +
> +	fdt_for_each_subnode(node, fdt, parent) {
> +		if (!fdt_node_name_eq(fdt, node, "cpu"))
> +			continue;
> +
> +		if (!get_ext_named(fdt, node, ext_name))
> +			return false;
> +
> +		ret = true;
> +	}
> +
> +	return ret;
> +}
> diff --git a/arch/riscv/kernel/pi/pi.h b/arch/riscv/kernel/pi/pi.h
> index 65da99466baf..26e7e5f84a30 100644
> --- a/arch/riscv/kernel/pi/pi.h
> +++ b/arch/riscv/kernel/pi/pi.h
> @@ -4,6 +4,8 @@
>  
>  #include <linux/types.h>
>  
> +bool early_isa_str(const void *fdt, const char *ext_name);
> +
>  /*
>   * The folowing functions are exported (but prefixed) declare them here so
>   * that LLVM does not complain it lacks the 'static' keyword (which, if
> @@ -11,6 +13,7 @@
>   */
>  
>  u64 get_kaslr_seed(uintptr_t dtb_pa);
> +u64 get_kaslr_seed_zkr(const uintptr_t dtb_pa);
>  bool set_nokaslr_from_cmdline(uintptr_t dtb_pa);
>  u64 set_satp_mode_from_cmdline(uintptr_t dtb_pa);
>  
> diff --git a/arch/riscv/mm/init.c b/arch/riscv/mm/init.c
> index 9940171c79f0..bfb068dc4a64 100644
> --- a/arch/riscv/mm/init.c
> +++ b/arch/riscv/mm/init.c
> @@ -1025,6 +1025,7 @@ static void __init pt_ops_set_late(void)
>  #ifdef CONFIG_RANDOMIZE_BASE
>  extern bool __init __pi_set_nokaslr_from_cmdline(uintptr_t dtb_pa);
>  extern u64 __init __pi_get_kaslr_seed(uintptr_t dtb_pa);
> +extern u64 __init __pi_get_kaslr_seed_zkr(const uintptr_t dtb_pa);
>  
>  static int __init print_nokaslr(char *p)
>  {
> @@ -1045,10 +1046,12 @@ asmlinkage void __init setup_vm(uintptr_t dtb_pa)
>  
>  #ifdef CONFIG_RANDOMIZE_BASE
>  	if (!__pi_set_nokaslr_from_cmdline(dtb_pa)) {
> -		u64 kaslr_seed = __pi_get_kaslr_seed(dtb_pa);
> +		u64 kaslr_seed = __pi_get_kaslr_seed_zkr(dtb_pa);
>  		u32 kernel_size = (uintptr_t)(&_end) - (uintptr_t)(&_start);
>  		u32 nr_pos;
>  
> +		if (kaslr_seed == 0)
> +			kaslr_seed = __pi_get_kaslr_seed(dtb_pa);
>  		/*
>  		 * Compute the number of positions available: we are limited
>  		 * by the early page table that only has one PUD and we must
> -- 
> 2.45.2
>

Jesse Taube June 27, 2024, 5:55 p.m. UTC | #2

On 6/27/24 07:45, Conor Dooley wrote:
> Hey Jesse,
> 
> On Wed, Jun 26, 2024 at 01:16:52PM -0400, Jesse Taube wrote:
>> Parse the device tree for Zkr in isa string.
>> If Zkr is present, use it to seed the kernel base address.
>>
>> Signed-off-by: Jesse Taube <[e-mail]>
>> ---
>>   arch/riscv/kernel/pi/Makefile           |  2 +-
>>   arch/riscv/kernel/pi/archrandom_early.c | 30 ++++++++
>>   arch/riscv/kernel/pi/fdt_early.c        | 94 +++++++++++++++++++++++++
>>   arch/riscv/kernel/pi/pi.h               |  3 +
>>   arch/riscv/mm/init.c                    |  5 +-
>>   5 files changed, 132 insertions(+), 2 deletions(-)
>>   create mode 100644 arch/riscv/kernel/pi/archrandom_early.c
>>
>> diff --git a/arch/riscv/kernel/pi/Makefile b/arch/riscv/kernel/pi/Makefile
>> index 1ef7584be0c3..dba902f2a538 100644
>> --- a/arch/riscv/kernel/pi/Makefile
>> +++ b/arch/riscv/kernel/pi/Makefile
>> @@ -33,5 +33,5 @@ $(obj)/string.o: $(srctree)/lib/string.c FORCE
>>   $(obj)/ctype.o: $(srctree)/lib/ctype.c FORCE
>>   	$(call if_changed_rule,cc_o_c)
>>   
>> -obj-y		:= cmdline_early.pi.o fdt_early.pi.o string.pi.o ctype.pi.o lib-fdt.pi.o lib-fdt_ro.pi.o
>> +obj-y		:= cmdline_early.pi.o fdt_early.pi.o string.pi.o ctype.pi.o lib-fdt.pi.o lib-fdt_ro.pi.o archrandom_early.pi.o
>>   extra-y		:= $(patsubst %.pi.o,%.o,$(obj-y))
>> diff --git a/arch/riscv/kernel/pi/archrandom_early.c b/arch/riscv/kernel/pi/archrandom_early.c
>> new file mode 100644
>> index 000000000000..c6261165e8a6
>> --- /dev/null
>> +++ b/arch/riscv/kernel/pi/archrandom_early.c
>> @@ -0,0 +1,30 @@
>> +// SPDX-License-Identifier: GPL-2.0-only
>> +
>> +#include <asm/csr.h>
>> +#include <linux/processor.h>
>> +
>> +#include "pi.h"
>> +
>> +/*
>> + * To avoid rewriting code include asm/archrandom.h and create macros
>> + * for the functions that won't be included.
>> + */
>> +#undef riscv_has_extension_unlikely
>> +#define riscv_has_extension_likely(...) false
>> +#undef pr_err_once
>> +#define pr_err_once(...)
>> +
>> +#include <asm/archrandom.h>
>> +
>> +u64 get_kaslr_seed_zkr(const uintptr_t dtb_pa)
>> +{
>> +	unsigned long seed = 0;
>> +
>> +	if (!early_isa_str((const void *)dtb_pa, "zkr"))
>> +		return 0;
>> +
>> +	if (!csr_seed_long(&seed))
>> +		return 0;
>> +
>> +	return seed;
>> +}
>> diff --git a/arch/riscv/kernel/pi/fdt_early.c b/arch/riscv/kernel/pi/fdt_early.c
>> index 40ee299702bf..ba76197b44d1 100644
>> --- a/arch/riscv/kernel/pi/fdt_early.c
>> +++ b/arch/riscv/kernel/pi/fdt_early.c
>> @@ -23,3 +23,97 @@ u64 get_kaslr_seed(uintptr_t dtb_pa)
>>   	*prop = 0;
>>   	return ret;
>>   }
>> +
>> +/* Based off of fdt_stringlist_contains */
>> +static int isa_string_contains(const char *strlist, int listlen, const char *str)
> 
> The variable names here are needlessly confusing IMO. The function also
> returns a bool, not an int.
> 
>> +{
>> +	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? I will try to copy how cpufeature.c as close 
as posible.

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

> 
>> +static int fdt_node_name_eq(const void *fdt, int offset,
>> +			    const char *s)
>> +{
>> +	int olen;
>> +	int len = strlen(s);
>> +	const char *p = fdt_get_name(fdt, offset, &olen);
>> +
>> +	if (!p || olen < len)
>> +		/* short match */
>> +		return 0;
>> +
>> +	if (memcmp(p, s, len) != 0)
>> +		return 0;
>> +
>> +	if (p[len] == '\0')
>> +		return 1;
>> +	else if (!memchr(s, '@', len) && (p[len] == '@'))
>> +		return 1;
>> +	else
>> +		return 0;
>> +}
>> +
>> +/*
>> + * Returns true if the extension is in the isa string
>> + * Returns false if the extension is not found
>> + */
>> +static bool get_ext_named(const void *fdt, int node, const char *name)
> 
> Could you rename this function please?

Yes.

> Having something named "get" that
> returns a bool, and not an "ext_named" is odd - and it'd be self
> explanatory in that case. Maybe it can just be moved into the sole caller
> and isn't needed?

I have it as a seperate function if in the future there needed to be a 
per cpu check.

>> +{
>> +	const void *prop;
>> +	int len;
>> +
>> +	prop = fdt_getprop(fdt, node, "riscv,isa-base", &len);
>> +	if (prop && isa_string_contains(prop, len, name))
>> +		return true;
> 
> This shouldn't be here, there'll not be an extension in this property.
Will remove.

> 
>> +	prop = fdt_getprop(fdt, node, "riscv,isa-extensions", &len);
>> +	if (prop && isa_string_contains(prop, len, name))
>> +		return true;
>> +
>> +	prop = fdt_getprop(fdt, node, "riscv,isa", &len);
>> +	if (prop && isa_string_contains(prop, len, name))
>> +		return true;
>> +
>> +	return false;
>> +}
>> +
>> +/*
>> + * 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?

> doesn't have
> them, even though it's disabled and will not be used by Linux.
> 
>> + * Returns false if the extension is not found
>> + */
>> +bool early_isa_str(const void *fdt, const char *ext_name)
> 
> Could you try to match the naming of the stuff used outside of pi?
> Maybe early_isa_ext_available()?

Yes.

Thanks,
Jesse Taube

> 
> Thanks for the update,
> Conor.
> 
>> +{
>> +	int node, parent;
>> +	bool ret = false;
>> +
>> +	parent = fdt_path_offset(fdt, "/cpus");
>> +	if (parent < 0)
>> +		return false;
>> +
>> +	fdt_for_each_subnode(node, fdt, parent) {
>> +		if (!fdt_node_name_eq(fdt, node, "cpu"))
>> +			continue;
>> +
>> +		if (!get_ext_named(fdt, node, ext_name))
>> +			return false;
>> +
>> +		ret = true;
>> +	}
>> +
>> +	return ret;
>> +}
>> diff --git a/arch/riscv/kernel/pi/pi.h b/arch/riscv/kernel/pi/pi.h
>> index 65da99466baf..26e7e5f84a30 100644
>> --- a/arch/riscv/kernel/pi/pi.h
>> +++ b/arch/riscv/kernel/pi/pi.h
>> @@ -4,6 +4,8 @@
>>   
>>   #include <linux/types.h>
>>   
>> +bool early_isa_str(const void *fdt, const char *ext_name);
>> +
>>   /*
>>    * The folowing functions are exported (but prefixed) declare them here so
>>    * that LLVM does not complain it lacks the 'static' keyword (which, if
>> @@ -11,6 +13,7 @@
>>    */
>>   
>>   u64 get_kaslr_seed(uintptr_t dtb_pa);
>> +u64 get_kaslr_seed_zkr(const uintptr_t dtb_pa);
>>   bool set_nokaslr_from_cmdline(uintptr_t dtb_pa);
>>   u64 set_satp_mode_from_cmdline(uintptr_t dtb_pa);
>>   
>> diff --git a/arch/riscv/mm/init.c b/arch/riscv/mm/init.c
>> index 9940171c79f0..bfb068dc4a64 100644
>> --- a/arch/riscv/mm/init.c
>> +++ b/arch/riscv/mm/init.c
>> @@ -1025,6 +1025,7 @@ static void __init pt_ops_set_late(void)
>>   #ifdef CONFIG_RANDOMIZE_BASE
>>   extern bool __init __pi_set_nokaslr_from_cmdline(uintptr_t dtb_pa);
>>   extern u64 __init __pi_get_kaslr_seed(uintptr_t dtb_pa);
>> +extern u64 __init __pi_get_kaslr_seed_zkr(const uintptr_t dtb_pa);
>>   
>>   static int __init print_nokaslr(char *p)
>>   {
>> @@ -1045,10 +1046,12 @@ asmlinkage void __init setup_vm(uintptr_t dtb_pa)
>>   
>>   #ifdef CONFIG_RANDOMIZE_BASE
>>   	if (!__pi_set_nokaslr_from_cmdline(dtb_pa)) {
>> -		u64 kaslr_seed = __pi_get_kaslr_seed(dtb_pa);
>> +		u64 kaslr_seed = __pi_get_kaslr_seed_zkr(dtb_pa);
>>   		u32 kernel_size = (uintptr_t)(&_end) - (uintptr_t)(&_start);
>>   		u32 nr_pos;
>>   
>> +		if (kaslr_seed == 0)
>> +			kaslr_seed = __pi_get_kaslr_seed(dtb_pa);
>>   		/*
>>   		 * Compute the number of positions available: we are limited
>>   		 * by the early page table that only has one PUD and we must
>> -- 
>> 2.45.2
>>

Conor Dooley June 27, 2024, 6:11 p.m. UTC | #3

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.

Alexandre Ghiti June 28, 2024, 9:32 a.m. UTC | #4

Hi Jesse,

On Wed, Jun 26, 2024 at 7:17 PM Jesse Taube <[e-mail]> wrote:
>
> Parse the device tree for Zkr in isa string.
> If Zkr is present, use it to seed the kernel base address.

It would be nice to explain why we finally decided to only use the Zkr
extension in !EFI platform, if @Ard Biesheuvel agrees, I guess you can
even quote him: "On an ACPI+EFI system, you have to be able to trust
the firmware. And you have to use the available abstractions.
Otherwise, there is no point."

>
> Signed-off-by: Jesse Taube <[e-mail]>
> ---
>  arch/riscv/kernel/pi/Makefile           |  2 +-
>  arch/riscv/kernel/pi/archrandom_early.c | 30 ++++++++
>  arch/riscv/kernel/pi/fdt_early.c        | 94 +++++++++++++++++++++++++
>  arch/riscv/kernel/pi/pi.h               |  3 +
>  arch/riscv/mm/init.c                    |  5 +-
>  5 files changed, 132 insertions(+), 2 deletions(-)
>  create mode 100644 arch/riscv/kernel/pi/archrandom_early.c
>
> diff --git a/arch/riscv/kernel/pi/Makefile b/arch/riscv/kernel/pi/Makefile
> index 1ef7584be0c3..dba902f2a538 100644
> --- a/arch/riscv/kernel/pi/Makefile
> +++ b/arch/riscv/kernel/pi/Makefile
> @@ -33,5 +33,5 @@ $(obj)/string.o: $(srctree)/lib/string.c FORCE
>  $(obj)/ctype.o: $(srctree)/lib/ctype.c FORCE
>         $(call if_changed_rule,cc_o_c)
>
> -obj-y          := cmdline_early.pi.o fdt_early.pi.o string.pi.o ctype.pi.o lib-fdt.pi.o lib-fdt_ro.pi.o
> +obj-y          := cmdline_early.pi.o fdt_early.pi.o string.pi.o ctype.pi.o lib-fdt.pi.o lib-fdt_ro.pi.o archrandom_early.pi.o
>  extra-y                := $(patsubst %.pi.o,%.o,$(obj-y))
> diff --git a/arch/riscv/kernel/pi/archrandom_early.c b/arch/riscv/kernel/pi/archrandom_early.c
> new file mode 100644
> index 000000000000..c6261165e8a6
> --- /dev/null
> +++ b/arch/riscv/kernel/pi/archrandom_early.c
> @@ -0,0 +1,30 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +
> +#include <asm/csr.h>
> +#include <linux/processor.h>
> +
> +#include "pi.h"
> +
> +/*
> + * To avoid rewriting code include asm/archrandom.h and create macros
> + * for the functions that won't be included.
> + */
> +#undef riscv_has_extension_unlikely
> +#define riscv_has_extension_likely(...) false
> +#undef pr_err_once
> +#define pr_err_once(...)
> +
> +#include <asm/archrandom.h>
> +
> +u64 get_kaslr_seed_zkr(const uintptr_t dtb_pa)
> +{
> +       unsigned long seed = 0;
> +
> +       if (!early_isa_str((const void *)dtb_pa, "zkr"))
> +               return 0;
> +
> +       if (!csr_seed_long(&seed))
> +               return 0;
> +
> +       return seed;
> +}
> diff --git a/arch/riscv/kernel/pi/fdt_early.c b/arch/riscv/kernel/pi/fdt_early.c
> index 40ee299702bf..ba76197b44d1 100644
> --- a/arch/riscv/kernel/pi/fdt_early.c
> +++ b/arch/riscv/kernel/pi/fdt_early.c
> @@ -23,3 +23,97 @@ u64 get_kaslr_seed(uintptr_t dtb_pa)
>         *prop = 0;
>         return ret;
>  }
> +
> +/* 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;
> +               p = memchr(strlist, '_', listlen);
> +               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_ */
> +static int fdt_node_name_eq(const void *fdt, int offset,
> +                           const char *s)
> +{
> +       int olen;
> +       int len = strlen(s);
> +       const char *p = fdt_get_name(fdt, offset, &olen);
> +
> +       if (!p || olen < len)
> +               /* short match */
> +               return 0;
> +
> +       if (memcmp(p, s, len) != 0)
> +               return 0;
> +
> +       if (p[len] == '\0')
> +               return 1;
> +       else if (!memchr(s, '@', len) && (p[len] == '@'))
> +               return 1;
> +       else
> +               return 0;
> +}
> +
> +/*
> + * Returns true if the extension is in the isa string
> + * Returns false if the extension is not found
> + */
> +static bool get_ext_named(const void *fdt, int node, const char *name)
> +{
> +       const void *prop;
> +       int len;
> +
> +       prop = fdt_getprop(fdt, node, "riscv,isa-base", &len);
> +       if (prop && isa_string_contains(prop, len, name))
> +               return true;
> +
> +       prop = fdt_getprop(fdt, node, "riscv,isa-extensions", &len);
> +       if (prop && isa_string_contains(prop, len, name))
> +               return true;
> +
> +       prop = fdt_getprop(fdt, node, "riscv,isa", &len);
> +       if (prop && isa_string_contains(prop, len, name))
> +               return true;
> +
> +       return false;
> +}
> +
> +/*
> + * Returns true if the extension is in the isa string on all cpus
> + * Returns false if the extension is not found
> + */
> +bool early_isa_str(const void *fdt, const char *ext_name)
> +{
> +       int node, parent;
> +       bool ret = false;
> +
> +       parent = fdt_path_offset(fdt, "/cpus");
> +       if (parent < 0)
> +               return false;
> +
> +       fdt_for_each_subnode(node, fdt, parent) {
> +               if (!fdt_node_name_eq(fdt, node, "cpu"))
> +                       continue;
> +
> +               if (!get_ext_named(fdt, node, ext_name))
> +                       return false;
> +
> +               ret = true;
> +       }
> +
> +       return ret;
> +}
> diff --git a/arch/riscv/kernel/pi/pi.h b/arch/riscv/kernel/pi/pi.h
> index 65da99466baf..26e7e5f84a30 100644
> --- a/arch/riscv/kernel/pi/pi.h
> +++ b/arch/riscv/kernel/pi/pi.h
> @@ -4,6 +4,8 @@
>
>  #include <linux/types.h>
>
> +bool early_isa_str(const void *fdt, const char *ext_name);
> +
>  /*
>   * The folowing functions are exported (but prefixed) declare them here so
>   * that LLVM does not complain it lacks the 'static' keyword (which, if
> @@ -11,6 +13,7 @@
>   */
>
>  u64 get_kaslr_seed(uintptr_t dtb_pa);
> +u64 get_kaslr_seed_zkr(const uintptr_t dtb_pa);
>  bool set_nokaslr_from_cmdline(uintptr_t dtb_pa);
>  u64 set_satp_mode_from_cmdline(uintptr_t dtb_pa);
>
> diff --git a/arch/riscv/mm/init.c b/arch/riscv/mm/init.c
> index 9940171c79f0..bfb068dc4a64 100644
> --- a/arch/riscv/mm/init.c
> +++ b/arch/riscv/mm/init.c
> @@ -1025,6 +1025,7 @@ static void __init pt_ops_set_late(void)
>  #ifdef CONFIG_RANDOMIZE_BASE
>  extern bool __init __pi_set_nokaslr_from_cmdline(uintptr_t dtb_pa);
>  extern u64 __init __pi_get_kaslr_seed(uintptr_t dtb_pa);
> +extern u64 __init __pi_get_kaslr_seed_zkr(const uintptr_t dtb_pa);
>
>  static int __init print_nokaslr(char *p)
>  {
> @@ -1045,10 +1046,12 @@ asmlinkage void __init setup_vm(uintptr_t dtb_pa)
>
>  #ifdef CONFIG_RANDOMIZE_BASE
>         if (!__pi_set_nokaslr_from_cmdline(dtb_pa)) {
> -               u64 kaslr_seed = __pi_get_kaslr_seed(dtb_pa);
> +               u64 kaslr_seed = __pi_get_kaslr_seed_zkr(dtb_pa);
>                 u32 kernel_size = (uintptr_t)(&_end) - (uintptr_t)(&_start);
>                 u32 nr_pos;
>
> +               if (kaslr_seed == 0)
> +                       kaslr_seed = __pi_get_kaslr_seed(dtb_pa);
>                 /*
>                  * Compute the number of positions available: we are limited
>                  * by the early page table that only has one PUD and we must
> --
> 2.45.2
>

Thanks,

Alex

Patch

diff --git a/arch/riscv/kernel/pi/Makefile b/arch/riscv/kernel/pi/Makefile
index 1ef7584be0c3..dba902f2a538 100644
--- a/arch/riscv/kernel/pi/Makefile
+++ b/arch/riscv/kernel/pi/Makefile
@@ -33,5 +33,5 @@  $(obj)/string.o: $(srctree)/lib/string.c FORCE
 $(obj)/ctype.o: $(srctree)/lib/ctype.c FORCE
 	$(call if_changed_rule,cc_o_c)
 
-obj-y		:= cmdline_early.pi.o fdt_early.pi.o string.pi.o ctype.pi.o lib-fdt.pi.o lib-fdt_ro.pi.o
+obj-y		:= cmdline_early.pi.o fdt_early.pi.o string.pi.o ctype.pi.o lib-fdt.pi.o lib-fdt_ro.pi.o archrandom_early.pi.o
 extra-y		:= $(patsubst %.pi.o,%.o,$(obj-y))
diff --git a/arch/riscv/kernel/pi/archrandom_early.c b/arch/riscv/kernel/pi/archrandom_early.c
new file mode 100644
index 000000000000..c6261165e8a6
--- /dev/null
+++ b/arch/riscv/kernel/pi/archrandom_early.c
@@ -0,0 +1,30 @@ 
+// SPDX-License-Identifier: GPL-2.0-only
+
+#include <asm/csr.h>
+#include <linux/processor.h>
+
+#include "pi.h"
+
+/*
+ * To avoid rewriting code include asm/archrandom.h and create macros
+ * for the functions that won't be included.
+ */
+#undef riscv_has_extension_unlikely
+#define riscv_has_extension_likely(...) false
+#undef pr_err_once
+#define pr_err_once(...)
+
+#include <asm/archrandom.h>
+
+u64 get_kaslr_seed_zkr(const uintptr_t dtb_pa)
+{
+	unsigned long seed = 0;
+
+	if (!early_isa_str((const void *)dtb_pa, "zkr"))
+		return 0;
+
+	if (!csr_seed_long(&seed))
+		return 0;
+
+	return seed;
+}
diff --git a/arch/riscv/kernel/pi/fdt_early.c b/arch/riscv/kernel/pi/fdt_early.c
index 40ee299702bf..ba76197b44d1 100644
--- a/arch/riscv/kernel/pi/fdt_early.c
+++ b/arch/riscv/kernel/pi/fdt_early.c
@@ -23,3 +23,97 @@  u64 get_kaslr_seed(uintptr_t dtb_pa)
 	*prop = 0;
 	return ret;
 }
+
+/* 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;
+		p = memchr(strlist, '_', listlen);
+		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_ */
+static int fdt_node_name_eq(const void *fdt, int offset,
+			    const char *s)
+{
+	int olen;
+	int len = strlen(s);
+	const char *p = fdt_get_name(fdt, offset, &olen);
+
+	if (!p || olen < len)
+		/* short match */
+		return 0;
+
+	if (memcmp(p, s, len) != 0)
+		return 0;
+
+	if (p[len] == '\0')
+		return 1;
+	else if (!memchr(s, '@', len) && (p[len] == '@'))
+		return 1;
+	else
+		return 0;
+}
+
+/*
+ * Returns true if the extension is in the isa string
+ * Returns false if the extension is not found
+ */
+static bool get_ext_named(const void *fdt, int node, const char *name)
+{
+	const void *prop;
+	int len;
+
+	prop = fdt_getprop(fdt, node, "riscv,isa-base", &len);
+	if (prop && isa_string_contains(prop, len, name))
+		return true;
+
+	prop = fdt_getprop(fdt, node, "riscv,isa-extensions", &len);
+	if (prop && isa_string_contains(prop, len, name))
+		return true;
+
+	prop = fdt_getprop(fdt, node, "riscv,isa", &len);
+	if (prop && isa_string_contains(prop, len, name))
+		return true;
+
+	return false;
+}
+
+/*
+ * Returns true if the extension is in the isa string on all cpus
+ * Returns false if the extension is not found
+ */
+bool early_isa_str(const void *fdt, const char *ext_name)
+{
+	int node, parent;
+	bool ret = false;
+
+	parent = fdt_path_offset(fdt, "/cpus");
+	if (parent < 0)
+		return false;
+
+	fdt_for_each_subnode(node, fdt, parent) {
+		if (!fdt_node_name_eq(fdt, node, "cpu"))
+			continue;
+
+		if (!get_ext_named(fdt, node, ext_name))
+			return false;
+
+		ret = true;
+	}
+
+	return ret;
+}
diff --git a/arch/riscv/kernel/pi/pi.h b/arch/riscv/kernel/pi/pi.h
index 65da99466baf..26e7e5f84a30 100644
--- a/arch/riscv/kernel/pi/pi.h
+++ b/arch/riscv/kernel/pi/pi.h
@@ -4,6 +4,8 @@ 
 
 #include <linux/types.h>
 
+bool early_isa_str(const void *fdt, const char *ext_name);
+
 /*
  * The folowing functions are exported (but prefixed) declare them here so
  * that LLVM does not complain it lacks the 'static' keyword (which, if
@@ -11,6 +13,7 @@ 
  */
 
 u64 get_kaslr_seed(uintptr_t dtb_pa);
+u64 get_kaslr_seed_zkr(const uintptr_t dtb_pa);
 bool set_nokaslr_from_cmdline(uintptr_t dtb_pa);
 u64 set_satp_mode_from_cmdline(uintptr_t dtb_pa);
 
diff --git a/arch/riscv/mm/init.c b/arch/riscv/mm/init.c
index 9940171c79f0..bfb068dc4a64 100644
--- a/arch/riscv/mm/init.c
+++ b/arch/riscv/mm/init.c
@@ -1025,6 +1025,7 @@  static void __init pt_ops_set_late(void)
 #ifdef CONFIG_RANDOMIZE_BASE
 extern bool __init __pi_set_nokaslr_from_cmdline(uintptr_t dtb_pa);
 extern u64 __init __pi_get_kaslr_seed(uintptr_t dtb_pa);
+extern u64 __init __pi_get_kaslr_seed_zkr(const uintptr_t dtb_pa);
 
 static int __init print_nokaslr(char *p)
 {
@@ -1045,10 +1046,12 @@  asmlinkage void __init setup_vm(uintptr_t dtb_pa)
 
 #ifdef CONFIG_RANDOMIZE_BASE
 	if (!__pi_set_nokaslr_from_cmdline(dtb_pa)) {
-		u64 kaslr_seed = __pi_get_kaslr_seed(dtb_pa);
+		u64 kaslr_seed = __pi_get_kaslr_seed_zkr(dtb_pa);
 		u32 kernel_size = (uintptr_t)(&_end) - (uintptr_t)(&_start);
 		u32 nr_pos;
 
+		if (kaslr_seed == 0)
+			kaslr_seed = __pi_get_kaslr_seed(dtb_pa);
 		/*
 		 * Compute the number of positions available: we are limited
 		 * by the early page table that only has one PUD and we must