linux-kernel - Re: Regarding perfmon

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <54ea5d8c-74ad-c89a-929f-5d570ca351df@arm.com>
Date:   Fri, 1 Jul 2022 12:52:36 +0530
From:   Anshuman Khandual <anshuman.khandual@....com>
To:     Leo Yan <leo.yan@...aro.org>
Cc:     "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        Ingo Molnar <mingo@...hat.com>,
        Peter Zijlstra <peterz@...radead.org>,
        Arnaldo Carvalho de Melo <acme@...nel.org>,
        Mark Rutland <mark.rutland@....com>
Subject: Re: Regarding perfmon_capable()



On 7/1/22 12:17, Leo Yan wrote:
> Hi Anshuman,
> 
> On Fri, Jul 01, 2022 at 10:37:37AM +0530, Anshuman Khandual wrote:
>> Hello,
>>
>> In perf event subsystem and related platform drivers registering a PMU,
>> should perfmon_capable() be used directly ? OR just wondering if instead
>> perf_allow_[cpu|kernel|tracepoint]() helpers should be used which also
>> checks for 'sysctl_perf_event_paranoid' ? Should not both capabilities
>> and 'sysctl_perf_event_paranoid' decide whether kernel/cpu/tracepoint
>> events will be captured for unprivileged users.
> 
> This is an interesting but important topic, let me join the discussion.
> 
> Simply to say, sysctl_perf_event_paranoid is a control knob,
> perfmon_capable() is for capabilities.  perfmon_capable() only allows
> privileged Perf users to access Perf events; on the other hand,
> sysctl_perf_event_paranoid can grant green light for non-privileged
> users to access perf events.

Could not unprivileged users have capabilities too ? I thought that was
the whole point for capabilities.

> 
> Therefore, if we use function perf_allow_[cpu|kernel|tracepoint]() as
> checking condition which is interfered by sysctl_perf_event_paranoid,
> it's superset of perfmon_capable().

Right. IIUC sysctl_perf_event_paranoid was the original method for perf
event to restrict access, where as capabilities is the new method. Hence
both needs to be checked for compatibility purpose for the original one.

> 
> On the other hand, even a Perf event can be opened by a non-privileged
> process, the low level driver still doesn't want to leak any sensitive
> info in the trace data or sampling.  This is why Arm SPE driver checks

Can/should low level PMU drivers enforce yet another layer of privilege
check even after the core perf allowed the event to be created in the
first place ?

> the condition perfmon_capable() and disables CONTEXTIDR tracing for
> non-privileged users (no matter what's the value of
> sysctl_perf_event_paranoid).
> 
> Just bear in mind for a corner case, some perf callback functions are
> invoked from the kernel threads context rather than user process
> context, this is why we might obeserve some strange cases that
> non-privileged users might be wrongly granted some tracing
> capabilities even we check with perfmon_capable() (Checking
> perfmon_capable() is not wrong, but it's wrong to do the checking in
> the kernel kthread context rather than user process context).

Is not pmu->event_init() called in user process context itself. Why can
not all privillege checking be done there and stored (if required) some
where more platform specific e.g event->hw.config or any other platform
data structure. Why should privilege gets checked in callbacks which
might run in privilege contexts to create such corner cases ?

> 
> This is my understanding, just correct me if any thing mentioned
> is not reliable.
> 
> Thanks,
> Leo
> 
>> arch/parisc/kernel/perf.c:      if (!perfmon_capable())
>> arch/powerpc/perf/imc-pmu.c:    if (!perfmon_capable())
>> arch/powerpc/perf/imc-pmu.c:    if (!perfmon_capable())
>> drivers/gpu/drm/i915/i915_perf.c:           i915_perf_stream_paranoid && !perfmon_capable()) {
>> drivers/gpu/drm/i915/i915_perf.c:                       if (oa_freq_hz > i915_oa_max_sample_rate && !perfmon_capable()) {
>> drivers/gpu/drm/i915/i915_perf.c:       if (i915_perf_stream_paranoid && !perfmon_capable()) {
>> drivers/gpu/drm/i915/i915_perf.c:       if (i915_perf_stream_paranoid && !perfmon_capable()) {
>> drivers/media/rc/bpf-lirc.c:            if (perfmon_capable())
>> drivers/perf/arm_spe_pmu.c:     if (IS_ENABLED(CONFIG_PID_IN_CONTEXTIDR) && perfmon_capable())
>> drivers/perf/arm_spe_pmu.c:     if (!perfmon_capable() &&
>>
>> Although BPF might use perfmon_capabale() alone, because it was never
>> dependent on 'sysctl_perf_event_paranoid' ?
>>
>> - Anshuman