riscv: supports_extension() broken for long isa strings

[Heinrich Schuchardt]

On 21.02.24 18:59, Conor Dooley wrote:
> 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)

Thanks Conor for reporting the issue. We should change all cpu_get_desc
implementations to:

int riscv_cpu_get_desc(const struct udevice *dev, char *buf, size_t *size)
{
	size_t old_size = *size;

	*size = snprintf(buf, *size, "%s", info) + 1;
	if (*size > old_size)
		return -ENOSPC;
	return 0;
}

With this change

size = 0;
cpu_get_desc(dev, desc, &size);

can be used to get the size of the information before allocating a buffer.

desc = malloc(size);
cpu_get_desc(dev, desc, size);

Best regards

Heinrich

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