Re: [PATCH v2 1/4] tun: fix inverted error check when re-attaching the filter

[netdev-bot+sashiko@kernel.org]

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

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

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] This patch un-inverts the check in tun_attach()
  (drivers/net/tun.c), so a failed filter re-attach now aborts TUNSETIFF…

--

tun: fix inverted error check when re-attaching the filter

This patch fixes the inverted sk_attach_filter() error check in
tun_attach(). Before, a successful filter re-attach returned early and the
queue was never published in tun->tfiles[]. Now a failed re-attach aborts
the attach, and the filter is detached again if a later step of
tun_attach() fails.

> diff --git a/drivers/net/tun.c b/drivers/net/tun.c
> index 5a302709a68aa..a2fffef3735fe 100644
> --- a/drivers/net/tun.c
> +++ b/drivers/net/tun.c

[ ... ]

> @@ -754,8 +755,9 @@ static int tun_attach(struct tun_struct *tun, struct file *file,
>  		lock_sock(tfile->socket.sk);
>  		err = sk_attach_filter(&tun->fprog, tfile->socket.sk);
>  		release_sock(tfile->socket.sk);
> -		if (!err)
> +		if (err)
>  			goto out;
> +		rollback_filter = true;
>  	}

[Severity: Medium]
A failed re-attach is now fatal. Can that make TUNSETIFF, or TUNSETQUEUE
with IFF_ATTACH_QUEUE, fail on a persistent TAP that has a filter
attached?

tun->fprog only holds the {len, filter} header saved at TUNATTACHFILTER
time, and filter is still a __user pointer:

drivers/net/tun.c:__tun_chr_ioctl() {
    ...
		ret = -EFAULT;
		if (copy_from_user(&tun->fprog, argp, sizeof(tun->fprog)))
			break;
    ...
}

sk_attach_filter() then reads the instructions again, from the address
space of whichever task is doing the attach:

net/core/filter.c:__get_filter() {
    ...
	if (copy_from_user(prog->insns, fprog->filter, fsize))
    ...
}

Consider this sequence:

1. Process A issues TUNATTACHFILTER.
2. A exits or frees the buffer.
3. Process B issues TUNSETIFF (without IFF_NOFILTER) or IFF_ATTACH_QUEUE.

The copy in step 3 fails with -EFAULT, or the program fails validation
with -EINVAL, and the attach is aborted. Before this patch the attach went
ahead without a filter. If the stale address happens to be mapped in B,
the filter is built from whatever bytes are at that address.

tun_set_queue() always passes skip_filter=false, so IFF_ATTACH_QUEUE has
no way to opt out:

		ret = tun_attach(tun, file, false, tun->flags & IFF_NAPI,
				 tun->flags & IFF_NAPI_FRAGS, true);

A later patch in this series, "tun: keep a kernel copy of the socket
filter program", seems to fix this:

- At TUNATTACHFILTER time, tun_copy_filter() copies the instructions into
  tun->fprog_kern.
- tun_attach() and tun_attach_filter() switch to
  sk_attach_filter_kern(&tun->fprog_kern, ...).
- The selftest reattach_filter_without_user_buffer() covers this case.

Would it make sense to put that patch before this one? That way the
stale user pointer read is never fatal at any point in the series.

A smaller gap is still there at the end of the series. It predates this
patch. If TUNATTACHFILTER is issued from a detached queue fd while
numqueues is 0, tun_attach_filter() loops zero times. It then sets
filter_attached = true without running bpf_check_classic(). Later
attaches fail with -EINVAL until TUNDETACHFILTER is issued.

[ ... ]

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