On Thu, May 4, 2023 at 1:13 PM Alexei Starovoitov
<alexei.starovoitov@xxxxxxxxx> wrote:
>
> On Tue, May 02, 2023 at 04:06:15PM -0700, Andrii Nakryiko wrote:
> > Move out flags validation and license checks out of the permission
> > checks. They were intermingled, which makes subsequent changes harder.
> > Clean this up: perform straightforward flag validation upfront, and
> > fetch and check license later, right where we use it. Also consolidate
> > capabilities check in one block, right after basic attribute sanity
> > checks.
> >
> > Signed-off-by: Andrii Nakryiko <andrii@xxxxxxxxxx>
> > ---
> > kernel/bpf/syscall.c | 44 +++++++++++++++++++++-----------------------
> > 1 file changed, 21 insertions(+), 23 deletions(-)
> >
> > diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
> > index 2e5ca52c45c4..d960eb476754 100644
> > --- a/kernel/bpf/syscall.c
> > +++ b/kernel/bpf/syscall.c
> > @@ -2573,18 +2573,6 @@ static int bpf_prog_load(union bpf_attr *attr, bpfptr_t uattr, u32 uattr_size)
> > struct btf *attach_btf = NULL;
> > int err;
> > char license[128];
> > - bool is_gpl;
> > -
> > - /* 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;
> >
> > if (CHECK_ATTR(BPF_PROG_LOAD))
> > return -EINVAL;
> > @@ -2598,21 +2586,22 @@ static int bpf_prog_load(union bpf_attr *attr, bpfptr_t uattr, u32 uattr_size)
> > BPF_F_XDP_DEV_BOUND_ONLY))
> > return -EINVAL;
> >
> > + /* 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;
> > +
>
> Since that part was done in patch 1. Move it into right spot in patch 1 right away?
fair enough, will do it right away in patch 1
>
> > if (!IS_ENABLED(CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS) &&
> > (attr->prog_flags & BPF_F_ANY_ALIGNMENT) &&
> > !bpf_capable())
> > return -EPERM;
> >
> > - /* copy eBPF program license from user space */
> > - if (strncpy_from_bpfptr(license,
> > - make_bpfptr(attr->license, uattr.is_kernel),
> > - sizeof(license) - 1) < 0)
> > - return -EFAULT;
> > - license[sizeof(license) - 1] = 0;
> > -
> > - /* eBPF programs must be GPL compatible to use GPL-ed functions */
> > - is_gpl = license_is_gpl_compatible(license);
> > -
> > if (attr->insn_cnt == 0 ||
> > attr->insn_cnt > (bpf_capable() ? BPF_COMPLEXITY_LIMIT_INSNS : BPF_MAXINSNS))
> > return -E2BIG;
> > @@ -2700,7 +2689,16 @@ static int bpf_prog_load(union bpf_attr *attr, bpfptr_t uattr, u32 uattr_size)
> > prog->jited = 0;
> >
> > atomic64_set(&prog->aux->refcnt, 1);
> > - prog->gpl_compatible = is_gpl ? 1 : 0;
> > +
> > + /* copy eBPF program license from user space */
> > + if (strncpy_from_bpfptr(license,
> > + make_bpfptr(attr->license, uattr.is_kernel),
> > + sizeof(license) - 1) < 0)
> > + return -EFAULT;
>
> This 'return' is incorrect. It misses lots of cleanup.
> Should probably be 'goto free_prog_sec', but pls double check.
> We don't have tests to check error handling. Just lucky code review.
>
yep, lucky indeed. My bad, I should have adjusted it to `goto free_prog_sec;`.
> > + license[sizeof(license) - 1] = 0;
> > +
> > + /* eBPF programs must be GPL compatible to use GPL-ed functions */
> > + prog->gpl_compatible = license_is_gpl_compatible(license) ? 1 : 0;
> >
> > if (bpf_prog_is_dev_bound(prog->aux)) {
> > err = bpf_prog_dev_bound_init(prog, attr);
> > --
> > 2.34.1
> >
