Re: [PATCH v1 2/2] perf cap: If capability is missing still perform root test

[Namhyung Kim <namhyung@kernel.org>]

On Fri, Jul 10, 2026 at 05:47:10AM +0000, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> - [High] The capability fallback logic overrides valid capability checks for root users and incorrectly short-circuits sequential capability checks for non-root users by improperly setting the `used_root` flag.
> --
> 
> --- Patch [2]: [PATCH v1 2/2] perf cap: If capability is missing still perform root test ---
> commit 8515c6747c6ffb6d27af14829f03c002334e5c67
> Author: Ian Rogers <irogers@google.com>
> 
> perf cap: If capability is missing still perform root test
> 
> This commit refines the capability checking logic in perf_cap__capable() to
> fallback to the used_root test if a capability is missing, rather than only
> doing so when the capget syscall fails.

I think we can process this independently.  It's not related to patch 1,
right?

> 
> > diff --git a/tools/perf/util/cap.c b/tools/perf/util/cap.c
> > --- a/tools/perf/util/cap.c
> > +++ b/tools/perf/util/cap.c
> 
> [ ... ]
> 
> > @@ -37,12 +38,21 @@ bool perf_cap__capable(int cap, bool *used_root)
> >  	if (cap >= 32) {
> >  		if (header.version == _LINUX_CAPABILITY_VERSION_3) {
> >  			cap_val = data[1].effective;
> > +			has_cap = (cap_val & (1U << (cap & 0x1f))) != 0;
> >  		} else {
> >  			/* Capability beyond 32 is requested but only 32 are supported. */
> > -			return false;
> > +			has_cap = false;
> >  		}
> >  	} else {
> >  		cap_val = data[0].effective;
> > +		has_cap = (cap_val & (1U << (cap & 0x1f))) != 0;
> >  	}
> > -	return (cap_val & (1 << (cap & 0x1f))) != 0;
> > +
> > +	if (has_cap) {
> > +		*used_root = false;
> > +		return true;
> > +	}
> > +
> > +	*used_root = true;
> > +	return geteuid() == 0;
> >  }
> 
> [Severity: High]
> Does setting *used_root = true unconditionally here when a capability is
> missing break sequential capability checks for non-root users?
> 
> For example, if an unprivileged user lacks CAP_SYS_ADMIN, this will return
> false but set used_root to true. In callers like perf_event_paranoid_check():
> 
> tools/perf/util/util.c:perf_event_paranoid_check() {
>     ...
>     if (!used_root && perf_cap__capable(CAP_PERFMON, &used_root))
>     ...
> }
> 
> If a previous check for CAP_SYS_ADMIN failed and poisoned the used_root
> flag, won't it cause the subsequent check for CAP_PERFMON to be skipped
> entirely?
> 
> This pattern appears in other locations as well, such as
> check_bpf_filter_capable() where a failed CAP_BPF check might skip the
> fallback for non-root users to access root-pinned BPF filters because
> !used_root evaluates to false.
> 
> Additionally, could this override valid capability drops for root users
> in containers? Because geteuid() == 0 evaluates to true, it seems this might
> falsely claim root users still possess the capability even if it was
> explicitly dropped, potentially leading to later failures instead of
> graceful fallbacks.

Can we simply remove the used_root parameter from perf_cap__capable()?
Normally I think root would have the capability.  If not, maybe we need
to reject the operation as it may happen in a container setup.

Thanks,
Namhyung