[PATCH v2 05/15] lib: sbi: Disable m/scounteren & enable mcountinhibit

[Atish Patra]

On Fri, May 28, 2021 at 8:53 AM Jessica Clarke <jrtc27 at jrtc27.com> wrote:
>
> On 28 May 2021, at 16:45, Atish Patra <atishp at atishpatra.org> wrote:
> >
> > On Thu, May 27, 2021 at 3:15 PM Jessica Clarke <jrtc27 at jrtc27.com> wrote:
> >>
> >> On 27 May 2021, at 23:06, Atish Patra <atishp at atishpatra.org> wrote:
> >>>
> >>> On Wed, May 26, 2021 at 5:36 PM Jessica Clarke <jrtc27 at jrtc27.com> wrote:
> >>>>
> >>>> On 27 May 2021, at 01:30, Atish Patra <atish.patra at 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
>


-- 
Regards,
Atish