Re: [PATCH] x86/bpf: use bpf_capable() instead of capable(CAP_SYS_ADMIN)

[Alexei Starovoitov <alexei.starovoitov@gmail.com>]

On Fri, Aug 8, 2025 at 4:59 PM Serge E. Hallyn <serge@hallyn.com> wrote:
>
> On Wed, Aug 06, 2025 at 08:18:55PM -0500, Serge E. Hallyn wrote:
> > On Wed, Aug 06, 2025 at 04:31:05PM +0200, Ondrej Mosnacek wrote:
> > > Don't check against the overloaded CAP_SYS_ADMINin do_jit(), but instead
> > > use bpf_capable(), which checks against the more granular CAP_BPF first.
> > > Going straight to CAP_SYS_ADMIN may cause unnecessary audit log spam
> > > under SELinux, as privileged domains using BPF would usually only be
> > > allowed CAP_BPF and not CAP_SYS_ADMIN.
> > >
> > > Link: https://bugzilla.redhat.com/show_bug.cgi?id=2369326
> > > Fixes: d4e89d212d40 ("x86/bpf: Call branch history clearing sequence on exit")
> > > Signed-off-by: Ondrej Mosnacek <omosnace@redhat.com>
> >
> > So this seems correct, *provided* that we consider it within the purview of
> > CAP_BPF to be able to avoid clearing the branch history buffer.

true, but...

> >
> > I suspect that's the case, but it might warrant discussion.
> >
> > Reviewed-by: Serge Hallyn <serge@hallyn.com>
>
> (BTW, I'm assuming this will get pulled into a BPF tree or something, and
> doesn't need to go into the capabilities tree.  Let me know if that's wrong)

Right.
scripts/get_maintainer.pl arch/x86/net/bpf_jit_comp.c
is your friend.

Pls cc author-s of the commit in question in the future.
Adding them now.

> > > diff --git a/arch/x86/net/bpf_jit_comp.c b/arch/x86/net/bpf_jit_comp.c
> > > index 15672cb926fc1..2a825e5745ca1 100644
> > > --- a/arch/x86/net/bpf_jit_comp.c
> > > +++ b/arch/x86/net/bpf_jit_comp.c
> > > @@ -2591,8 +2591,7 @@ emit_jmp:
> > >                     seen_exit = true;
> > >                     /* Update cleanup_addr */
> > >                     ctx->cleanup_addr = proglen;
> > > -                   if (bpf_prog_was_classic(bpf_prog) &&
> > > -                       !capable(CAP_SYS_ADMIN)) {
> > > +                   if (bpf_prog_was_classic(bpf_prog) && !bpf_capable()) {

This looks wrong for several reasons.

1.
bpf_capable() and CAP_BPF in general applies to eBPF only.
There is no precedent so far to do anything differently
for cBPF when CAP_BPF is present.

2.
commit log states that
"privileged domains using BPF would usually only be allowed CAP_BPF
and not CAP_SYS_ADMIN"
which is true for eBPF only, since cBPF is always allowed for
all unpriv users.
Start chrome browser and you get cBPF loaded.

3.
glancing over bugzilla it seems that the issue is
excessive audit spam and not related to CAP_BPF and privileges.
If so then the fix is to use
ns_capable_noaudit(&init_user_ns, CAP_SYS_ADMIN)

4.
I don't understand how the patch is supposed to fix the issue.
iio-sensor-proxy is probably unpriv. Why would it use CAP_BPF?
It's using cBPF, so there is no reason for it to have CAP_BPF.
So capable(CAP_BPF) will fail just like capable(CAP_SYS_ADMIN),
but since CAP_BPF check was done first, the audit won't
be printed, because it's some undocumented internal selinux behavior ?
None of it is in the commit log :(

5.
And finally all that looks like a selinux bug.
Just because something in the kernel is asking capable(CAP_SYS_ADMIN)
there is no need to spam users with the wrong message:
"SELinux is preventing iio-sensor-prox from using the 'sys_admin' capabilities."
iio-sensor-prox is not trying to use 'sys_admin' capabilities.
cBPF prog will be loaded anyway, with or without BHB clearing.