Re: [PATCH] lib: sbi: Only enable TM bit in scounteren

[Atish Patra <atish.patra@linux.dev>]

On 5/13/25 5:35 PM, Jessica Clarke wrote:
> On 14 May 2025, at 01:25, Atish Patra <atishp@rivosinc.com> wrote:
> 
>> The S-mode should disable Cycle and instruction counter for user space
>> to avoid side channel attacks. The Linux kernel already does this so that
>> any random user space code shouldn't be able to monitor cycle/instruction
>> without higher privilege mode involvement.
>>
>> Remove the CY/IR bits in scountern in OpenSBI.
> 
> Isn’t this a breaking change? S-mode OSes that are happy to allow
> U-mode to read the counters are now broken, such as FreeBSD. BBL always
> set scounteren to all ones (with bits above 2 being pointless without
> programming mhpmeventX), and OpenSBI did that until it switched to this
> behaviour of CY|TM|IR. So whether or not it was explicit in the
> specification what these were initialised to, S-mode OSes exist that
> assumed CY, TM and IR at least were available. Please do not break
> things; treat the firmware<->S-mode interface in the same way as
> kernel<->userspace, i.e. don’t break userspace/S-mode.
> 

Allowing CY and IR direct access is a big security concern which was 
discussed in multiple threads[1] almost two year back. This resulted in 
patches in Linux that restricts that behavior. I assumed that other OS 
might have adopted a similar behavior by now. Hence the patch.

[1] 
https://lore.kernel.org/linux-riscv/CAOnJCUKCwnOXGWKiwQQxZ92t4138JAOqzkkqtwApHRy6YuS0Kw@mail.gmail.com/
> OSes that want to opt out can. And if you really want to push for this,
> you need to wait for OSes to be changed to explicitly initialise
> scounteren and only do it once old versions are sufficiently rare. But
> you can’t just go doing this with no warning / deprecation period.
> 

It seems FreeBSD continue to have the legacy behavior. It would be great 
if you can fix the freeBSD code sooner than later. Please let us know 
once that is done.

I am absolutely fine waiting for some more time until the the commonly 
used S-mode OS behave as expected. However, we can not wait forever as 
well. So we have to set a flag day in the future avoid carrying this 
forever.

How about OpenSBI v1.7(~2-3 months) or OpenSBI v1.8 (planned towards end 
of 2025) for this fix to be merged ?

> NAK.
> 
> Jess
> 
>> Signed-off-by: Atish Patra <atishp@rivosinc.com>
>> ---
>> To: anup Patel <anup@brainfault.org>
>> Cc: opensbi@lists.infradead.org
>> ---
>> lib/sbi/sbi_hart.c | 4 ++--
>> 1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/lib/sbi/sbi_hart.c b/lib/sbi/sbi_hart.c
>> index fc4925b57625..177f356b7b77 100644
>> --- a/lib/sbi/sbi_hart.c
>> +++ b/lib/sbi/sbi_hart.c
>> @@ -49,10 +49,10 @@ static void mstatus_init(struct sbi_scratch *scratch)
>>
>> csr_write(CSR_MSTATUS, mstatus_val);
>>
>> - /* Disable user mode usage of all perf counters except default ones (CY, TM, IR) */
>> + /* Disable user mode usage of all perf counters except TM */
>> if (misa_extension('S') &&
>>     sbi_hart_priv_version(scratch) >= SBI_HART_PRIV_VER_1_10)
>> - csr_write(CSR_SCOUNTEREN, 7);
>> + csr_write(CSR_SCOUNTEREN, 0x02);
>>
>> /**
>> * OpenSBI doesn't use any PMU counters in M-mode.
>>
>> ---
>> base-commit: 316daaf1c299c29ac46e52145f65521f48ec63b5
>> change-id: 20250512-fix_scounteren-8b7b682bb864
>> --
>> Regards,
>> Atish patra
>>
>>
>> -- 
>> opensbi mailing list
>> opensbi@lists.infradead.org
>> http://lists.infradead.org/mailman/listinfo/opensbi
> 
> 


-- 
opensbi mailing list
opensbi@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/opensbi