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?
> 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.
> + 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
>
