* [PATCH] lib: sbi: Only enable TM bit in scounteren @ 2025-05-14 0:25 Atish Patra 2025-05-14 0:35 ` Jessica Clarke 0 siblings, 1 reply; 6+ messages in thread From: Atish Patra @ 2025-05-14 0:25 UTC (permalink / raw) To: anup; +Cc: opensbi, 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 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] lib: sbi: Only enable TM bit in scounteren 2025-05-14 0:25 [PATCH] lib: sbi: Only enable TM bit in scounteren Atish Patra @ 2025-05-14 0:35 ` Jessica Clarke 2025-05-14 0:49 ` Samuel Holland 2025-05-14 7:34 ` Atish Patra 0 siblings, 2 replies; 6+ messages in thread From: Jessica Clarke @ 2025-05-14 0:35 UTC (permalink / raw) To: Atish Patra; +Cc: anup, opensbi 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] lib: sbi: Only enable TM bit in scounteren 2025-05-14 0:35 ` Jessica Clarke @ 2025-05-14 0:49 ` Samuel Holland 2025-05-14 0:56 ` Jessica Clarke 2025-05-14 7:34 ` Atish Patra 1 sibling, 1 reply; 6+ messages in thread From: Samuel Holland @ 2025-05-14 0:49 UTC (permalink / raw) To: Jessica Clarke, Atish Patra; +Cc: anup, opensbi 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] lib: sbi: Only enable TM bit in scounteren 2025-05-14 0:49 ` Samuel Holland @ 2025-05-14 0:56 ` Jessica Clarke 0 siblings, 0 replies; 6+ messages in thread From: Jessica Clarke @ 2025-05-14 0:56 UTC (permalink / raw) To: Samuel Holland; +Cc: Atish Patra, anup, opensbi 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] lib: sbi: Only enable TM bit in scounteren 2025-05-14 0:35 ` Jessica Clarke 2025-05-14 0:49 ` Samuel Holland @ 2025-05-14 7:34 ` Atish Patra 2025-07-21 10:53 ` Anup Patel 1 sibling, 1 reply; 6+ messages in thread From: Atish Patra @ 2025-05-14 7:34 UTC (permalink / raw) To: Jessica Clarke; +Cc: anup, opensbi 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] lib: sbi: Only enable TM bit in scounteren 2025-05-14 7:34 ` Atish Patra @ 2025-07-21 10:53 ` Anup Patel 0 siblings, 0 replies; 6+ messages in thread From: Anup Patel @ 2025-07-21 10:53 UTC (permalink / raw) To: Atish Patra; +Cc: Jessica Clarke, opensbi 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2025-07-21 11:40 UTC | newest] Thread overview: 6+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2025-05-14 0:25 [PATCH] lib: sbi: Only enable TM bit in scounteren Atish Patra 2025-05-14 0:35 ` Jessica Clarke 2025-05-14 0:49 ` Samuel Holland 2025-05-14 0:56 ` Jessica Clarke 2025-05-14 7:34 ` Atish Patra 2025-07-21 10:53 ` Anup Patel
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox