Re: [PATCH bpf-next 2/5] bpf: Introduce BPF_LOADER_LOAD_FD command

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [Critical] Information Leak / Out-of-Bounds Read in BPF verifier via loader_load_fd
- [High] Out-of-bounds Read / Denial of Service in ELF header validation
- [Medium] File Descriptor Hijacking Race Condition (TOCTOU)
- [Medium] API Misuse / Broken functionality on non-JIT configurations
- [Low] Direct cast of `__aligned_u64` to `void *` for `copy_to_user`
--

--- Patch [2]: [PATCH bpf-next 2/5] bpf: Introduce BPF_LOADER_LOAD_FD command ---
commit f9a928bc7f89cffc836753c26236211ed8039c51
Author: Thiébaud Weksteen <[email protected]>

bpf: Introduce BPF_LOADER_LOAD_FD command

This commit introduces the BPF_LOADER_LOAD_FD command to allow loading
and executing loader BPF programs directly from an ELF file.

> diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
> index 8d111da886553..d79cd63f9f7cd 100644
> --- a/kernel/bpf/syscall.c
> +++ b/kernel/bpf/syscall.c

[ ... ]

> @@ -6291,6 +6292,336 @@ static int prog_assoc_struct_ops(union bpf_attr *attr)

[ ... ]

> +static int bpf_elf_validity_cache_sechdrs(struct elf_info *info)
> +{
> +	Elf64_Shdr *sechdrs;
> +	Elf64_Shdr *shdr;
> +	int i;
> +	int err;
> +
> +	err = bpf_elf_validity_ehdr(info);
> +	if (err < 0)
> +		return err;
> +
> +	if (info->hdr->e_shentsize != sizeof(Elf64_Shdr)) {
> +		pr_err("Invalid ELF section header size\n");
> +		return -ENOEXEC;
> +	}
> +
> +	/*
> +	 * e_shnum is 16 bits, and sizeof(Elf64_Shdr) is
> +	 * known and small. So e_shnum * sizeof(Elf64_Shdr)
> +	 * will not overflow unsigned long on any platform.
> +	 */
> +	if (info->hdr->e_shoff >= info->len
> +	    || (info->hdr->e_shnum * sizeof(Elf64_Shdr) >
> +		info->len - info->hdr->e_shoff)) {
> +		pr_err("Invalid ELF section header overflow\n");
> +		return -ENOEXEC;
> +	}
> +
> +	sechdrs = (void *)info->hdr + info->hdr->e_shoff;
> +
> +	/*
> +	 * The code assumes that section 0 has a length of zero and
> +	 * an addr of zero, so check for it.
> +	 */
> +	if (sechdrs[0].sh_type != SHT_NULL

[Severity: High]
Does this bounds check properly handle the case where info->hdr->e_shnum
is 0?

If an attacker provides an ELF file with e_shnum set to 0 and e_shoff set
near the end of the file (e.g., info->len - 1), the size check evaluates
0 > 1, which is false. The code will then proceed to access sechdrs[0]
unconditionally.

Since info->hdr is backed by a vmalloc buffer, could this read past the
buffer boundary into the unmapped guard page and cause a panic?

[ ... ]

> +static int loader_load_fd(union bpf_attr *attr)
> +{

[ ... ]

> +	memset(&sattr, 0, sizeof(sattr));
> +	sattr.prog_type = BPF_PROG_TYPE_SYSCALL;
> +	sattr.license = (unsigned long) license;
> +	sattr.insns = (unsigned long) insns;
> +	sattr.insn_cnt = insns_sz / sizeof(struct bpf_insn);
> +	sattr.fd_array = (unsigned long) &map_fd;

[Severity: Critical]
Is it safe to pass a kernel stack pointer (&map_fd) via sattr.fd_array
while leaving fd_array_cnt implicitly as 0?

When fd_array_cnt is 0, the BPF verifier falls back to the legacy sparse
array resolution path in fd_array_get_map_idx_sparse(). In this path, the
verifier extracts an immediate value from a BPF_PSEUDO_MAP_FD instruction
and uses it as an index.

Since fd_array is treated as a kernel pointer, copy_from_bpfptr_offset()
expands directly to a memory copy:

    memcpy(&fd, &map_fd + idx, sizeof(int));

Because the index is attacker-controlled and lacks bounds checking, could
this allow an unprivileged user to reliably read arbitrary memory from the
kernel stack?

> +	sattr.prog_flags = BPF_F_SLEEPABLE;
> +	strscpy(sattr.prog_name, BPF_LOADER_PROG_SEC, sizeof(BPF_LOADER_PROG_SEC));
> +	prog_fd = kern_sys_bpf(BPF_PROG_LOAD, &sattr, sizeof(sattr));
> +	if (prog_fd < 0) {
> +		err = prog_fd;
> +		goto close_map_err;
> +	}
> +
> +	memset(&sattr, 0, sizeof(sattr));
> +	ctx_sz = attr->load_fd.ctx_size;
> +	kctx = kzalloc(ctx_sz, GFP_KERNEL);
> +	if (kctx == NULL) {
> +		err = -ENOMEM;
> +		goto close_prog_err;
> +	}
> +	sattr.test.prog_fd = prog_fd;
> +	sattr.test.ctx_in = (unsigned long) kctx;
> +	sattr.test.ctx_size_in = ctx_sz;
> +	err = kern_sys_bpf(BPF_PROG_TEST_RUN, &sattr, sizeof(sattr));

[Severity: Medium]
Does kern_sys_bpf() support BPF_PROG_TEST_RUN when CONFIG_BPF_JIT is
disabled?

Looking at kern_sys_bpf(), it only intercepts BPF_PROG_TEST_RUN when
CONFIG_BPF_JIT is enabled. Without JIT, it falls through to ____bpf_sys_bpf()
which explicitly rejects BPF_PROG_TEST_RUN and returns -EINVAL.

Could this cause the BPF_LOADER_LOAD_FD command to be completely broken on
non-JIT kernel configurations?

> +	if (err < 0)
> +		goto free_ctx;
> +	err = sattr.test.retval;
> +	if (err < 0)
> +		goto free_ctx;
> +
> +	if (copy_to_user((void *) attr->load_fd.ctx, kctx, ctx_sz) != 0)

[Severity: Low]
Is it correct to directly cast __aligned_u64 to a void pointer here?

Directly casting attr->load_fd.ctx to (void *) bypasses Sparse address space
annotations for __user memory. On 32-bit architectures, this can also cause
pointer truncation warnings.

Should this use the u64_to_user_ptr() macro instead?

> +		err = -EFAULT;
> +
> +free_ctx:
> +	kfree(kctx);
> +close_prog_err:
> +	close_fd(prog_fd);
> +close_map_err:
> +	close_fd(map_fd);

[Severity: Medium]
Could unconditionally closing these file descriptors introduce a race
condition?

The kern_sys_bpf() function allocates file descriptors and installs them,
making them temporarily visible in the calling process's file descriptor
table.

If a concurrent thread blocks copy_to_user() (for instance, by passing a
userfaultfd-backed memory region for attr->load_fd.ctx), it could close
these exposed file descriptors and open new ones.

When the page fault resolves, would loader_load_fd() resume and inadvertently
close the newly opened files that now occupy those descriptor slots?

> +out_free_buf:
> +	vfree(buf);
> +out:
> +	return err;
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=2