Re: [PATCH bpf-next] bpf: add BPF_F_SEALED flag to seal BPF links at creation

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] The BPF_F_SEALED flag is applied via a file descriptor lookup after the FD has been exposed to user space, creating a TOCTOU race that allows cross-object sabotage and unauthorized link sealing.
- [High] The implementation of BPF_F_SEALED introduces a permanent memory leak, enabling a Denial of Service (DoS) via kernel resource exhaustion.
- [Medium] Concurrent executions of `link_seal_fd` on the same link can leak multiple self-references due to a missing atomic read-modify-write.
--

--- Patch [1]: [PATCH bpf-next] bpf: add BPF_F_SEALED flag to seal BPF links at creation ---
commit f9d98fb378be461fdf9509562339b45a07b8d912
Author: David Windsor <[email protected]>

bpf: add BPF_F_SEALED flag to seal BPF links at creation

This commit introduces a new BPF_F_SEALED flag for BPF_LINK_CREATE that
creates a permanently sealed link. A sealed link cannot have its program
replaced, cannot be detached, and holds an extra self-reference so it
persists until reboot.

> diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
> index 94091130bcc50..e8bd57d5c907a 100644
> --- a/kernel/bpf/syscall.c
> +++ b/kernel/bpf/syscall.c
[ ... ]
> @@ -5776,17 +5777,41 @@ static int bpf_map_do_batch(const union bpf_attr *attr,
>  	return err;
>  }
>  
> +/* Seal the just-created link: take a self-reference that is never released. */
> +static void link_seal_fd(int fd)
> +{
> +	struct bpf_link *link;
> +
> +	link = bpf_link_get_from_fd(fd);
> +	if (IS_ERR(link))
> +		return;
> +
> +	if (!READ_ONCE(link->sealed)) {

[Severity: Medium]
Is this flag check safe against concurrent execution?

If the file descriptor is raced via dup2(), multiple threads might enter
link_seal_fd() for the same victim link concurrently. Since the READ_ONCE()
and WRITE_ONCE() are not atomic, could multiple threads observe false and
increment the link reference count multiple times?

> +		bpf_link_inc(link);
> +		WRITE_ONCE(link->sealed, true);

[Severity: High]
Does taking a self-reference here without any release path allow users to
exhaust kernel memory?

Since link_detach() explicitly returns -EPERM for sealed links, a user could
repeatedly create sealed links and close the fds, leaking the bpf_link and
bpf_prog indefinitely. Is there an administrative way to reclaim this memory?

> +	}
> +
> +	bpf_link_put_direct(link);
> +}
[ ... ]
> @@ -5880,6 +5905,9 @@ static int link_create(union bpf_attr *attr, bpfptr_t uattr)
>  out:
>  	if (ret < 0)
>  		bpf_prog_put(prog);
> +out_seal:
> +	if (ret >= 0 && seal)
> +		link_seal_fd(ret);

[Severity: High]
Does looking up the link from the file descriptor here create a race
condition?

By the time link_create() reaches this point, attach functions called
earlier have already installed the fd into the process's file descriptor
table.

Could a concurrent thread use dup2() to overwrite this fd with one pointing
to a different victim bpf_link before link_seal_fd() executes? This would
cause the system to permanently seal the victim link instead of the newly
created one.

>  	return ret;
>  }

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