riscv: supports_extension() broken for long isa strings

[Conor Dooley]

Yo,

I mentioned this last night to Heinrich on IRC, supports_extension() is
broken for ISA strings longer than 32 characters. M-Mode U-Boot doesn't
parse a devicetree, so this doesn't apply there, but for S-mode
supports_extension() looks like:

static inline bool supports_extension(char ext)
{

	struct udevice *dev;
	char desc[32];
	int i;

	uclass_find_first_device(UCLASS_CPU, &dev);
	if (!dev) {
		debug("unable to find the RISC-V cpu device\n");
		return false;
	}
	if (!cpu_get_desc(dev, desc, sizeof(desc))) {
		/*
		 * skip the first 4 characters (rv32|rv64) and
		 * check until underscore
		 */
		for (i = 4; i < sizeof(desc); i++) {
			if (desc[i] == '_' || desc[i] == '\0')
				break;
			if (desc[i] == ext)
				return true;
		}
	}

	return false;
}

cpu_get_desc is implemented by riscv_cpu_get_desc():
static int riscv_cpu_get_desc(const struct udevice *dev, char *buf, int size)
{
	const char *isa;

	isa = dev_read_string(dev, "riscv,isa");
	if (size < (strlen(isa) + 1))
		return -ENOSPC;

	strcpy(buf, isa);

	return 0;
}

On most extant systems, riscv,isa is a pretty short string - between 10
and 20 characters. In QEMU's default virt machine however, we get:
riscv,isa = "rv64imafdch_zicbom_zicbop_zicboz_zicntr_zicsr_zifencei_zihintntl_zihintpause_zihpm_zawrs_zfa_zca_zcd_zba_zbb_zbc_zbs_sstc_svadu";

Since desc can only contain 32 elements, the size < strlen() test fails
and cpu_get_desc() returns an error and supports_extension() in turn
returns false. 

Currently, in S-Mode, there's only two extensions that U-Boot ever looks
for and they lie inside the single letter section, so 32 charcters would
be sufficiently sized, if cpu_get_desc() supported undersized buffers.

I came across this while adding support for a different way of detecting
ISA extensions, rather than running into an actual problem because U-Boot
seems not to actually make use of supports_extension() other than enabling
an FPU that there seem to be no users of in U-Boot at present. I also
assume that using U-Boot in QEMU is somewhat of a rare case, given with
virt you can boot an OS kernel directly. That'd make the impact of this
problem pretty low, given I just happened to notice that in my test
environment no extensions were being detected and the operation of
U-Boot seemed unaffected.

I'm mostly just wondering if, given the impact seems to be rather low,
if I should "bother" making a minimal fix for this that would be
applied to master (or backported? not 100% sure of the release process
for U-Boot), or if I can just fix it in passing while making "riscv,isa"
optional?

A minimal fix would look something like the following:

diff --git a/arch/riscv/cpu/cpu.c b/arch/riscv/cpu/cpu.c
index 8445c5823e..df508ac4a1 100644
--- a/arch/riscv/cpu/cpu.c
+++ b/arch/riscv/cpu/cpu.c
@@ -39,7 +39,6 @@ static inline bool supports_extension(char ext)
 	return csr_read(CSR_MISA) & (1 << (ext - 'a'));
 #elif CONFIG_CPU
 	struct udevice *dev;
-	char desc[32];
 	int i;
 
 	uclass_find_first_device(UCLASS_CPU, &dev);
@@ -47,15 +46,16 @@ static inline bool supports_extension(char ext)
 		debug("unable to find the RISC-V cpu device\n");
 		return false;
 	}
-	if (!cpu_get_desc(dev, desc, sizeof(desc))) {
+	const char *isa = dev_read_string(dev, "riscv,isa");
+	if (isa) {
 		/*
 		 * skip the first 4 characters (rv32|rv64) and
 		 * check until underscore
 		 */
-		for (i = 4; i < sizeof(desc); i++) {
-			if (desc[i] == '_' || desc[i] == '\0')
+		for (i = 4; i < strlen(isa); i++) {
+			if (isa[i] == '_' || isa[i] == '\0')
 				break;
-			if (desc[i] == ext)
+			if (isa[i] == ext)
 				return true;
 		}
 	}

Cheers,
Conor.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 228 bytes
Desc: not available
URL: <https://lists.denx.de/pipermail/u-boot/attachments/20240221/a0130ddb/attachment.sig>