Re: [PATCH] bpf: fix out-of-bounds write in bpf_bprintf_prepare with %pI4/%pI6

[Mykyta Yatsenko <mykyta.yatsenko5@gmail.com>]

Ibrahim Zein <zeroxjacks@gmail.com> writes:

> In bpf_bprintf_prepare(), the bounds check for %pI4 and %pI6 format
> specifiers uses sizeof_cur_ip (4 for IPv4, 16 for IPv6), which is the
> raw byte count of the IP address. However, snprintf() returns the
> length of the formatted string, not the raw bytes. For IPv4 this can
> be up to 15 characters (255.255.255.255) and for IPv6 up to 39.
>
> tmp_buf is then advanced by (err + 1) using the full string length,
> which can push tmp_buf past tmp_buf_end. The next iteration's bounds
> check underflows due to unsigned arithmetic and passes, allowing a
> write past the end of the per-CPU bin_args buffer.
>
> Fix this by checking against the maximum formatted string size:
> 16 bytes for IPv4 and 40 bytes for IPv6.
>
> Signed-off-by: Ibrahim Zein <zeroxjacks@gmail.com>
> ---
>  kernel/bpf/helpers.c                          |  2 +-
>  .../bpf/prog_tests/test_snprintf_ip.c         | 54 +++++++++++++
>  .../selftests/bpf/progs/test_snprintf_ip.c    | 77 +++++++++++++++++++
>  3 files changed, 132 insertions(+), 1 deletion(-)
>  create mode 100644 tools/testing/selftests/bpf/prog_tests/test_snprintf_ip.c
>  create mode 100644 tools/testing/selftests/bpf/progs/test_snprintf_ip.c
>
> diff --git a/kernel/bpf/helpers.c b/kernel/bpf/helpers.c
> index cb6d242bd..dcaa822ba 100644
> --- a/kernel/bpf/helpers.c
> +++ b/kernel/bpf/helpers.c
> @@ -930,7 +930,7 @@ int bpf_bprintf_prepare(const char *fmt, u32 fmt_size, const u64 *raw_args,
>  				goto nocopy_fmt;
>  
>  			sizeof_cur_ip = (fmt[i] == '4') ? 4 : 16;
> -			if (tmp_buf_end - tmp_buf < sizeof_cur_ip) {
> +			if (tmp_buf_end - tmp_buf < (size_t)((fmt[i] == '4') ? 16 : 40)) {
not sure if casting to size_it is needed here.
>  				err = -ENOSPC;
>  				goto out;
>  			}
> diff --git a/tools/testing/selftests/bpf/prog_tests/test_snprintf_ip.c b/tools/testing/selftests/bpf/prog_tests/test_snprintf_ip.c
> new file mode 100644
> index 000000000..5b000d6d1
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/prog_tests/test_snprintf_ip.c
> @@ -0,0 +1,54 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/* Copyright (c) 2026 Ibrahim Zein <zeroxjacks@gmail.com> */
> +#include <test_progs.h>
> +#include "test_snprintf_ip.skel.h"
> +
> +#define EXP_IP4_OUT  "192.168.1.1"
> +#define EXP_IP4_RET  sizeof(EXP_IP4_OUT)
> +
> +#define EXP_WORST_IP4_OUT  "255.255.255.255"
> +#define EXP_WORST_IP4_RET  sizeof(EXP_WORST_IP4_OUT)
> +
> +#define EXP_IP6_OUT  "ffff:ffff:ffff:ffff:ffff:ffff:ffff:ffff"
> +#define EXP_IP6_RET  sizeof(EXP_IP6_OUT)
> +
> +void test_snprintf_ip(void)
The test passes with or without the fix, I suggest to rewrite it to fail
on the base revision, otherwise it is not very useful.
> +{
> +	struct test_snprintf_ip *skel;
> +
> +	skel = test_snprintf_ip__open_and_load();
> +	if (!ASSERT_OK_PTR(skel, "skel_open"))
> +		return;
> +
> +	skel->bss->pid = getpid();
> +
> +	if (!ASSERT_OK(test_snprintf_ip__attach(skel), "skel_attach"))
> +		goto cleanup;
> +
> +	/* trigger tracepoint */
> +	usleep(1);
> +
> +	/* Test 1: normal IPv4 */
> +	ASSERT_STREQ(skel->bss->ip4_out, EXP_IP4_OUT, "ip4_out");
> +	ASSERT_EQ(skel->bss->ip4_ret, EXP_IP4_RET, "ip4_ret");
> +
> +	/* Test 2: normal IPv6 */
> +	ASSERT_STREQ(skel->bss->ip6_out, EXP_IP6_OUT, "ip6_out");
> +	ASSERT_EQ(skel->bss->ip6_ret, EXP_IP6_RET, "ip6_ret");
> +
> +	/* Test 3: worst-case IPv4 "255.255.255.255" */
> +	ASSERT_STREQ(skel->bss->worst_ip4_out, EXP_WORST_IP4_OUT,
> +		     "worst_ip4_out");
> +	ASSERT_EQ(skel->bss->worst_ip4_ret, EXP_WORST_IP4_RET,
> +		  "worst_ip4_ret");
> +
> +	/*
> +	 * Test 4: near-overflow scenario.
> +	 * Before fix: tmp_buf overflows, kernel may crash or corrupt memory.
> +	 * After fix: returns valid length without overflow.
> +	 */
> +	ASSERT_GE(skel->bss->near_overflow_ret, 0, "near_overflow_ret");
> +
> +cleanup:
> +	test_snprintf_ip__destroy(skel);
> +}
> diff --git a/tools/testing/selftests/bpf/progs/test_snprintf_ip.c b/tools/testing/selftests/bpf/progs/test_snprintf_ip.c
> new file mode 100644
> index 000000000..5471a2b8b
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/progs/test_snprintf_ip.c
> @@ -0,0 +1,77 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/* Copyright (c) 2026 Ibrahim Zein <zeroxjacks@gmail.com> */
> +#include <linux/bpf.h>
> +#include <bpf/bpf_helpers.h>
> +
> +/*
> + * Test that bpf_snprintf() with %pI4/%pI6 does not overflow the
> + * internal bin_args buffer when the buffer is nearly full.
> + *
> + * The bug: sizeof_cur_ip (4 for IPv4) was used for the bounds check,
> + * but snprintf() returns the full formatted string length (up to 15
> + * for "255.255.255.255"), pushing tmp_buf past tmp_buf_end.
> + */
> +
> +/* Output buffers */
> +char ip4_out[64] = {};
> +long ip4_ret = 0;
> +
> +char ip6_out[128] = {};
> +long ip6_ret = 0;
> +
> +/* Test %pI4 with worst-case IP (255.255.255.255) */
> +char worst_ip4_out[64] = {};
> +long worst_ip4_ret = 0;
> +
> +/* Return value for the near-overflow test */
> +long near_overflow_ret = 0;
> +
> +__u32 pid = 0;
> +
> +SEC("raw_tp/sys_enter")
> +int handler(const void *ctx)
> +{
> +	/* Worst-case IPv4: "255.255.255.255" = 15 chars */
> +	const __u8 ip4_worst[] = {255, 255, 255, 255};
> +	/* Normal IPv4 */
> +	const __u8 ip4_normal[] = {192, 168, 1, 1};
> +	/* IPv6 worst-case */
> +	const __u8 ip6_worst[] = {0xff, 0xff, 0xff, 0xff,
> +				   0xff, 0xff, 0xff, 0xff,
> +				   0xff, 0xff, 0xff, 0xff,
> +				   0xff, 0xff, 0xff, 0xff};
> +
> +	if ((int)bpf_get_current_pid_tgid() != pid)
> +		return 0;
> +
> +	/* Test 1: normal %pI4 usage */
> +	ip4_ret = BPF_SNPRINTF(ip4_out, sizeof(ip4_out),
> +			       "%pI4", &ip4_normal);
> +
> +	/* Test 2: normal %pI6 usage */
> +	ip6_ret = BPF_SNPRINTF(ip6_out, sizeof(ip6_out),
> +			       "%pI6", &ip6_worst);
> +
> +	/* Test 3: worst-case %pI4 "255.255.255.255" */
> +	worst_ip4_ret = BPF_SNPRINTF(worst_ip4_out, sizeof(worst_ip4_out),
> +				     "%pI4", &ip4_worst);
> +
> +	/*
> +	 * Test 4: near-overflow scenario.
> +	 * Fill bin_args with scalar args (%d), then use %pI4.
> +	 * Before the fix: tmp_buf overflows by 8 bytes.
> +	 * After the fix: correctly returns -ENOSPC or fits exactly.
> +	 *
> +	 * We use fewer args here since BPF_SNPRINTF macro has a limit,
> +	 * but this validates the format string parsing path.
> +	 */
> +	/* 11 x %d + 1 x %pI4 = 12 args (BPF_SNPRINTF max) */
> +	near_overflow_ret = BPF_SNPRINTF(NULL, 0,
> +		"%d %d %d %d %d %d %d %d %d %d %d %pI4",
> +		1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11,
> +		&ip4_worst);
> +
> +	return 0;
> +}
> +
> +char _license[] SEC("license") = "GPL";
> -- 
> 2.52.0