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

[Jessica Clarke]

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.

Jess