Messages in this thread | | | From | Atish Patra <> | Date | Mon, 9 Jan 2023 11:38:15 -0800 | Subject | Re: Expected rdpmc behavior during context swtich and a RISC-V conundrum |
| |
On Mon, Jan 9, 2023 at 7:26 AM Mark Rutland <mark.rutland@arm.com> wrote: > > On Mon, Jan 09, 2023 at 01:06:45AM -0800, Atish Patra wrote: > > On Fri, Jan 6, 2023 at 4:02 AM Mark Rutland <mark.rutland@arm.com> wrote: > > > > > > On Thu, Jan 05, 2023 at 11:59:24AM -0800, Atish Patra wrote: > > > > Hi All, > > > > There was a recent uabi update[1] for RISC-V that allows the users to > > > > read cycle and instruction count without any checks. > > > > We tried to restrict that behavior to address security concerns > > > > earlier but it resulted in breakage for some user space > > > > applications[2]. > > > > Thus, previous behavior was restored where a user on RISC-V platforms > > > > can directly read cycle or instruction count[3]. > > > > > > > > Comparison with other ISAs w.r.t user space access of counters: > > > > ARM64 > > > > -- Enabled/Disabled via (/proc/sys/kernel/perf_user_access) > > > > -- Only for task bound events configured via perf. > > > > > > > > X86 > > > > --- rdpmc instruction > > > > --- Enable/Disable via “/sys/devices/cpu/rdpmc” > > > > -- Before v4.0 > > > > -- any process (even without active perf event) rdpmc > > > > After v4.0 > > > > -- Default behavior changed to support only active events in a > > > > process’s context. > > > > -- Configured through perf similar to ARM64 > > > > -- Continue to maintain backward compatibility for unrestricted access > > > > by writing 2 to “/sys/devices/cpu/rdpmc” > > > > > > > > IMO, RISC-V should only enable user space access through perf similar > > > > to ARM64 and x86 (post v4.0). > > > > However, we do have to support the legacy behavior to avoid > > > > application breakage. > > > > As per my understanding a direct user space access can lead to the > > > > following problems: > > > > > > > > 1) There is no context switch support, so counts from other contexts are exposed > > > > 2) If a perf user is allocated one of these counters, the counter > > > > value will be written > > > > > > > > Looking at the x86 code as it continues to allow the above behavior, > > > > rdpmc_always_available_key is enabled in the above case. However, > > > > during the context switch (cr4_update_pce_mm) > > > > only dirty counters are cleared. It only prevents leakage from perf > > > > task to rdpmc task. > > > > > > > > How does the context switch of counters work for users who enable > > > > unrestricted access by writing 2 to “/sys/devices/cpu/rdpmc” ? > > > > Otherwise, rdpmc users likely get noise from other applications. Is > > > > that expected ? > > > > > > Regardless of leakage, they're also going to get random jumps in the visible > > > values of the cycle count and instruction count as the task is context-switched > > > (and/or if those values get reset across idle, as can happen on arm64), so > > > those aren't going to be useful unless a number of other constraints apply. > > > > > > > Agreed. > > > > > AFAICT the affected package was actually a library of intrinsics; does this > > > affect a real application, or was this just in tests? If it's the latter there > > > might still be scope to properly lock this down... > > > > > > > Unfortunately, there are real applications In RISC-V started using > > cycle counters due to legacy reasons. > > > > Here is the short list from debian repo pointed out in [1] > > https://codesearch.debian.net/search?q=%22rdcycle+%250%22 > > The first of those is GRUB, when running bare metal. > > The second is this library again, which is not a whole application. > > I see that it's used in some benchmarks in real applications, e.g. firefox, > chrome. However, as above we know that's *broken* today. > > Is that code actually run? > > > Looking at aarch64 code in one of the application, it seems they rely > > on reading "pmccntr_el0" to read time > > https://sources.debian.org/src/chromium/108.0.5359.124-1/third_party/ffmpeg/libavutil/aarch64/timer.h/ > > That part is under ifdefs for mac os: > > | #if defined(__APPLE__) >
Yes. Sorry I missed that earlier. I checked few other application code which correctly use cntvct_el0 as you pointed out.
> ... and it's nonsensical anyway, pmccntr_el0 is *not* a timer, and I don't know > if Mac OS would bother to context-switch the value, so it's very likely broken > anyhow. > > > AFAIK, any counter access from EL0 is disabled by default in > > reset_pmuserenr_el0 and should be enabled via the > > proc/sys/perf_user_access > > in armv8pmu_enable_user_access. Is that correct ? > > Yes, we *only* enable access for tasks doing self-monitoring via perf. > > No other useage makes sense, since the value is arbitrarily reset, and it's not > consistent across CPUs. It *cannot* be used as a timer. >
Yes. That's what we were trying to convince the user space application maintainers too.
> > I couldn't find any application actually enabling the access using > > perf_user_access. Maybe I am missing something? > > Otherwise, the above application would trap on access to pmccntr_el0. > > As above, that's on Mac OS, not Linux. >
Yeah. My bad. Sorry for the noise :)
> Thanks, > Mark. > > > > > [1] https://groups.google.com/a/groups.riscv.org/g/sw-dev/c/REWcwYnzsKE?pli=1 > > > Thanks, > > > Mark. > > > > > > > This can be a security concern also where a rogue rdpmc user > > > > application can monitor other critical applications to initiate side > > > > channel attack. > > > > > > > > Am I missing something? Please correct my understanding of the x86 > > > > implementation if it is wrong. > > > > > > > > [1] https://lore.kernel.org/lkml/20221201135110.3855965-1-conor.dooley@microchip.com/ > > > > [2] https://groups.google.com/a/groups.riscv.org/g/sw-dev/c/REWcwYnzsKE?pli=1 > > > > [3] https://lore.kernel.org/all/YxIzgYP3MujXdqwj@aurel32.net/T/ > > > > > > > > -- > > > > Regards, > > > > Atish > > > > > > > > -- > > Regards, > > Atish
-- Regards, Atish
| |