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

[Mitchell Horne]

On Fri, May 28, 2021 at 12:46 PM 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.

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 at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/opensbi