Re: rdcycle from userland with RISCV_PMU_SBI=y

[Anup Patel <apatel@ventanamicro.com>]

On Fri, Sep 2, 2022 at 3:52 PM Jan Wassenberg <janwas@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.

Since the RISC-V ecosystem is still evolving, it is best time to fix
things now rather
than later.

Currently, only "time" CSR is available to user-space and all other performance
counters should be enabled (or accessed) using Linux perf syscalls.

>
> 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.

At the moment, there is no HW mechanism defined in RISC-V to obfuscate "time" or
"cycle" CSRs for user-space. On the other hand, using "cycle" for
measuring passing
time is semantically incorrect.

In the future, someone can always propose a RISC-V ISA extension to obfuscate
"time" CSR values visible to user-space but the "cycle" counter is
purely a performance
counter and accurately reflects cycles taken by the CPU.

>
> 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.

The "cycle" counter is extensively used for performance analysis so it
is very unlikely
there might any ISA extension created to add jitters in performance
counters. The
"time" CSR (i.e. rdtime) on other hand is only used for measuring
passing time so
an ISA extension is possible to add some jitter. For example, in H-extension we
already have "htimedelta" CSR to add some delta to the "time" CSR values seen
by each Guest/VM under a hypervisor.

Regards,
Anup

>
> On Fri, Sep 2, 2022 at 10:19 AM Anup Patel <anup@brainfault.org> wrote:
>>
>> On Fri, Sep 2, 2022 at 1:23 PM Mathieu Malaterre <malat@debian.org> wrote:
>> >
>> > Atish,
>> >
>> > On Fri, Sep 2, 2022 at 9:12 AM Atish Patra <atishp@atishpatra.org> wrote:
>> > >
>> > >
>> > >
>> > > On Thu, Sep 1, 2022 at 3:34 PM Atish Patra <atishp@atishpatra.org> wrote:
>> > >>
>> > >>
>> > >>
>> > >> On Thu, Sep 1, 2022 at 2:15 PM Aurelien Jarno <aurelien@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@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@lists.infradead.org
>> > http://lists.infradead.org/mailman/listinfo/linux-riscv

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv