[PATCH bpf] selftests/bpf: Add tests to assert that netfilter progs cannot write to skb

[PATCH bpf] selftests/bpf: Add tests to assert that netfilter progs cannot write to skb

[Florian Westphal]

The netfilter framework is allergic to ip header changing after
validation done by ip/ipv6 stack.

Assert that bpf netfilter program type exposes readonly access to the
skb, either via direct packet access or via dynptr API.

Following additional tests are expected to be rejected by verifier:

1. alter skb->len.
2. alter skb->data.
3. prog calls bpf_dynptr_slice_rdwr.
4. alter location returned by dynptr API.

Add following test case for bpf runtime:
- alter skb data via bpf_dynptr_write()

Test checks via __retval() that bpf_dynptr_write() returned nonzero value.

Signed-off-by: Florian Westphal <fw@strlen.de>
---
 .../bpf/progs/verifier_netfilter_ctx.c        | 78 +++++++++++++++++++
 1 file changed, 78 insertions(+)

diff --git a/tools/testing/selftests/bpf/progs/verifier_netfilter_ctx.c b/tools/testing/selftests/bpf/progs/verifier_netfilter_ctx.c
index e2cbc5bda65e..7e304331409f 100644
--- a/tools/testing/selftests/bpf/progs/verifier_netfilter_ctx.c
+++ b/tools/testing/selftests/bpf/progs/verifier_netfilter_ctx.c
@@ -113,4 +113,82 @@ int with_valid_ctx_access_test6(struct bpf_nf_ctx *ctx)
 	return th->dest == bpf_htons(22) ? NF_ACCEPT : NF_DROP;
 }
 
+SEC("netfilter")
+__description("netfilter test prog with skb write access")
+__failure __msg("only read is supported")
+int skb_len_write(struct bpf_nf_ctx *ctx)
+{
+	ctx->skb->len = 1;
+	return 1;
+}
+
+SEC("netfilter")
+__description("netfilter test prog with skb data write access")
+__failure __msg("cannot write into rdonly_untrusted_mem")
+int with_invalid_data_write_test4(struct bpf_nf_ctx *ctx)
+{
+	ctx->skb->data[0] = 0;
+	return 1;
+}
+
+SEC("netfilter")
+__description("netfilter test prog with bpf_dynptr_write")
+__success __failure_unpriv
+__retval(0)
+int with_dynptr_write(struct bpf_nf_ctx *ctx)
+{
+	struct __sk_buff *skb = (struct __sk_buff *)ctx->skb;
+	struct bpf_dynptr ptr;
+	u8 buffer[1] = {};
+
+	if (bpf_dynptr_from_skb(skb, 0, &ptr))
+		return 1;
+
+	if (bpf_dynptr_write(&ptr, 0, buffer, sizeof(buffer), 0))
+		return 0; /* must always fail */
+
+	return 1;
+}
+
+SEC("netfilter")
+__description("netfilter test prog with bpf_dynptr_slice_rdwr")
+__failure __msg("the prog does not allow writes to packet data")
+int with_dynptr_rdwr(struct bpf_nf_ctx *ctx)
+{
+	struct __sk_buff *skb = (struct __sk_buff *)ctx->skb;
+	u8 buffer_iph[20] = {};
+	struct bpf_dynptr ptr;
+	struct iphdr *iph;
+
+	if (bpf_dynptr_from_skb(skb, 0, &ptr))
+		return 1;
+
+	iph = bpf_dynptr_slice_rdwr(&ptr, 0, buffer_iph, sizeof(buffer_iph));
+	if (!iph)
+		return 0;
+
+	return 1;
+}
+
+SEC("netfilter")
+__description("netfilter test prog with bpf dynptr_slice_write + write")
+__failure __msg("cannot write into rdonly_mem")
+int with_dynptr_store(struct bpf_nf_ctx *ctx)
+{
+	struct __sk_buff *skb = (struct __sk_buff *)ctx->skb;
+	u8 buffer_iph[20] = {};
+	struct bpf_dynptr ptr;
+	struct iphdr *iph;
+
+	if (bpf_dynptr_from_skb(skb, 0, &ptr))
+		return 1;
+
+	iph = bpf_dynptr_slice(&ptr, 0, buffer_iph, sizeof(buffer_iph));
+	if (!iph)
+		return 0;
+	iph->protocol = 42;
+
+	return 1;
+}
+
 char _license[] SEC("license") = "GPL";
-- 
2.55.0

Re: [PATCH bpf] selftests/bpf: Add tests to assert that netfilter progs cannot write to skb

[bot+bpf-ci]

[-- Attachment #1: Type: text/plain, Size: 2799 bytes --]

> diff --git a/tools/testing/selftests/bpf/progs/verifier_netfilter_ctx.c b/tools/testing/selftests/bpf/progs/verifier_netfilter_ctx.c
> index e2cbc5bda65e..7e304331409f 100644
> --- a/tools/testing/selftests/bpf/progs/verifier_netfilter_ctx.c
> +++ b/tools/testing/selftests/bpf/progs/verifier_netfilter_ctx.c
> @@ -113,4 +113,82 @@ int with_valid_ctx_access_test6(struct bpf_nf_ctx *ctx)
>  	return th->dest == bpf_htons(22) ? NF_ACCEPT : NF_DROP;
>  }
>
> +SEC("netfilter")
> +__description("netfilter test prog with skb write access")
> +__failure __msg("only read is supported")
> +int skb_len_write(struct bpf_nf_ctx *ctx)
> +{
> +	ctx->skb->len = 1;
> +	return 1;
> +}
> +
> +SEC("netfilter")
> +__description("netfilter test prog with skb data write access")
> +__failure __msg("cannot write into rdonly_untrusted_mem")
> +int with_invalid_data_write_test4(struct bpf_nf_ctx *ctx)

This isn't a bug, but would a name like skb_data_write read better here,
given with_invalid_ctx_access_test4() already exists in this file and the
other new progs carry no test number?

> +{
> +	ctx->skb->data[0] = 0;
> +	return 1;
> +}

[ ... ]

> +SEC("netfilter")
> +__description("netfilter test prog with bpf dynptr_slice_write + write")
> +__failure __msg("cannot write into rdonly_mem")
> +int with_dynptr_store(struct bpf_nf_ctx *ctx)

This isn't a bug, but should this description say bpf_dynptr_slice() (the
helper the prog actually calls) rather than 'bpf dynptr_slice_write'? The
neighbouring descriptions spell the callee exactly ('netfilter test prog
with bpf_dynptr_write', 'netfilter test prog with bpf_dynptr_slice_rdwr'),
and no helper named dynptr_slice_write exists.

> +{
> +	struct __sk_buff *skb = (struct __sk_buff *)ctx->skb;
> +	u8 buffer_iph[20] = {};
> +	struct bpf_dynptr ptr;
> +	struct iphdr *iph;
> +
> +	if (bpf_dynptr_from_skb(skb, 0, &ptr))
> +		return 1;
> +
> +	iph = bpf_dynptr_slice(&ptr, 0, buffer_iph, sizeof(buffer_iph));
> +	if (!iph)
> +		return 0;
> +	iph->protocol = 42;
> +
> +	return 1;
> +}

Regarding the commit message:

>     Assert that bpf netfilter program type exposes readonly access to the
>     skb, either via direct packet access or via dynptr API.

This isn't a bug, but since netfilter progs have no direct packet access,
would it be clearer to describe this case as writing through ctx->skb (BTF
walk) instead of 'direct packet access'? The test (ctx->skb->data[0] = 0)
walks a BTF pointer, which is why the expected message is 'cannot write
into rdonly_untrusted_mem' rather than a packet-access error.


---
AI reviewed your patch. Please fix the bug or email reply why it's not a bug.
See: https://github.com/kernel-patches/vmtest/blob/master/ci/claude/README.md

CI run summary: https://github.com/kernel-patches/bpf/actions/runs/33601668670

Re: [PATCH bpf] selftests/bpf: Add tests to assert that netfilter progs cannot write to skb

[Florian Westphal]

bot+bpf-ci@kernel.org <bot+bpf-ci@kernel.org> wrote:
> >     Assert that bpf netfilter program type exposes readonly access to the
> >     skb, either via direct packet access or via dynptr API.

> This isn't a bug, but since netfilter progs have no direct packet access,
> would it be clearer to describe this case as writing through ctx->skb (BTF
> walk) instead of 'direct packet access'? The test (ctx->skb->data[0] = 0)
> walks a BTF pointer, which is why the expected message is 'cannot write
> into rdonly_untrusted_mem' rather than a packet-access error.

I'll shorten this to:

"Assert that bpf netfilter programs do not allow skb write access."

I'll also follow the other suggested changes to function names.