Re: [PATCH bpf-next 2/2] selftests/bpf: Cover small 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] Missing bounds check for the enum parameter `manip` in the `bpf_ct_set_nat_info` kfunc allows a BPF program to pass an arbitrary value, triggering a `WARN_ON` and corrupting the conntrack state.
--

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

selftests/bpf: Cover small conntrack opts error writes

This commit adds a conntrack kfunc regression check for opts__sz values
that do not cover opts->error. It verifies that the guard value remains
intact when the kfunc returns NULL due to an error.

> diff --git a/tools/testing/selftests/bpf/progs/test_bpf_nf.c b/tools/testing/selftests/bpf/progs/test_bpf_nf.c
> index 076fbf03a1268..df43649ecb785 100644
> --- a/tools/testing/selftests/bpf/progs/test_bpf_nf.c
> +++ b/tools/testing/selftests/bpf/progs/test_bpf_nf.c
[ ... ]
> @@ -124,6 +128,28 @@ nf_ct_test(struct nf_conn *(*lookup_fn)(void *, struct bpf_sock_tuple *, u32,
>  	else
>  		test_einval_len_opts = opts_def.error;
>  
> +	opts_def.error = CT_OPTS_ERROR_GUARD;
> +	ct = lookup_fn(ctx, &bpf_tuple, sizeof(bpf_tuple.ipv4), &opts_def,
> +		       sizeof(opts_def.netns_id));
> +	if (ct) {
> +		bpf_ct_release(ct);
> +		test_einval_len_opts_small_lookup = -EINVAL;
> +	} else {
> +		test_einval_len_opts_small_lookup = opts_def.error;
> +	}
> +
> +	opts_def.error = CT_OPTS_ERROR_GUARD;
> +	ct = alloc_fn(ctx, &bpf_tuple, sizeof(bpf_tuple.ipv4), &opts_def,
> +		      sizeof(opts_def.netns_id));
> +	if (ct) {
> +		ct = bpf_ct_insert_entry(ct);
> +		if (ct)
> +			bpf_ct_release(ct);
> +		test_einval_len_opts_small_alloc = -EINVAL;
> +	} else {
> +		test_einval_len_opts_small_alloc = opts_def.error;
> +	}
> +

[Severity: High]
This is a pre-existing issue, but while reviewing these conntrack kfunc
tests I noticed that bpf_ct_set_nat_info() lacks a bounds check for the
manip enum parameter.

Since the BPF verifier does not check the range of enum values passed to
kfuncs unless they are annotated with __const, could a BPF program pass an
arbitrary scalar value for the manip parameter?

Looking at net/netfilter/nf_nat_bpf.c:bpf_ct_set_nat_info():

    __bpf_kfunc int bpf_ct_set_nat_info(struct nf_conn___init *nfct,
                                        union nf_inet_addr *addr, int port,
                                        enum nf_nat_manip_type manip)
    {
        ...
        return nf_nat_setup_info(ct, &range, manip) == NF_DROP ? -ENOMEM : 0;
    }

If an invalid value is passed directly to nf_nat_setup_info(), would it
trigger the WARN_ON(maniptype != NF_NAT_MANIP_SRC && maniptype !=
NF_NAT_MANIP_DST) and fall through the default branches?

It seems this could result in the connection status being inconsistently set,
potentially corrupting the conntrack state or acting as a local denial of
service vector if panic_on_warn is enabled.

>  	opts_def.l4proto = IPPROTO_ICMP;
>  	ct = lookup_fn(ctx, &bpf_tuple, sizeof(bpf_tuple.ipv4), &opts_def,
>  		       sizeof(opts_def));

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