linux-kernel - Re: [PATCH 2/2] RISC-V: make use of variadic sbi

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-Id: <DALGSCDW0GIG.10I22KD2SCSNX@ventanamicro.com>
Date: Fri, 13 Jun 2025 16:10:52 +0200
From: Radim Krčmář <rkrcmar@...tanamicro.com>
To: "David Laight" <david.laight.linux@...il.com>
Cc: <linux-riscv@...ts.infradead.org>, <linux-kernel@...r.kernel.org>, "Paul
 Walmsley" <paul.walmsley@...ive.com>, "Palmer Dabbelt"
 <palmer@...belt.com>, "Albert Ou" <aou@...s.berkeley.edu>, "Alexandre
 Ghiti" <alex@...ti.fr>, "Atish Patra" <atishp@...osinc.com>, "Andrew Jones"
 <ajones@...tanamicro.com>, Clément Léger
 <cleger@...osinc.com>, "Anup Patel" <apatel@...tanamicro.com>
Subject: Re: [PATCH 2/2] RISC-V: make use of variadic sbi_ecall

2025-06-13T11:54:59+01:00, David Laight <david.laight.linux@...il.com>:
> On Thu, 12 Jun 2025 16:57:55 +0200
> Radim Krčmář <rkrcmar@...tanamicro.com> wrote:
>
>> The new sbi_ecall doesn't have to list all 8 arguments anymore, so only
>> pass the actual numbers of arguments for each SBI function.
>> 
>> Trailing 0 is sometimes intentional.
> ...
>> @@ -630,10 +630,10 @@ static int pmu_sbi_snapshot_setup(struct riscv_pmu *pmu, int cpu)
>>  	if (IS_ENABLED(CONFIG_32BIT))
>>  		ret = sbi_ecall(SBI_EXT_PMU, SBI_EXT_PMU_SNAPSHOT_SET_SHMEM,
>>  				cpu_hw_evt->snapshot_addr_phys,
>> -				(u64)(cpu_hw_evt->snapshot_addr_phys) >> 32, 0, 0, 0, 0);
>> +				(u64)(cpu_hw_evt->snapshot_addr_phys) >> 32);
>
> That doesn't look right (and other similar ones).

This one is wrong, but because I missed the flags.  This patch should
have been:

		ret = sbi_ecall(SBI_EXT_PMU, SBI_EXT_PMU_SNAPSHOT_SET_SHMEM,
				cpu_hw_evt->snapshot_addr_phys,
				(u64)(cpu_hw_evt->snapshot_addr_phys) >> 32, 0);

I'll fix that in v2, thanks.  I think you might be referring to the fact
that the code would make more sense as:

		ret = sbi_ecall(SBI_EXT_PMU, SBI_EXT_PMU_SNAPSHOT_SET_SHMEM,
				lower_32_bits(cpu_hw_evt->snapshot_addr_phys),
				upper_32_bits(cpu_hw_evt->snapshot_addr_phys))

I fully agree with that, but it's a different patch... I would even
special case the `if` with CONFIG_32BIT && CONFIG_PHYS_ADDR_T_64BIT to
make it extra clear why we're doing such a weird thing.

> The values are still 64bit - so get passed as two 32bit values (in some way)
> so that varargs code will get the wrong values.

The SBI function prototype looks like this in the specification:

  struct sbiret sbi_pmu_snapshot_set_shmem(unsigned long shmem_phys_lo,
                                           unsigned long shmem_phys_hi,
                                           unsigned long flags)

SBI defines long to be the native register width, 32-bit with
CONFIG_32BIT, and therefore uses 2 registers to pass the physical
address, because the physical address can be up to 34 bits on RV32.

The macro will result in the same arguments as before, and it is what
the sbi_ecall actually should do.

> I guess the previous change wasn't tested on 32bit?

It wasn't even compiled, because 64-bit phys_addr_t on CONFIG_32BIT
requires CONFIG_PHYS_ADDR_T_64BIT, but that config combination seems
impossible at this point.
"(u64)(cpu_hw_evt->snapshot_addr_phys) >> 32)" is a fancy way to say 0.

If we were able to compile with CONFIG_PHYS_ADDR_T_64BIT, I think the
patch would produce the desired result, hopefully with a warning that
we're implicitly casting u64 to u32, but that was there even before this
patch.

Enabling CONFIG_PHYS_ADDR_T_64BIT will have its share of issues --
I noticed a bug where other 32-bit function (SBI_EXT_NACL_SET_SHMEM)
forgets to pass the upper part of the physical address, but I didn't
include it in this series, because it made no difference right now.