On Thu, May 4, 2023 at 3:51 PM Andrii Nakryiko
<andrii.nakryiko@xxxxxxxxx> wrote:
>
> On Thu, May 4, 2023 at 3:20 PM Alexei Starovoitov
> <alexei.starovoitov@xxxxxxxxx> wrote:
> >
> > On Tue, May 02, 2023 at 04:06:19PM -0700, Andrii Nakryiko wrote:
> > > diff --git a/kernel/bpf/core.c b/kernel/bpf/core.c
> > > index 4d057d39c286..c0d60da7e0e0 100644
> > > --- a/kernel/bpf/core.c
> > > +++ b/kernel/bpf/core.c
> > > @@ -661,7 +661,7 @@ static bool bpf_prog_kallsyms_candidate(const struct bpf_prog *fp)
> > > void bpf_prog_kallsyms_add(struct bpf_prog *fp)
> > > {
> > > if (!bpf_prog_kallsyms_candidate(fp) ||
> > > - !bpf_capable())
> > > + !fp->aux->bpf_capable)
> > > return;
> >
> > Looking at this bit made me worry about classic bpf.
> > bpf_prog_alloc_no_stats() zeros all fields include aux->bpf_capable.
> > And loading of classic progs doesn't go through bpf_check().
> > So fp->aux->bpf_capable will stay 'false' even when root loads cBPF.
> > It doesn't matter here, since bpf_prog_kallsyms_candidate() will return false
> > for cBPF.
> >
> > Maybe we should init aux->bpf_capable in bpf_prog_alloc_no_stats()
> > to stay consistent between cBPF and eBPF ?
I'd like to avoid doing this deep inside bpf_prog_alloc_no_stats() or
bpf_prog_alloc() because this decision about privileges will be later
based on some other factors besides CAP_BPF. So hard-coding
bpf_capable() here defeats the purpose.
I did look at classic BPF, there are three places in net/core/filter.c
where we allocated struct bpf_prog from struct sock_fprog/struct
sock_fprog_kern: bpf_prog_create, bpf_prog_create_from_user,
__get_filter.
Would it be ok if I just hard-coded `prog->aux->bpf_capable =
bpf_capable();` (and same for perfmon) in those three functions? Or
leave them always false? Because taking a step back a bit, do we want
to allow privileged classic BPF programs at all? Maybe it's actually
good that we force those cBPF programs to be unprivileged?
Right now it doesn't even matter, I think, because these privileges
are only checked during verification. But assuming that in the future
we might want to check that at runtime from helpers/kfuncs, probably
best to have a strategy here.
Tl;dr, I don't know what's the best approach to take, but just calling
into bpf_capable() deeply inside bpf_prog_alloc/bpf_prog_alloc_stats
makes subsequent work to implement trusted unpriv harder. Thoughts?
> > It probably has no effect, but anyone looking at crash dumps with drgn
> > will have a consistent view of aux->bpf_capable field.
>
> classic BPF predates my involvement with BPF, so I didn't even think
> about that. I'll check and make sure we do initialize aux->bpf_capable
> for that
