On Wed, May 3, 2023 at 3:33 PM Stanislav Fomichev <sdf@xxxxxxxxxx> wrote: > > On Wed, May 3, 2023 at 12:04 PM Andrii Nakryiko > <andrii.nakryiko@xxxxxxxxx> wrote: > > > > On Wed, May 3, 2023 at 11:28 AM Stanislav Fomichev <sdf@xxxxxxxxxx> wrote: > > > > > > On 05/02, Andrii Nakryiko wrote: > > > > Make each bpf() syscall command a bit more self-contained, making it > > > > easier to further enhance it. We move sysctl_unprivileged_bpf_disabled > > > > handling down to map_create() and bpf_prog_load(), two special commands > > > > in this regard. > > > > > > > > Also swap the order of checks, calling bpf_capable() only if > > > > sysctl_unprivileged_bpf_disabled is true, avoiding unnecessary audit > > > > messages. > > > > > > > > Signed-off-by: Andrii Nakryiko <andrii@xxxxxxxxxx> > > > > --- > > > > kernel/bpf/syscall.c | 37 ++++++++++++++++++++++--------------- > > > > 1 file changed, 22 insertions(+), 15 deletions(-) > > > > > > > > diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c > > > > index 14f39c1e573e..d5009fafe0f4 100644 > > > > --- a/kernel/bpf/syscall.c > > > > +++ b/kernel/bpf/syscall.c > > > > @@ -1132,6 +1132,17 @@ static int map_create(union bpf_attr *attr) > > > > int f_flags; > > > > int err; > > > > > > [..] > > > > > > > + /* Intent here is for unprivileged_bpf_disabled to block key object > > > > + * creation commands for unprivileged users; other actions depend > > > > + * of fd availability and access to bpffs, so are dependent on > > > > + * object creation success. Capabilities are later verified for > > > > + * operations such as load and map create, so even with unprivileged > > > > + * BPF disabled, capability checks are still carried out for these > > > > + * and other operations. > > > > + */ > > > > + if (sysctl_unprivileged_bpf_disabled && !bpf_capable()) > > > > + return -EPERM; > > > > + > > > > > > Does it make sense to have something like unpriv_bpf_capable() to avoid > > > the copy-paste? > > > > It's a simple if condition used in exactly two places, so I had > > preference to keep permissions checks grouped together in the same > > block of code in respective MAP_CREATE and PROG_LOAD handlers. I can > > factor it out into a function, but I see little value in that, tbh. > > Ack, up to you. I'm more concerned about the comment tbh, not the check itself. I'll trim those comments down and keep relevant map *or* prog parts.
