Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Fri, 1 Jul 2022 16:50:52 +0800
From:   Leo Yan <leo.yan@linaro.org>
To:     Anshuman Khandual <anshuman.khandual@arm.com>
Cc:     "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        Ingo Molnar <mingo@redhat.com>,
        Peter Zijlstra <peterz@infradead.org>,
        Arnaldo Carvalho de Melo <acme@kernel.org>,
        Mark Rutland <mark.rutland@arm.com>
Subject: Re: Regarding perfmon_capable()
Message-ID: <20220701085052.GB659023@leoy-ThinkPad-X240s>
References: <9be223fb-5803-b676-902a-28e1c168cd8a@arm.com>
 <20220701064732.GA659023@leoy-ThinkPad-X240s>
 <54ea5d8c-74ad-c89a-929f-5d570ca351df@arm.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <54ea5d8c-74ad-c89a-929f-5d570ca351df@arm.com>
Precedence: bulk

On Fri, Jul 01, 2022 at 12:52:36PM +0530, Anshuman Khandual wrote:
> 
> 
> 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.

Agree, I might introduce confusion so just clarify.

Based on the document Documentation/admin-guide/perf-security.rst, we can
"create perf_users group for privileged Perf users".  Here privileged
users mean not only the root user, it also includes the users in
perf_users group.

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

Good point.  Seems to me, Perf core layer checks permission with a big
granularity, and low level driver can control permission with smaller
granularity, e.g. for a perf event, the low level driver can control
only its partial functionality used by non-privileged users.

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

TBH, I didn't remember in details, but let's assume if Perf works in
system wide mode and it needs to enable PMU for all CPUs in the system.
IIUC, it needs to use per CPU's workqueue to enable PMU events, I think
the function event_function_call() can give hints for why some Perf flow
is not invoked in user process context.

Thanks,
Leo