rdcycle from userland with RISCV_PMU_SBI=y

[Jan Wassenberg]

(Forwarding in plain text mode as apparently required by the list,
apologies for the duplication)

On Fri, Sep 2, 2022 at 12:22 PM Jan Wassenberg <janwas at google.com> wrote:
>
> OK, but even if frequency changes, a cycle counter seems more useful for benchmarking than a low-resolution wall time.
>
> FYI it's not just Highway, the Google benchmark library also uses rdcycle.
>
> Some personal opinions on security (disclaimer: I have some experience but am not an expert):
> Disabling cycle counters does not seem to provide any extra security on a multi-core system unless
> it also detects and mitigates software timers.
> See section 6 in http://cal.cs.columbia.edu/papers/isca12_timewarp.pdf. In short, other core(s) can increment a shared counter,
> which is close enough to a cycle counter to serve as a side channel. Detecting such coordination via performance counters
> seems feasible, though it risks false alarms for something like spinlocks.
>
> Reducing timer resolution could still be defeated by interpolating. If we're determined to do something,
> perhaps the cycle counter could have some jitter (> LLC latency), ideally only enabled if
> a possible attack is detected by trapping rdcycle and checking the elapsed time since the last rdcycle.
>
> On Fri, Sep 2, 2022 at 10:19 AM Anup Patel <anup at brainfault.org> wrote:
>>
>> On Fri, Sep 2, 2022 at 1:23 PM Mathieu Malaterre <malat at debian.org> wrote:
>> >
>> > 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.
>>
>> The RISC-V spec does not make any guarantee about granularity of the
>> cycle counter as well. In fact, on systems with cpufreq enabled the
>> rate of cycle counter increment will vary at runtime.
>>
>> On other hand, the time CSR (i.e. rdtime) will always increment at a
>> fixed platform specific rate which can be read from
>> "/proc/device-tree/cpus/timebase-frequency".
>>
>> Regards,
>> Anup
>>
>>
>> >
>> > _______________________________________________
>> > linux-riscv mailing list
>> > linux-riscv at lists.infradead.org
>> > http://lists.infradead.org/mailman/listinfo/linux-riscv