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

[sashiko-bot@kernel.org]

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.

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

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260710053927.1864021-1-irogers@google.com?part=2