Re: [PATCH bpf-next 1/2] bpf: Guard conntrack opts error writes

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [High] TOCTOU Vulnerability / Use-After-Free due to multiple reads of BPF-provided `opts->netns_id`
--

commit 8efa13ac7f8992d846ab4af35e05c540c530390f
Author: Yiyang Chen <chenyy23@mails.tsinghua.edu.cn>

bpf: Guard conntrack opts error writes

This commit updates the BPF conntrack lookup and allocation wrappers to
conditionally write to opts->error. It ensures the write only occurs if
the provided opts__sz is large enough to include the error field.

> diff --git a/net/netfilter/nf_conntrack_bpf.c b/net/netfilter/nf_conntrack_bpf.c
> index 40c261cd0af38..3c182024ec509 100644
> --- a/net/netfilter/nf_conntrack_bpf.c
> +++ b/net/netfilter/nf_conntrack_bpf.c
[ ... ]
> @@ -298,7 +303,8 @@ bpf_xdp_ct_alloc(struct xdp_md *xdp_ctx, struct bpf_sock_tuple *bpf_tuple,
>  	nfct = __bpf_nf_ct_alloc_entry(dev_net(ctx->rxq->dev), bpf_tuple, tuple__sz,
>  				       opts, opts__sz, 10);

[Severity: High]
This is a pre-existing issue, but does __bpf_nf_ct_alloc_entry() (and
similarly __bpf_nf_ct_lookup()) have a time-of-check to time-of-use
(TOCTOU) vulnerability regarding opts->netns_id?

The BPF verifier allows the opts argument to point to memory that can be
concurrently modified by a BPF program. Inside __bpf_nf_ct_alloc_entry(),
opts->netns_id is read multiple times:

net/netfilter/nf_conntrack_bpf.c:__bpf_nf_ct_alloc_entry() {
    ...
    if (opts->netns_id >= 0) {
        net = get_net_ns_by_id(net, opts->netns_id);
    ...
out:
    if (opts->netns_id >= 0)
        put_net(net);
}

Since function calls like get_net_ns_by_id() act as compiler barriers,
the value will be reloaded from memory. If a concurrent thread modifies
opts->netns_id from a negative value to a valid ID between the initial
check and the final put_net() check, could the function drop a reference
it never acquired, leading to a refcount underflow and use-after-free?

Similarly, changing it from a valid ID to a negative value could cause
a reference leak. Could we cache the value of opts->netns_id in a local
variable to ensure the same value is used for both checks?

>  	if (IS_ERR(nfct)) {
> -		opts->error = PTR_ERR(nfct);
> +		if (bpf_ct_opts_has_error(opts__sz))
> +			opts->error = PTR_ERR(nfct);
>  		return NULL;
>  	}
>

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1781586477.git.chenyy23@mails.tsinghua.edu.cn?part=1