| Message ID | 20210527003044.889681-6-atish.patra@wdc.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | SBI PMU extension support | expand |
On 27 May 2021, at 01:30, Atish Patra <atish.patra@wdc.com> wrote: > > Currently, all bits in mcountern are enabled unconditionally at > boot time. With SBI PMU extension, this should enabled only during > performance monitoring for a particular event except the TM bit. However, > this is done only if mcountinhibit is implemented because the supervisor > mode can not start/stop any event without mcountinhibit. > > Similarly, supervisor should take care enabling scounteren which allows > U-mode to access pmu counters. Disable all bits in scounteren in M-mode. Turning off CY or IR seems like a bad idea to me. Similarly, all of CY, IR and TM should be enabled in scounteren by default, this is just needlessly hostile. IMO both mcounteren and scounteren should be initialised to 7 on boot. Jess
On Wed, May 26, 2021 at 5:36 PM Jessica Clarke <jrtc27@jrtc27.com> wrote: > > On 27 May 2021, at 01:30, Atish Patra <atish.patra@wdc.com> wrote: > > > > Currently, all bits in mcountern are enabled unconditionally at > > boot time. With SBI PMU extension, this should enabled only during > > performance monitoring for a particular event except the TM bit. However, > > this is done only if mcountinhibit is implemented because the supervisor > > mode can not start/stop any event without mcountinhibit. > > > > Similarly, supervisor should take care enabling scounteren which allows > > U-mode to access pmu counters. Disable all bits in scounteren in M-mode. > > Turning off CY or IR seems like a bad idea to me. Similarly, all of CY, IR and > TM should be enabled in scounteren by default, this is just needlessly hostile. This happens only if the platform supports mcountinhibit. As per the priv spec, no counter should run unless the corresponding bit in mcountinhibit is cleared. This patch just follows the spec. > IMO both mcounteren and scounteren should be initialised to 7 on boot. > > Jess > > > -- > opensbi mailing list > opensbi@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/opensbi
On 27 May 2021, at 23:06, Atish Patra <atishp@atishpatra.org> wrote: > > On Wed, May 26, 2021 at 5:36 PM Jessica Clarke <jrtc27@jrtc27.com> wrote: >> >> On 27 May 2021, at 01:30, Atish Patra <atish.patra@wdc.com> wrote: >>> >>> Currently, all bits in mcountern are enabled unconditionally at >>> boot time. With SBI PMU extension, this should enabled only during >>> performance monitoring for a particular event except the TM bit. However, >>> this is done only if mcountinhibit is implemented because the supervisor >>> mode can not start/stop any event without mcountinhibit. >>> >>> Similarly, supervisor should take care enabling scounteren which allows >>> U-mode to access pmu counters. Disable all bits in scounteren in M-mode. >> >> Turning off CY or IR seems like a bad idea to me. Similarly, all of CY, IR and >> TM should be enabled in scounteren by default, this is just needlessly hostile. > > This happens only if the platform supports mcountinhibit. As per the > priv spec, no counter should run unless the corresponding bit in > mcountinhibit > is cleared. This patch just follows the spec. I don’t understand your reply. The privileged spec just says how the CSRs behave, it doesn’t say that you need to make an SBI call (which isn’t part of the privileged spec) for cycle and instret to count. Requiring that is unnecessarily hostile; in practice, everything will want those two counters to be enabled, because arbitrary userspace software wants to use them. They should never be disabled by OpenSBI unless S-mode explicitly asks for them to be disabled. Jess
On Thu, May 27, 2021 at 3:15 PM Jessica Clarke <jrtc27@jrtc27.com> wrote: > > On 27 May 2021, at 23:06, Atish Patra <atishp@atishpatra.org> wrote: > > > > On Wed, May 26, 2021 at 5:36 PM Jessica Clarke <jrtc27@jrtc27.com> wrote: > >> > >> On 27 May 2021, at 01:30, Atish Patra <atish.patra@wdc.com> wrote: > >>> > >>> Currently, all bits in mcountern are enabled unconditionally at > >>> boot time. With SBI PMU extension, this should enabled only during > >>> performance monitoring for a particular event except the TM bit. However, > >>> this is done only if mcountinhibit is implemented because the supervisor > >>> mode can not start/stop any event without mcountinhibit. > >>> > >>> Similarly, supervisor should take care enabling scounteren which allows > >>> U-mode to access pmu counters. Disable all bits in scounteren in M-mode. > >> > >> Turning off CY or IR seems like a bad idea to me. Similarly, all of CY, IR and > >> TM should be enabled in scounteren by default, this is just needlessly hostile. > > > > This happens only if the platform supports mcountinhibit. As per the > > priv spec, no counter should run unless the corresponding bit in > > mcountinhibit > > is cleared. This patch just follows the spec. > > I don’t understand your reply. The privileged spec just says how the CSRs > behave, it doesn’t say that you need to make an SBI call (which isn’t part of > the privileged spec) for cycle and instret to count. Requiring that is > unnecessarily hostile; in practice, everything will want those two counters to > be enabled, because arbitrary userspace software wants to use them. That's a security risk. One rogue process can have access to cycle & instructions of the entire kernel & M-mode firmware. The counters should report a value only if it is being asked to start/stop a userspace application (e.g. perf). We can keep CY & IR bits enabled in OpenSBI but it must be disabled by the Linux kernel PMU driver by default. The effect will remain the same. Once sscopmf is implemented, there is a plan to set MINH bit always so that supervisor/userspace don't know how many cycles are spent while running OpenSBI. They should > never be disabled by OpenSBI unless S-mode explicitly asks for them to be > disabled. > > Jess >
On 28 May 2021, at 16:45, Atish Patra <atishp@atishpatra.org> wrote: > > On Thu, May 27, 2021 at 3:15 PM Jessica Clarke <jrtc27@jrtc27.com> wrote: >> >> On 27 May 2021, at 23:06, Atish Patra <atishp@atishpatra.org> wrote: >>> >>> On Wed, May 26, 2021 at 5:36 PM Jessica Clarke <jrtc27@jrtc27.com> wrote: >>>> >>>> On 27 May 2021, at 01:30, Atish Patra <atish.patra@wdc.com> wrote: >>>>> >>>>> Currently, all bits in mcountern are enabled unconditionally at >>>>> boot time. With SBI PMU extension, this should enabled only during >>>>> performance monitoring for a particular event except the TM bit. However, >>>>> this is done only if mcountinhibit is implemented because the supervisor >>>>> mode can not start/stop any event without mcountinhibit. >>>>> >>>>> Similarly, supervisor should take care enabling scounteren which allows >>>>> U-mode to access pmu counters. Disable all bits in scounteren in M-mode. >>>> >>>> Turning off CY or IR seems like a bad idea to me. Similarly, all of CY, IR and >>>> TM should be enabled in scounteren by default, this is just needlessly hostile. >>> >>> This happens only if the platform supports mcountinhibit. As per the >>> priv spec, no counter should run unless the corresponding bit in >>> mcountinhibit >>> is cleared. This patch just follows the spec. >> >> I don’t understand your reply. The privileged spec just says how the CSRs >> behave, it doesn’t say that you need to make an SBI call (which isn’t part of >> the privileged spec) for cycle and instret to count. Requiring that is >> unnecessarily hostile; in practice, everything will want those two counters to >> be enabled, because arbitrary userspace software wants to use them. > > That's a security risk. One rogue process can have access to cycle & > instructions of the entire kernel & M-mode firmware. > The counters should report a value only if it is being asked to > start/stop a userspace application (e.g. perf). > > We can keep CY & IR bits enabled in OpenSBI but it must be disabled by > the Linux kernel PMU driver by default. > The effect will remain the same. > > Once sscopmf is implemented, there is a plan to set MINH bit always so > that supervisor/userspace don't know how many cycles > are spent while running OpenSBI. That side-channel will always exist by measuring time. This just breaks valid use-cases in the name of “security”. __builtin_readcyclecounter is a thing and needs to work otherwise you *will* break software. At the end of the day I don’t care what you do in the Linux kernel, but I do care what gets done in the platform firmware that breaks existing 3rd-party userspace software on FreeBSD. Jess
On Fri, May 28, 2021 at 12:46 PM Atish Patra <atishp@atishpatra.org> wrote: > > On Thu, May 27, 2021 at 3:15 PM Jessica Clarke <jrtc27@jrtc27.com> wrote: > > > > On 27 May 2021, at 23:06, Atish Patra <atishp@atishpatra.org> wrote: > > > > > > On Wed, May 26, 2021 at 5:36 PM Jessica Clarke <jrtc27@jrtc27.com> wrote: > > >> > > >> On 27 May 2021, at 01:30, Atish Patra <atish.patra@wdc.com> wrote: > > >>> > > >>> Currently, all bits in mcountern are enabled unconditionally at > > >>> boot time. With SBI PMU extension, this should enabled only during > > >>> performance monitoring for a particular event except the TM bit. However, > > >>> this is done only if mcountinhibit is implemented because the supervisor > > >>> mode can not start/stop any event without mcountinhibit. > > >>> > > >>> Similarly, supervisor should take care enabling scounteren which allows > > >>> U-mode to access pmu counters. Disable all bits in scounteren in M-mode. > > >> > > >> Turning off CY or IR seems like a bad idea to me. Similarly, all of CY, IR and > > >> TM should be enabled in scounteren by default, this is just needlessly hostile. > > > > > > This happens only if the platform supports mcountinhibit. As per the > > > priv spec, no counter should run unless the corresponding bit in > > > mcountinhibit > > > is cleared. This patch just follows the spec. > > > > I don’t understand your reply. The privileged spec just says how the CSRs > > behave, it doesn’t say that you need to make an SBI call (which isn’t part of > > the privileged spec) for cycle and instret to count. Requiring that is > > unnecessarily hostile; in practice, everything will want those two counters to > > be enabled, because arbitrary userspace software wants to use them. > > That's a security risk. One rogue process can have access to cycle & > instructions of the entire kernel & M-mode firmware. For the uninformed (me), can you elaborate on this risk? > The counters should report a value only if it is being asked to > start/stop a userspace application (e.g. perf). > > We can keep CY & IR bits enabled in OpenSBI but it must be disabled by > the Linux kernel PMU driver by default. > The effect will remain the same. > > Once sscopmf is implemented, there is a plan to set MINH bit always so > that supervisor/userspace don't know how many cycles > are spent while running OpenSBI. > > They should > > never be disabled by OpenSBI unless S-mode explicitly asks for them to be > > disabled. > > > > Jess > > > > > -- > Regards, > Atish > > -- > opensbi mailing list > opensbi@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/opensbi
On Fri, May 28, 2021 at 8:53 AM Jessica Clarke <jrtc27@jrtc27.com> wrote: > > On 28 May 2021, at 16:45, Atish Patra <atishp@atishpatra.org> wrote: > > > > On Thu, May 27, 2021 at 3:15 PM Jessica Clarke <jrtc27@jrtc27.com> wrote: > >> > >> On 27 May 2021, at 23:06, Atish Patra <atishp@atishpatra.org> wrote: > >>> > >>> On Wed, May 26, 2021 at 5:36 PM Jessica Clarke <jrtc27@jrtc27.com> wrote: > >>>> > >>>> On 27 May 2021, at 01:30, Atish Patra <atish.patra@wdc.com> wrote: > >>>>> > >>>>> Currently, all bits in mcountern are enabled unconditionally at > >>>>> boot time. With SBI PMU extension, this should enabled only during > >>>>> performance monitoring for a particular event except the TM bit. However, > >>>>> this is done only if mcountinhibit is implemented because the supervisor > >>>>> mode can not start/stop any event without mcountinhibit. > >>>>> > >>>>> Similarly, supervisor should take care enabling scounteren which allows > >>>>> U-mode to access pmu counters. Disable all bits in scounteren in M-mode. > >>>> > >>>> Turning off CY or IR seems like a bad idea to me. Similarly, all of CY, IR and > >>>> TM should be enabled in scounteren by default, this is just needlessly hostile. > >>> > >>> This happens only if the platform supports mcountinhibit. As per the > >>> priv spec, no counter should run unless the corresponding bit in > >>> mcountinhibit > >>> is cleared. This patch just follows the spec. > >> > >> I don’t understand your reply. The privileged spec just says how the CSRs > >> behave, it doesn’t say that you need to make an SBI call (which isn’t part of > >> the privileged spec) for cycle and instret to count. Requiring that is > >> unnecessarily hostile; in practice, everything will want those two counters to > >> be enabled, because arbitrary userspace software wants to use them. > > > > That's a security risk. One rogue process can have access to cycle & > > instructions of the entire kernel & M-mode firmware. > > The counters should report a value only if it is being asked to > > start/stop a userspace application (e.g. perf). > > > > We can keep CY & IR bits enabled in OpenSBI but it must be disabled by > > the Linux kernel PMU driver by default. > > The effect will remain the same. > > > > Once sscopmf is implemented, there is a plan to set MINH bit always so > > that supervisor/userspace don't know how many cycles > > are spent while running OpenSBI. > > That side-channel will always exist by measuring time. This just breaks valid > use-cases in the name of “security”. > > __builtin_readcyclecounter is a thing and needs to work otherwise you *will* > break software. At the end of the day I don’t care what you do in the Linux > kernel, but I do care what gets done in the platform firmware that breaks > existing 3rd-party userspace software on FreeBSD. > Ahh okay. I didn't know about __builtin_readcyclecounter. Ofcourse, we can't break userspace. I will revise the patch to enable CY & IR. I will check if Linux user space has such a requirement too. What about a setting MINH bit in sscofpmf extension[1] implementation? Does __builtin_readcyclecounter expect to report the counter for all the modes ? [1] https://lists.riscv.org/g/tech-privileged/message/452?p=,,,20,0,0,0::Created,,fast+track,20,2,20,80278800 > Jess >
> What about a setting MINH bit in sscofpmf extension[1] implementation?
Sorry for getting late to the discussion. I think it needs to be
re-iterated that CY and IR are not part of Sscofpmf extension. There are
no corresponding mhpmevent registers for them. I asked Greg about this
in a private thread and if I understood it correctly, the reason for not
tackling CY and IR is that it would cause the extension to fall out of
the fast-track process.
So all in all, Sscofpmf will not be enough to tackle the sampling of
neither of those two. Unless there will be a copy of those counters in
mhpmcounters.
--
Best Regards,
Stan Kardach
> -----Original Message----- > From: opensbi <opensbi-bounces@lists.infradead.org> On Behalf Of Stanislaw > Kardach > Sent: 05 June 2021 21:47 > To: Atish Patra <Atish.Patra@wdc.com> > Cc: opensbi@lists.infradead.org > Subject: [v2,05/15] lib: sbi: Disable m/scounteren & enable mcountinhibit > > > What about a setting MINH bit in sscofpmf extension[1] implementation? > > Sorry for getting late to the discussion. I think it needs to be re-iterated that > CY and IR are not part of Sscofpmf extension. There are no corresponding > mhpmevent registers for them. I asked Greg about this in a private thread and > if I understood it correctly, the reason for not tackling CY and IR is that it > would cause the extension to fall out of the fast-track process. > > So all in all, Sscofpmf will not be enough to tackle the sampling of neither of > those two. Unless there will be a copy of those counters in mhpmcounters. Even if CY and IR are not part of Sscofpmf extension, we still have mechanism to start/stop these counters via mcountinhibit so software (Kernel and and Hypervisors) can use SBI PMU calls to start/stop these counters only for Processes/Guest/VMs that have explicitly requested it. To address this nature of CY and IR, the filter flags of SBI PMU config matching call are defined as optional for the SBI implementation so these filter flags can be ignored for CY and IR counters. Regards, Anup > > -- > Best Regards, > Stan Kardach > > > -- > opensbi mailing list > opensbi@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/opensbi
diff --git a/lib/sbi/sbi_hart.c b/lib/sbi/sbi_hart.c index 0d5b7b8d3509..9f1ee20fa978 100644 --- a/lib/sbi/sbi_hart.c +++ b/lib/sbi/sbi_hart.c @@ -51,12 +51,26 @@ static void mstatus_init(struct sbi_scratch *scratch) csr_write(CSR_MSTATUS, mstatus_val); - /* Enable user/supervisor use of perf counters */ + /* Disable user mode usage of perf counters */ if (misa_extension('S') && sbi_hart_has_feature(scratch, SBI_HART_HAS_SCOUNTEREN)) - csr_write(CSR_SCOUNTEREN, -1); - if (sbi_hart_has_feature(scratch, SBI_HART_HAS_MCOUNTEREN)) - csr_write(CSR_MCOUNTEREN, -1); + csr_write(CSR_SCOUNTEREN, 0); + + if (sbi_hart_has_feature(scratch, SBI_HART_HAS_MCOUNTEREN)) { + if (sbi_hart_has_feature(scratch, SBI_HART_HAS_MCOUNTINHIBIT)) + /** + * Just enable TM bit now. All other counters will be + * enabled at runtime after S-mode request + */ + csr_write(CSR_MCOUNTEREN, 2); + else + /* Supervisor mode usage are enabled by default */ + csr_write(CSR_MCOUNTEREN, -1); + } + + /* Counters will start running at runtime after S-mode request */ + if (sbi_hart_has_feature(scratch, SBI_HART_HAS_MCOUNTINHIBIT)) + csr_write(CSR_MCOUNTINHIBIT, -1); /* Disable all interrupts */ csr_write(CSR_MIE, 0);