rdcycle from userland with RISCV_PMU_SBI=y

[Mathieu Malaterre]

Atish,

On Fri, Sep 2, 2022 at 9:12 AM Atish Patra <atishp at atishpatra.org> wrote:
>
>
>
> On Thu, Sep 1, 2022 at 3:34 PM Atish Patra <atishp at atishpatra.org> wrote:
>>
>>
>>
>> On Thu, Sep 1, 2022 at 2:15 PM Aurelien Jarno <aurelien at aurel32.net> wrote:
>>>
>>> Hi all,
>>>
>>> Mathieu Malaterre reported an issue in Debian with the highway package,
>>> which uses the rdcycle pseudo instruction, and started to fail recently.
>>>
>>> We tracked down an issue to the "perf platform driver based on SBI PMU
>>> extension" recently added in Linux 5.18 [1]. On a board which runs an
>>> SBI with the PMU extension, this causes the rdcycle pseudo instruction
>>> to generate a SIGILL. This is because the driver explicitly set
>>> CSR_SCOUNTEREN to 0x02, giving access from userland to rdtime but not
>>> rdcycle.
>>>
>>> I wonder if this change is intentional, and if there is a reason to
>>> forbid using the rdcycle pseudo instruction from userland. If it is the
>>> case, this should probably be changed so that the behaviour does not
>>> differ from board to board depending on the available PMU extension.
>>>
>>
>> Yes. The change was intentional due to security reasons. One rogue process can have access to cycle &
>> instructions of the entire kernel always which can lead to some sort of side channel attacks.
>>
>> However, I agree that we can't break userspace. I was not aware of the fact that there are already users of rdcycle
>> in the userspace. I will send a patch to restore the original behavior by enabling CY, IR bits in scounteren.
>>
>
> Merging the thread from sw-dev:
>
> "
> > > On Thu, Sep 1, 2022 at 7:36 PM Paul Walmsley <paul.walmsley at sifive.com> wrote:
> > >>
> > >> If I recall correctly,  other architectures don't allow direct access to their cycle counters from userspace for security reasons.   Any reason why RISC-V shouldn't follow the same approach?
> > >
> > >
> > > That was the intention behind disabling the access in the PMU driver. At that time, I didn’t find any users. Obviously I was wrong 😂. But is that a sufficient reason to existing break user space ?
> >
> > As already pointed out, we can't compromise security by having all
> > apps unrestricted access to the cycle counter. We already have the
> > Linux perf subsystem for managing counters so apps should always use
> > Linux perf syscalls.
> >
> > IMO, the package "highway" directly accessing the cycle counter should
> > be fixed instead of fixing the Linux SBI PMU driver.
> >
> > Further investigating the highway project
> > (https://github.com/google/highway), it seems this project is using
> > "rdcycle" to track timer ticks which is totally wrong. Instead the use
> > of "rdcycle" should be replaced with "rdtime" in this project.
> > (Refer, line 156 of
> > https://github.com/google/highway/blob/master/hwy/nanobenchmark.cc)
>
> Pay attention that quite a few projects are using rdcycle in user-space already:
>
> * https://codesearch.debian.net/search?q=%22rdcycle+%250%22
>
> Anyway if you believe rdtime is the right fix, we can fix one project
> at a time..."
>
> Thanks for sharing the list. I took a quick look. The first few ones at least  (firefox, llvm, chrome)
> seems to use rdcycle for timestamp. All of the packages use "rdtsc" for x86.
> The equivalent instruction in RISC-V should be rdtime not rdcycle.

[CC-ing highway main dev]

Here is his comment on the rdtime patch proposal:

> For benchmarking we really would prefer a cycle count, rather than "real time clock ticks", for which the spec makes no guarantees. If/when those turn out to be millisecond-resolution, they are not very useful.