On Fri, May 05, 2023 at 12:08:21PM -0700, Andrii Nakryiko wrote:
> 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?
Indeed. cBPF is always allowed for unpriv.
In that sense the value prog->aux->bpf_capable for cBPF is meaningless.
Let's keep the patch as-is then.
