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

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

[Atish Patra]

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.

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

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

[Jessica Clarke]

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.

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.

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

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

[Samuel Holland]

Hi Jess,

On 2025-05-13 7: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.
> 
> 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.

This shouldn't be a breaking change for any OS that supports SMP. While the
architectural state of the boot hart upon S-mode entry is not well defined, the
SBI HSM specification explicitly states that "All other registers remain in an
undefined state." (This wording needs to be modified in SBI 3.0 to account for
FWFT defining the reset value of most features, but the point stands.) So it
would be inappropriate for an OS to rely on the preexisting scounteren CSR value
on any secondary hart. Thus, any SMP OS must have code to initialize scounteren
anyway.

Regards,
Samuel

> 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

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

[Jessica Clarke]

On 14 May 2025, at 01:49, Samuel Holland <samuel.holland@sifive.com> wrote:
> 
> Hi Jess,
> 
> On 2025-05-13 7: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.
>> 
>> 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.
> 
> This shouldn't be a breaking change for any OS that supports SMP. While the
> architectural state of the boot hart upon S-mode entry is not well defined, the
> SBI HSM specification explicitly states that "All other registers remain in an
> undefined state." (This wording needs to be modified in SBI 3.0 to account for
> FWFT defining the reset value of most features, but the point stands.) So it
> would be inappropriate for an OS to rely on the preexisting scounteren CSR value
> on any secondary hart. Thus, any SMP OS must have code to initialize scounteren
> anyway.

Should. But in practice these things get missed, and FreeBSD does not
have code to do that today. Is that a bug? Sure. But try arguing that
for a Linux kernel change that breaks userspace; it’s the same argument
here. I can add it, but that won’t help existing releases, and I’d like
to not have users’ systems break when they update their firmware.

Jess

> Regards,
> Samuel
> 
>> 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

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

[Atish Patra]

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

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

[Anup Patel]

On Wed, May 14, 2025 at 1:04 PM Atish Patra <atish.patra@linux.dev> wrote:
>
> 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 ?

KVM RISC-V is already updated to not initialize scounteren
for Guest/VM so along these lines lets go ahead with this patch.

I have applied this patch to the riscv/opensbi and this will be
part of OpenSBI v1.8 release in Dec 2025.

Thanks,
Anup

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