Re: [PATCH net v8 2/2] selftests: net: add TAP socket filter attach tests

[netdev-bot+sashiko@kernel.org]

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

Critical: 0 · High: 0 · Medium: 1 · Low: 2

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Medium] In the kernel commit "tun: keep a kernel copy of the socket
  filter program" (part of this series), the TUNATTACHFILTER case in…
- [Low] attach_filter_bad_prog_keeps_program() and
  attach_filter_bad_len_keeps_program() pass whether or not…
- [Low] In reattach_filter_without_user_buffer(), the comment says "The
  two checks below ... cannot tell a reinstalled filter from no filter:…

--

selftests: net: add TAP socket filter attach tests

This adds TAP selftests for TUNATTACHFILTER. One test reattaches a queue
after the user buffer holding the program has been made unreadable. Two
others check that a rejected program (bad length or bad instructions)
leaves the installed filter in place. The last two cover attaching a
queue without a filter, via IFF_NOFILTER or after TUNDETACHFILTER.

> diff --git a/tools/testing/selftests/net/tun.c b/tools/testing/selftests/net/tun.c
> index 34de2580e32ec..3730fba859a46 100644
> --- a/tools/testing/selftests/net/tun.c
> +++ b/tools/testing/selftests/net/tun.c

[ ... ]

> @@ -548,6 +550,253 @@ TEST_F(tun, reattach_close_delete)

[ ... ]

> +static int filter_get(int fd, struct sock_fprog *fp)
> +{
> +	return ioctl(fd, TUNGETFILTER, (void *)fp);
> +}

[Severity: Medium]
This is about the kernel side of this series ("tun: keep a kernel copy of
the socket filter program"), not this selftest. Can TUNGETFILTER now
return uninitialized kernel stack bytes to user space?

In __tun_chr_ioctl(), the TUNATTACHFILTER case declares fprog on the
stack without an initializer:

drivers/net/tun.c:__tun_chr_ioctl() {
    ...
	case TUNATTACHFILTER: {
		struct sock_fprog_kern fprog_kern;
		struct sock_fprog fprog;
    ...
		tun->fprog_kern = fprog_kern;
		tun->fprog = fprog;
    ...
}

tun_copy_filter() only writes the two named members:

	fprog->len = fp.len;
	fprog->filter = fp.filter;

struct sock_fprog is an unsigned short followed by a pointer. That leaves
6 bytes of padding on 64-bit (2 on 32-bit) that nothing writes.
TUNGETFILTER then copies the whole struct out:

		if (copy_to_user(argp, &tun->fprog, sizeof(tun->fprog)))

Before the series, tun->fprog was filled by copy_from_user() of the full
struct, so the padding came from user space.

Whether the padding really reaches tun->fprog depends on codegen, and
CONFIG_INIT_STACK_ALL_ZERO would hide it. Could fprog be zero-initialized,
or could tun_copy_filter() assign *fprog = fp instead?

The tests here zero gf before filter_get() and check only gf.len, so they
would not catch this.

[ ... ]

> +TEST_F(tun, reattach_filter_without_user_buffer)
> +{

[ ... ]

> +	ret = tun_attach(self->fd, self->ifname);
> +	EXPECT_EQ(ret, 0);
> +
> +	flags = tun_get_iff_flags(self->fd);
> +	EXPECT_GE(flags, 0);
> +	/* The queue is attached again, not left disabled */
> +	EXPECT_EQ(flags & IFF_DETACH_QUEUE, 0);
> +	EXPECT_EQ(flags & IFF_NOFILTER, 0);
> +
> +	/* The two checks below document the expected state, they cannot
> +	 * tell a reinstalled filter from no filter: tun_detach() leaves
> +	 * sk_filter alone, and TUNGETFILTER only returns the header.
> +	 */

[Severity: Low]
Is the first reason in this comment attached to the right check?

tun_detach() here uses TUNSETQUEUE with IFF_DETACH_QUEUE, which ends up
in __tun_detach(tfile, false). That path never calls sk_detach_filter(),
so the queue keeps its sk_filter while it is detached.

TUNGETIFF derives IFF_NOFILTER from the socket:

drivers/net/tun.c:__tun_chr_ioctl() {
    ...
		if (!tfile->socket.sk->sk_filter)
			ifr.ifr_flags |= IFF_NOFILTER;
    ...
}

So the IFF_NOFILTER check on self->fd just above this comment passes
whether or not tun_attach() reinstalled the program from fprog_kern.
"tun_detach() leaves sk_filter alone" explains that check, not the two
TUNGETFILTER checks below.

Could the comment be moved, or reworded, so that the IFF_NOFILTER check on
self->fd doesn't look like evidence of reinstallation? Only the
fresh-queue check later in the test actually shows that.

> +	EXPECT_EQ(filter_get(self->fd, &gf), 0);
> +	EXPECT_EQ(gf.len, ARRAY_SIZE(filter_accept));

[ ... ]

> +TEST_F(tun, attach_filter_bad_prog_keeps_program)
> +{
> +	struct sock_fprog gf = { 0 };
> +	struct sock_filter bad[] = {
> +		BPF_STMT(BPF_LD | BPF_W | BPF_LEN, 0),
> +		BPF_JUMP(BPF_JMP | BPF_JEQ | BPF_K, 0, 1, 0),
> +	};
> +	struct ifreq ifr = { 0 };
> +	short flags = 0;
> +	int fd;
> +
> +	ASSERT_EQ(filter_attach(self->fd, filter_accept,
> +				ARRAY_SIZE(filter_accept)), 0);
> +
> +	errno = 0;
> +	EXPECT_EQ(filter_attach(self->fd, bad, ARRAY_SIZE(bad)), -1);
> +	EXPECT_EQ(errno, EINVAL);

[Severity: Low]
Would this test, and attach_filter_bad_len_keeps_program() with len == 0,
still pass if the early validation in tun_copy_filter() were removed?

The fixture has two attached queues (self->fd and self->fd2). Without the
early !fp.len and bpf_prog_validate_classic() checks, tun_attach_filter()
would call sk_attach_filter_kern() on tfiles[0]. There bpf_prog_create()
rejects both len == 0 (bpf_check_basics_ok()) and this bad array
(bpf_check_classic()) with -EINVAL, before sk_filter is replaced.

With i == 0 the out_detach rollback does nothing, and the ioctl does not
update tun->fprog or tun->fprog_kern. So errno, gf.len and both
IFF_NOFILTER checks give the same result either way.

The early check only changes behaviour when tun->numqueues == 0, for
example after every queue has been detached with IFF_DETACH_QUEUE. In
that case the loop in tun_attach_filter() does not run. Without the
check, the bad program would be saved with filter_attached set, and
later attaches would fail.

Could a variant detach both queues before the bad TUNATTACHFILTER, so the
early rejection is actually covered? The partial-rollback path in
tun_attach_filter(), where a failure on queue k > 0 leaves queues 0..k-1
without a filter, is not exercised by any test either.

> +
> +	/* the program attached before is untouched */
> +	EXPECT_EQ(filter_get(self->fd, &gf), 0);
> +	EXPECT_EQ(gf.len, ARRAY_SIZE(filter_accept));

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261028093235.148649-1-clementwei90%40163.com