Message-ID: <20240627-proven-irritably-33594282739f@wendy>
Date: Thu, 27 Jun 2024 12:45:42 +0100
From: Conor Dooley <conor.dooley@...rochip.com>
To: Jesse Taube <jesse@...osinc.com>
CC: <linux-riscv@...ts.infradead.org>, Ard Biesheuvel <ardb@...nel.org>, Paul
 Walmsley <paul.walmsley@...ive.com>, Palmer Dabbelt <palmer@...belt.com>,
	Albert Ou <aou@...s.berkeley.edu>, Nathan Chancellor <nathan@...nel.org>,
	Nick Desaulniers <ndesaulniers@...gle.com>, Bill Wendling <morbo@...gle.com>,
	Justin Stitt <justinstitt@...gle.com>, Alexandre Ghiti
	<alexghiti@...osinc.com>, Masahiro Yamada <masahiroy@...nel.org>, Wende Tan
	<twd2.me@...il.com>, Christophe JAILLET <christophe.jaillet@...adoo.fr>, Sami
 Tolvanen <samitolvanen@...gle.com>, Andrew Morton
	<akpm@...ux-foundation.org>, Baoquan He <bhe@...hat.com>, Chen Jiahao
	<chenjiahao16@...wei.com>, "Mike Rapoport (IBM)" <rppt@...nel.org>, "Vishal
 Moola (Oracle)" <vishal.moola@...il.com>, <linux-kernel@...r.kernel.org>,
	<llvm@...ts.linux.dev>
Subject: Re: [PATCH v2 3/3] RISC-V: Use Zkr to seed KASLR base address

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 <jesse@...osinc.com>
> ---
>  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
> 

Download attachment "signature.asc" of type "application/pgp-signature" (229 bytes)

Powered by blists - more mailing lists