[PATCH net v6 0/2] tun: fix re-attaching the socket filter

[PATCH net v6 0/2] tun: fix re-attaching the socket filter

[Rongguang Wei]

From: Rongguang Wei <weirongguang@kylinos.cn>

This is v6 of the series that fixes attaching a queue to a TAP device which
re-installs the socket filter of a persistent device.

Patch 1 keeps a copy of the program in the kernel when it is configured,
instead of reading tun->fprog from user space again on every later attach,
and fixes the inverted error check in tun_attach(). The two changes are
one patch on purpose: with only the kernel copy every re-attach returns 0
without publishing the queue, and with only the check fixed a re-attach
that used to succeed without a filter starts to fail. IFF_NOFILTER keeps
working and is no longer needed as a workaround.

Patch 2 adds selftests: it re-attaches a queue after the buffer the program
was copied from was made unreadable, attaches a second queue from a fresh
socket to check that the filter is rebuilt from the kernel copy, and checks
that a rejected TUNATTACHFILTER keeps the saved program, plus the ways to
attach a queue without a filter.

Thanks to Willem de Bruijn for the reviews, and to the CI reviews for
the comments they raised.

Rongguang Wei (2):
  tun: keep a kernel copy of the socket filter program
  selftests: net: add TAP socket filter attach tests

---
v6:
  - add description for return value of sk_attach_filter_kern and No code
    change is involved.
v5:
  - add kernel-doc for sk_attach_filter_kern() describing what the missing
    original program means for SO_GET_FILTER and sock_diag
  - mention in the message that a rejected header now leaves the installed
    filter alone
  - fix the wording of the mprotect() comment and note what the IFF_NOFILTER
    and TUNGETFILTER checks in the re-attach test
  - https://lore.kernel.org/netdev/20261012021113.45939-1-clementwei90@163.com/
v4:
  - merge the kernel copy and the error check into one patch, so that no
    step of the series turns a re-attach into a silent no-op or into a new
    failure
  - charge the kernel copy to the caller's memory cgroup
  - selftests: check that the queue is attached again, and attach a new
    queue to see that the filter was installed from the kernel copy
  - https://lore.kernel.org/netdev/20261003063859.136895-1-clementwei90@163.com/
v3:
  - reorder: the kernel copy of the program comes before the corrected
    error check
  - add sk_attach_filter_kern() in the patch that first uses it
  - address the review of the selftests
  - https://lore.kernel.org/netdev/20260930080746.135017-1-clementwei90@163.com/
v2:
  - roll back the filter attach when a later step of tun_attach() fails
  - keep a copy of the program in the kernel, so that a later attach does
    not depend on the address space of the process that set the filter
  - add selftests for the re-attach and for a rejected TUNATTACHFILTER
  - https://lore.kernel.org/netdev/20260929093712.131096-1-clementwei90@163.com/
v1:
  - https://lore.kernel.org/netdev/20260923025653.59348-1-clementwei90@163.com/
---
 drivers/net/tun.c                 |  56 ++++++++-
 include/linux/filter.h            |   1 +
 net/core/filter.c                 |  41 +++++++
 tools/testing/selftests/net/tun.c | 197 ++++++++++++++++++++++++++++++
 4 files changed, 290 insertions(+), 5 deletions(-)

base-commit: 0fcb05b72344f40806cbe810de8872f1698bf053
-- 
2.25.1


No virus found
		Checked by Hillstone Network AntiVirus

[PATCH net v6 1/2] tun: keep a kernel copy of the socket filter program

[Rongguang Wei]

From: Rongguang Wei <weirongguang@kylinos.cn>

TUNATTACHFILTER copies only the sock_fprog header, so tun->fprog.filter
stays a pointer into the address space of the process that issued the
ioctl. tun_attach() reads it again whenever a queue is attached to the
persistent device later on.

Rebuilding the filter from that pointer is not reliable. With the inverted
check below, a failed read (-EFAULT for an unmapped address, -EINVAL for
bytes that are not a valid classic BPF program) was not fatal: for a new
tfile err is overwritten by "err = 0", so the queue was attached without a
filter. And when the read succeeded, the early return meant the queue was
never published at all.

Keep the program in the kernel instead: tun->fprog_kern holds the
instructions, and each queue gets its own program built from it with the
new sk_attach_filter_kern(), the kernel memory counterpart of
sk_attach_filter(). The copy is freed when the filter is detached or
replaced and with the device, and tun->fprog is left untouched so
TUNGETFILTER keeps its uapi behaviour.

Also fix the inverted check at the same time when tun_attach() returns
early when sk_attach_filter_kern() succeeds instead of when it fails.
Neither change works on its own: with only the kernel copy, every re-attach
returns 0 without publishing the queue; with only the check fixed, a
re-attach that used to succeed without installing a filter would start to
fail.

sk_attach_filter_kern() builds the program with bpf_prog_create(), which
does not keep an original program, so SO_GET_FILTER returns -EACCES and
sock_diag omits the filter for sockets that use it. tun sockets are not
exposed as file descriptors, so this is not user visible.

Because the length is checked in tun_copy_filter() before anything is
replaced, a TUNATTACHFILTER with a bad header now fails early and leaves
the previously attached filter in place; before this series the failure
happened on queue 0 and cleared filter_attached.

The inverted check was discovered by manual code inspection first [1], and
the review of v1 reported the other things.

[1] https://lore.kernel.org/netdev/20260923025653.59348-1-clementwei90@163.com/

Fixes: 54f968d6efdb ("tuntap: move socket to tun_file")
Link: https://lore.kernel.org/netdev/179027275318.2160803.4185895144088175048@kernel.org/
Signed-off-by: Rongguang Wei <weirongguang@kylinos.cn>
Reviewed-by: Willem de Bruijn <willemb@google.com>
---
 drivers/net/tun.c      | 56 ++++++++++++++++++++++++++++++++++++++----
 include/linux/filter.h |  1 +
 net/core/filter.c      | 41 +++++++++++++++++++++++++++++++
 3 files changed, 93 insertions(+), 5 deletions(-)

diff --git a/drivers/net/tun.c b/drivers/net/tun.c
index 242899f7fd07..e493c4554d5d 100644
--- a/drivers/net/tun.c
+++ b/drivers/net/tun.c
@@ -197,6 +197,7 @@ struct tun_struct {
 	int			sndbuf;
 	struct tap_filter	txflt;
 	struct sock_fprog	fprog;
+	struct sock_fprog_kern	fprog_kern;
 	/* protected by rtnl lock */
 	bool			filter_attached;
 	u32			msg_enable;
@@ -722,12 +723,47 @@ static void tun_force_wake_queue(struct tun_struct *tun,
 	spin_unlock_bh(&tfile->tx_ring.consumer_lock);
 }
 
+/* Copy the filter that @argp points at into the kernel, so that it can be
+ * installed again later, independent of the ioctl caller's address space.
+ * tun->fprog and tun->fprog_kern are updated only once the copy succeeded.
+ */
+static int tun_copy_filter(struct tun_struct *tun, struct sock_fprog __user *argp)
+{
+	struct sock_fprog fprog;
+	struct sock_filter *insns;
+
+	if (copy_from_user(&fprog, argp, sizeof(fprog)))
+		return -EFAULT;
+
+	if (!fprog.len || fprog.len > BPF_MAXINSNS)
+		return -EINVAL;
+
+	insns = kmalloc_array(fprog.len, sizeof(struct sock_filter),
+			      GFP_KERNEL_ACCOUNT);
+	if (!insns)
+		return -ENOMEM;
+
+	if (copy_from_user(insns, fprog.filter,
+			   fprog.len * sizeof(struct sock_filter))) {
+		kfree(insns);
+		return -EFAULT;
+	}
+
+	kfree(tun->fprog_kern.filter);
+	tun->fprog_kern.len = fprog.len;
+	tun->fprog_kern.filter = insns;
+	tun->fprog = fprog;
+
+	return 0;
+}
+
 static int tun_attach(struct tun_struct *tun, struct file *file,
 		      bool skip_filter, bool napi, bool napi_frags,
 		      bool publish_tun)
 {
 	struct tun_file *tfile = file->private_data;
 	struct net_device *dev = tun->dev;
+	bool rollback_filter = false;
 	int err;
 
 	err = security_tun_dev_attach(tfile->socket.sk, tun->security);
@@ -752,10 +788,11 @@ static int tun_attach(struct tun_struct *tun, struct file *file,
 	/* Re-attach the filter to persist device */
 	if (!skip_filter && (tun->filter_attached == true)) {
 		lock_sock(tfile->socket.sk);
-		err = sk_attach_filter(&tun->fprog, tfile->socket.sk);
+		err = sk_attach_filter_kern(&tun->fprog_kern, tfile->socket.sk);
 		release_sock(tfile->socket.sk);
-		if (!err)
+		if (err)
 			goto out;
+		rollback_filter = true;
 	}
 
 	if (!tfile->detached &&
@@ -817,6 +854,11 @@ static int tun_attach(struct tun_struct *tun, struct file *file,
 	WRITE_ONCE(tun->numqueues, tun->numqueues + 1);
 	tun_set_real_num_queues(tun);
 out:
+	if (err && rollback_filter) {
+		lock_sock(tfile->socket.sk);
+		sk_detach_filter(tfile->socket.sk);
+		release_sock(tfile->socket.sk);
+	}
 	return err;
 }
 
@@ -2400,6 +2442,7 @@ static void tun_free_netdev(struct net_device *dev)
 	security_tun_dev_free_security(tun->security);
 	__tun_set_ebpf(tun, &tun->steering_prog, NULL);
 	__tun_set_ebpf(tun, &tun->filter_prog, NULL);
+	kfree(tun->fprog_kern.filter);
 }
 
 static void tun_setup(struct net_device *dev)
@@ -3064,6 +3107,9 @@ static void tun_detach_filter(struct tun_struct *tun, int n)
 		release_sock(tfile->socket.sk);
 	}
 
+	kfree(tun->fprog_kern.filter);
+	tun->fprog_kern.filter = NULL;
+	tun->fprog_kern.len = 0;
 	tun->filter_attached = false;
 }
 
@@ -3075,7 +3121,7 @@ static int tun_attach_filter(struct tun_struct *tun)
 	for (i = 0; i < tun->numqueues; i++) {
 		tfile = rtnl_dereference(tun->tfiles[i]);
 		lock_sock(tfile->socket.sk);
-		ret = sk_attach_filter(&tun->fprog, tfile->socket.sk);
+		ret = sk_attach_filter_kern(&tun->fprog_kern, tfile->socket.sk);
 		release_sock(tfile->socket.sk);
 		if (ret) {
 			tun_detach_filter(tun, i);
@@ -3423,8 +3469,8 @@ static long __tun_chr_ioctl(struct file *file, unsigned int cmd,
 		ret = -EINVAL;
 		if ((tun->flags & TUN_TYPE_MASK) != IFF_TAP)
 			break;
-		ret = -EFAULT;
-		if (copy_from_user(&tun->fprog, argp, sizeof(tun->fprog)))
+		ret = tun_copy_filter(tun, argp);
+		if (ret)
 			break;
 
 		ret = tun_attach_filter(tun);
diff --git a/include/linux/filter.h b/include/linux/filter.h
index 39decde7fc73..0de5a738fb26 100644
--- a/include/linux/filter.h
+++ b/include/linux/filter.h
@@ -1218,6 +1218,7 @@ int bpf_prog_create_from_user(struct bpf_prog **pfp, struct sock_fprog *fprog,
 void bpf_prog_destroy(struct bpf_prog *fp);
 
 int sk_attach_filter(struct sock_fprog *fprog, struct sock *sk);
+int sk_attach_filter_kern(struct sock_fprog_kern *fprog, struct sock *sk);
 int sk_attach_bpf(u32 ufd, struct sock *sk);
 int sk_reuseport_attach_filter(struct sock_fprog *fprog, struct sock *sk);
 int sk_reuseport_attach_bpf(u32 ufd, struct sock *sk);
diff --git a/net/core/filter.c b/net/core/filter.c
index 70dc621672f2..57bef64b72bc 100644
--- a/net/core/filter.c
+++ b/net/core/filter.c
@@ -1567,6 +1567,47 @@ int sk_attach_filter(struct sock_fprog *fprog, struct sock *sk)
 }
 EXPORT_SYMBOL_GPL(sk_attach_filter);
 
+/**
+ * sk_attach_filter_kern - attach a classic BPF filter held in kernel memory
+ * @fprog: program to attach, with @fprog->filter pointing at kernel memory
+ * @sk: socket to attach the filter to
+ *
+ * Attach a copy of a classic BPF program that the kernel already holds, the
+ * way sk_attach_filter() does for one that still lives in user space. The
+ * caller must hold the socket lock; SOCK_FILTER_LOCKED is checked here.
+ *
+ * The program is built by bpf_prog_create(), which does not keep an original
+ * copy of the instructions. sk_get_filter() therefore returns -EACCES and
+ * sock_diag leaves the filter out for a socket that uses this helper. A
+ * classic BPF program cannot be duplicated with bpf_prog_copy(), so keeping
+ * the original would cost another allocation and a copy per attach.
+ *
+ * Return: 0 on success, -EPERM if @sk has SOCK_FILTER_LOCKED set, -EINVAL if
+ * the instructions are not a valid classic BPF program, and -ENOMEM on an
+ * allocation failure.
+ */
+int sk_attach_filter_kern(struct sock_fprog_kern *fprog, struct sock *sk)
+{
+	struct bpf_prog *prog;
+	int err;
+
+	if (sock_flag(sk, SOCK_FILTER_LOCKED))
+		return -EPERM;
+
+	err = bpf_prog_create(&prog, fprog);
+	if (err)
+		return err;
+
+	err = __sk_attach_prog(prog, sk);
+	if (err < 0) {
+		__bpf_prog_release(prog);
+		return err;
+	}
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(sk_attach_filter_kern);
+
 int sk_reuseport_attach_filter(struct sock_fprog *fprog, struct sock *sk)
 {
 	struct bpf_prog *prog = __get_filter(fprog, sk);
-- 
2.25.1


No virus found
		Checked by Hillstone Network AntiVirus

Re: [PATCH net v6 1/2] tun: keep a kernel copy of the socket filter program

[netdev-bot+sashiko]

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

Critical: 0 · High: 0 · Medium: 1 (2) · 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] TUNATTACHFILTER can save a classic BPF program that does not
  pass validation, and every later queue attach then fails.
- [Low] The new kernel-doc for sk_attach_filter_kern()
  (net/core/filter.c:1582) justifies not keeping orig_prog with "A…
- [Low] The kernel-doc 'Return:' section of sk_attach_filter_kern()
  (net/core/filter.c:1585-1588) lists 0, -EPERM, -EINVAL and -ENOMEM as…

Pre-existing issues:
- [Medium] This problem wasn't introduced by this patch, but a
  TUNATTACHFILTER that fails during the per-queue attach leaves the…
- [Medium] This problem wasn't introduced by this patch, but
  TUNDETACHFILTER calls tun_detach_filter(tun, tun->numqueues).

--

tun: keep a kernel copy of the socket filter program

This patch keeps a kernel copy, in tun->fprog_kern, of the classic BPF
program passed to TUNATTACHFILTER. It adds sk_attach_filter_kern(), which
builds a per-queue program from that copy. It also fixes the inverted error
check in tun_attach() that runs when the filter is re-attached on a
persistent device.

Link: https://lore.kernel.org/netdev/179027275318.2160803.4185895144088175048@kernel.org/

> diff --git a/drivers/net/tun.c b/drivers/net/tun.c
> index 242899f7fd071..e493c4554d5df 100644
> --- a/drivers/net/tun.c
> +++ b/drivers/net/tun.c

[ ... ]

> @@ -722,12 +723,47 @@ static void tun_force_wake_queue(struct tun_struct *tun,
>  	spin_unlock_bh(&tfile->tx_ring.consumer_lock);
>  }
>  
> +/* Copy the filter that @argp points at into the kernel, so that it can be
> + * installed again later, independent of the ioctl caller's address space.
> + * tun->fprog and tun->fprog_kern are updated only once the copy succeeded.
> + */
> +static int tun_copy_filter(struct tun_struct *tun, struct sock_fprog __user *argp)
> +{
> +	struct sock_fprog fprog;
> +	struct sock_filter *insns;
> +
> +	if (copy_from_user(&fprog, argp, sizeof(fprog)))
> +		return -EFAULT;
> +
> +	if (!fprog.len || fprog.len > BPF_MAXINSNS)
> +		return -EINVAL;

[ ... ]

> +	kfree(tun->fprog_kern.filter);
> +	tun->fprog_kern.len = fprog.len;
> +	tun->fprog_kern.filter = insns;
> +	tun->fprog = fprog;

[Severity: Medium]
Only the header is checked here. The instructions go into tun->fprog_kern
without being validated. Can TUNATTACHFILTER store a program that
bpf_check_classic() would reject, and still return 0?

The instructions are only checked in the per-queue loop in
tun_attach_filter():

	for (i = 0; i < tun->numqueues; i++) {
		...
		ret = sk_attach_filter_kern(&tun->fprog_kern, tfile->socket.sk);
		...
	}

	tun->filter_attached = true;

If numqueues is 0, the loop never runs, and filter_attached is set to true
for the invalid program.

numqueues == 0 seems reachable on a multiqueue TAP device. Disable every
queue with TUNSETQUEUE/IFF_DETACH_QUEUE. __tun_detach(tfile, false) lowers
numqueues but leaves tfile->tun set, so TUNATTACHFILTER can still be issued
from the fd of a disabled queue.

With the inverted check now fixed, tun_attach() then does:

		err = sk_attach_filter_kern(&tun->fprog_kern, tfile->socket.sk);
		release_sock(tfile->socket.sk);
		if (err)
			goto out;

Wouldn't every IFF_ATTACH_QUEUE then fail with -EINVAL? tun_set_queue()
always passes skip_filter=false. The same applies to every TUNSETIFF on the
persistent device without IFF_NOFILTER. Before this patch, a TUNSETIFF
attach of a new tfile in this state succeeded, just without a filter.

The commit message says:

  Neither change works on its own: with only the kernel copy, every re-attach
  returns 0 without publishing the queue; with only the check fixed, a
  re-attach that used to succeed without installing a filter would start to
  fail.

The early-failure note in the commit message only covers bad headers. Since
the program is now a fixed kernel copy, could it be validated once in
tun_copy_filter() before the old copy is replaced? For example, call
bpf_prog_create() and then bpf_prog_destroy().

[ ... ]

> @@ -3064,6 +3107,9 @@ static void tun_detach_filter(struct tun_struct *tun, int n)
>  		release_sock(tfile->socket.sk);
>  	}
>  
> +	kfree(tun->fprog_kern.filter);
> +	tun->fprog_kern.filter = NULL;
> +	tun->fprog_kern.len = 0;
>  	tun->filter_attached = false;
>  }

[Severity: Medium]
This isn't a bug introduced by this patch, but TUNDETACHFILTER calls
tun_detach_filter(tun, tun->numqueues). That only walks the active
tun->tfiles[] entries, not the queues on tun->disabled.

So a queue disabled with IFF_DETACH_QUEUE (__tun_detach(tfile, false))
keeps its socket filter. Once filter_attached is cleared, re-enabling that
queue with IFF_ATTACH_QUEUE goes through tun_attach(), which skips the
filter block:

	if (!skip_filter && (tun->filter_attached == true)) {

Nothing removes the existing sk_filter on that path. Does tun_net_xmit()
then keep applying the old filter even though TUNDETACHFILTER succeeded? If
the old program was a drop-all filter, it would keep dropping packets.

>  
> @@ -3075,7 +3121,7 @@ static int tun_attach_filter(struct tun_struct *tun)
>  	for (i = 0; i < tun->numqueues; i++) {
>  		tfile = rtnl_dereference(tun->tfiles[i]);
>  		lock_sock(tfile->socket.sk);
> -		ret = sk_attach_filter(&tun->fprog, tfile->socket.sk);
> +		ret = sk_attach_filter_kern(&tun->fprog_kern, tfile->socket.sk);
>  		release_sock(tfile->socket.sk);
>  		if (ret) {
>  			tun_detach_filter(tun, i);

[Severity: Medium]
This is a pre-existing issue, but a TUNATTACHFILTER that fails here leaves
the device in an inconsistent state.

By this point tun_copy_filter() has already freed the old fprog_kern and
overwritten tun->fprog and tun->fprog_kern. Suppose sk_attach_filter_kern()
fails at queue i. Invalid instructions always fail at i == 0, and -ENOMEM
can happen at any i. tun_detach_filter(tun, i) then detaches only queues
0..i-1 and clears fprog_kern and filter_attached.

Do queues i..numqueues-1 keep the previous socket filter? tun_net_xmit()
would keep running it:

	if (tfile->socket.sk->sk_filter) {
		drop_reason = sk_filter_reason(tfile->socket.sk, skb);

Queues attached afterwards would get no filter. TUNGETFILTER would return
the header of the program that failed.

The patch only moves the length check earlier, so this can still happen
with bad instructions or allocation failures. Validating the instructions
in tun_copy_filter(), as asked above, would remove the bad-instruction
case.

[ ... ]

> diff --git a/net/core/filter.c b/net/core/filter.c
> index 70dc621672f2e..57bef64b72bcc 100644
> --- a/net/core/filter.c
> +++ b/net/core/filter.c
> @@ -1567,6 +1567,47 @@ int sk_attach_filter(struct sock_fprog *fprog, struct sock *sk)
>  }
>  EXPORT_SYMBOL_GPL(sk_attach_filter);
>  
> +/**
> + * sk_attach_filter_kern - attach a classic BPF filter held in kernel memory
> + * @fprog: program to attach, with @fprog->filter pointing at kernel memory
> + * @sk: socket to attach the filter to
> + *
> + * Attach a copy of a classic BPF program that the kernel already holds, the
> + * way sk_attach_filter() does for one that still lives in user space. The
> + * caller must hold the socket lock; SOCK_FILTER_LOCKED is checked here.
> + *
> + * The program is built by bpf_prog_create(), which does not keep an original
> + * copy of the instructions. sk_get_filter() therefore returns -EACCES and
> + * sock_diag leaves the filter out for a socket that uses this helper. A
> + * classic BPF program cannot be duplicated with bpf_prog_copy(), so keeping

[Severity: Low]
This isn't a bug, but is bpf_prog_copy() the intended name here? No
function by that name seems to exist in the tree. This comment is the only
place it appears.

> + * the original would cost another allocation and a copy per attach.
> + *
> + * Return: 0 on success, -EPERM if @sk has SOCK_FILTER_LOCKED set, -EINVAL if
> + * the instructions are not a valid classic BPF program, and -ENOMEM on an
> + * allocation failure.
> + */

[Severity: Low]
Is this list of return values complete? Any error from bpf_prog_create() is
passed through unchanged, including errors from runtime selection:

sk_attach_filter_kern()
  bpf_prog_create()
    bpf_prepare_filter()
      bpf_migrate_filter()
        bpf_prog_select_runtime()

__bpf_prog_select_runtime() sets -ENOTSUPP when a JIT is required but no
JIT image was produced, for example with CONFIG_BPF_JIT_ALWAYS_ON:

		if (!fp->jited && jit_needed) {
			*err = -ENOTSUPP;
			return fp;

It also passes through whatever bpf_prog_lock_ro() returns. Userspace can
see these codes through TUNATTACHFILTER and the tun_attach() re-attach
paths.

[ ... ]

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

Re: [PATCH net v6 1/2] tun: keep a kernel copy of the socket filter program

[Rongguang Wei]

on 2026/10/20 15:38, netdev-bot+sashiko@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 3 potential
> issue(s) to consider.
> 
> Critical: 0 · High: 0 · Medium: 1 (2) · 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] TUNATTACHFILTER can save a classic BPF program that does not
>   pass validation, and every later queue attach then fails.
> - [Low] The new kernel-doc for sk_attach_filter_kern()
>   (net/core/filter.c:1582) justifies not keeping orig_prog with "A…
> - [Low] The kernel-doc 'Return:' section of sk_attach_filter_kern()
>   (net/core/filter.c:1585-1588) lists 0, -EPERM, -EINVAL and -ENOMEM as…
> 
> Pre-existing issues:
> - [Medium] This problem wasn't introduced by this patch, but a
>   TUNATTACHFILTER that fails during the per-queue attach leaves the…
> - [Medium] This problem wasn't introduced by this patch, but
>   TUNDETACHFILTER calls tun_detach_filter(tun, tun->numqueues).
> 
> --
> 
> tun: keep a kernel copy of the socket filter program
> 
> This patch keeps a kernel copy, in tun->fprog_kern, of the classic BPF
> program passed to TUNATTACHFILTER. It adds sk_attach_filter_kern(), which
> builds a per-queue program from that copy. It also fixes the inverted error
> check in tun_attach() that runs when the filter is re-attached on a
> persistent device.
> 
> Link: https://lore.kernel.org/netdev/179027275318.2160803.4185895144088175048@kernel.org/
> 
>> diff --git a/drivers/net/tun.c b/drivers/net/tun.c
>> index 242899f7fd071..e493c4554d5df 100644
>> --- a/drivers/net/tun.c
>> +++ b/drivers/net/tun.c
> 
> [ ... ]
> 
>> @@ -722,12 +723,47 @@ static void tun_force_wake_queue(struct tun_struct *tun,
>>  	spin_unlock_bh(&tfile->tx_ring.consumer_lock);
>>  }
>>  
>> +/* Copy the filter that @argp points at into the kernel, so that it can be
>> + * installed again later, independent of the ioctl caller's address space.
>> + * tun->fprog and tun->fprog_kern are updated only once the copy succeeded.
>> + */
>> +static int tun_copy_filter(struct tun_struct *tun, struct sock_fprog __user *argp)
>> +{
>> +	struct sock_fprog fprog;
>> +	struct sock_filter *insns;
>> +
>> +	if (copy_from_user(&fprog, argp, sizeof(fprog)))
>> +		return -EFAULT;
>> +
>> +	if (!fprog.len || fprog.len > BPF_MAXINSNS)
>> +		return -EINVAL;
> 
> [ ... ]
> 
>> +	kfree(tun->fprog_kern.filter);
>> +	tun->fprog_kern.len = fprog.len;
>> +	tun->fprog_kern.filter = insns;
>> +	tun->fprog = fprog;
> 
> [Severity: Medium]
> Only the header is checked here. The instructions go into tun->fprog_kern
> without being validated. Can TUNATTACHFILTER store a program that
> bpf_check_classic() would reject, and still return 0?
> 
> The instructions are only checked in the per-queue loop in
> tun_attach_filter():
> 
> 	for (i = 0; i < tun->numqueues; i++) {
> 		...
> 		ret = sk_attach_filter_kern(&tun->fprog_kern, tfile->socket.sk);
> 		...
> 	}
> 
> 	tun->filter_attached = true;
> 
> If numqueues is 0, the loop never runs, and filter_attached is set to true
> for the invalid program.
> 
> numqueues == 0 seems reachable on a multiqueue TAP device. Disable every
> queue with TUNSETQUEUE/IFF_DETACH_QUEUE. __tun_detach(tfile, false) lowers
> numqueues but leaves tfile->tun set, so TUNATTACHFILTER can still be issued
> from the fd of a disabled queue.
> 
> With the inverted check now fixed, tun_attach() then does:
> 
> 		err = sk_attach_filter_kern(&tun->fprog_kern, tfile->socket.sk);
> 		release_sock(tfile->socket.sk);
> 		if (err)
> 			goto out;
> 
> Wouldn't every IFF_ATTACH_QUEUE then fail with -EINVAL? tun_set_queue()
> always passes skip_filter=false. The same applies to every TUNSETIFF on the
> persistent device without IFF_NOFILTER. Before this patch, a TUNSETIFF
> attach of a new tfile in this state succeeded, just without a filter.
> 
> The commit message says:
> 
>   Neither change works on its own: with only the kernel copy, every re-attach
>   returns 0 without publishing the queue; with only the check fixed, a
>   re-attach that used to succeed without installing a filter would start to
>   fail.
> 
> The early-failure note in the commit message only covers bad headers. Since
> the program is now a fixed kernel copy, could it be validated once in
> tun_copy_filter() before the old copy is replaced? For example, call
> bpf_prog_create() and then bpf_prog_destroy().
> 
OK. I folded the fix into v7. Add a new bpf_prog_validate_classic(), 
which runs the checks of bpf_check_classic() on the copy without allocating 
a struct bpf_prog.
> [ ... ]
> 
>> @@ -3064,6 +3107,9 @@ static void tun_detach_filter(struct tun_struct *tun, int n)
>>  		release_sock(tfile->socket.sk);
>>  	}
>>  
>> +	kfree(tun->fprog_kern.filter);
>> +	tun->fprog_kern.filter = NULL;
>> +	tun->fprog_kern.len = 0;
>>  	tun->filter_attached = false;
>>  }
> 
> [Severity: Medium]
> This isn't a bug introduced by this patch, but TUNDETACHFILTER calls
> tun_detach_filter(tun, tun->numqueues). That only walks the active
> tun->tfiles[] entries, not the queues on tun->disabled.
> 
> So a queue disabled with IFF_DETACH_QUEUE (__tun_detach(tfile, false))
> keeps its socket filter. Once filter_attached is cleared, re-enabling that
> queue with IFF_ATTACH_QUEUE goes through tun_attach(), which skips the
> filter block:
> 
> 	if (!skip_filter && (tun->filter_attached == true)) {
> 
> Nothing removes the existing sk_filter on that path. Does tun_net_xmit()
> then keep applying the old filter even though TUNDETACHFILTER succeeded? If
> the old program was a drop-all filter, it would keep dropping packets.
> 
>>  
>> @@ -3075,7 +3121,7 @@ static int tun_attach_filter(struct tun_struct *tun)
>>  	for (i = 0; i < tun->numqueues; i++) {
>>  		tfile = rtnl_dereference(tun->tfiles[i]);
>>  		lock_sock(tfile->socket.sk);
>> -		ret = sk_attach_filter(&tun->fprog, tfile->socket.sk);
>> +		ret = sk_attach_filter_kern(&tun->fprog_kern, tfile->socket.sk);
>>  		release_sock(tfile->socket.sk);
>>  		if (ret) {
>>  			tun_detach_filter(tun, i);
> 
> [Severity: Medium]
> This is a pre-existing issue, but a TUNATTACHFILTER that fails here leaves
> the device in an inconsistent state.
> 
> By this point tun_copy_filter() has already freed the old fprog_kern and
> overwritten tun->fprog and tun->fprog_kern. Suppose sk_attach_filter_kern()
> fails at queue i. Invalid instructions always fail at i == 0, and -ENOMEM
> can happen at any i. tun_detach_filter(tun, i) then detaches only queues
> 0..i-1 and clears fprog_kern and filter_attached.
> 
> Do queues i..numqueues-1 keep the previous socket filter? tun_net_xmit()
> would keep running it:
> 
> 	if (tfile->socket.sk->sk_filter) {
> 		drop_reason = sk_filter_reason(tfile->socket.sk, skb);
> 
> Queues attached afterwards would get no filter. TUNGETFILTER would return
> the header of the program that failed.
> 
> The patch only moves the length check earlier, so this can still happen
> with bad instructions or allocation failures. Validating the instructions
> in tun_copy_filter(), as asked above, would remove the bad-instruction
> case.
> 
> [ ... ]
> 
>> diff --git a/net/core/filter.c b/net/core/filter.c
>> index 70dc621672f2e..57bef64b72bcc 100644
>> --- a/net/core/filter.c
>> +++ b/net/core/filter.c
>> @@ -1567,6 +1567,47 @@ int sk_attach_filter(struct sock_fprog *fprog, struct sock *sk)
>>  }
>>  EXPORT_SYMBOL_GPL(sk_attach_filter);
>>  
>> +/**
>> + * sk_attach_filter_kern - attach a classic BPF filter held in kernel memory
>> + * @fprog: program to attach, with @fprog->filter pointing at kernel memory
>> + * @sk: socket to attach the filter to
>> + *
>> + * Attach a copy of a classic BPF program that the kernel already holds, the
>> + * way sk_attach_filter() does for one that still lives in user space. The
>> + * caller must hold the socket lock; SOCK_FILTER_LOCKED is checked here.
>> + *
>> + * The program is built by bpf_prog_create(), which does not keep an original
>> + * copy of the instructions. sk_get_filter() therefore returns -EACCES and
>> + * sock_diag leaves the filter out for a socket that uses this helper. A
>> + * classic BPF program cannot be duplicated with bpf_prog_copy(), so keeping
> 
> [Severity: Low]
> This isn't a bug, but is bpf_prog_copy() the intended name here? No
> function by that name seems to exist in the tree. This comment is the only
> place it appears.
> 
>> + * the original would cost another allocation and a copy per attach.
>> + *
>> + * Return: 0 on success, -EPERM if @sk has SOCK_FILTER_LOCKED set, -EINVAL if
>> + * the instructions are not a valid classic BPF program, and -ENOMEM on an
>> + * allocation failure.
>> + */
> 
> [Severity: Low]
> Is this list of return values complete? Any error from bpf_prog_create() is
> passed through unchanged, including errors from runtime selection:
> 
> sk_attach_filter_kern()
>   bpf_prog_create()
>     bpf_prepare_filter()
>       bpf_migrate_filter()
>         bpf_prog_select_runtime()
> 
> __bpf_prog_select_runtime() sets -ENOTSUPP when a JIT is required but no
> JIT image was produced, for example with CONFIG_BPF_JIT_ALWAYS_ON:
> 
> 		if (!fp->jited && jit_needed) {
> 			*err = -ENOTSUPP;
> 			return fp;
> 
> It also passes through whatever bpf_prog_lock_ro() returns. Userspace can
> see these codes through TUNATTACHFILTER and the tun_attach() re-attach
> paths.
> 
> [ ... ]
> 
pw-bot: cr

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

[Rongguang Wei]

From: Rongguang Wei <weirongguang@kylinos.cn>

reattach_filter_without_user_buffer() attaches a filter, makes the buffer
the program was copied from unreadable, detaches the queue and attaches it
again. It then attaches a second queue from a fresh socket, which starts
without a filter of its own, so a clear IFF_NOFILTER shows that the filter
was installed again from the kernel copy.

attach_filter_bad_len_keeps_program() checks that a rejected
TUNATTACHFILTER leaves the installed program and the filter state that
TUNGETIFF reports alone, and that a queue attached afterwards still gets
the filter.

attach_filter_nofilter_flag() and detach_filter_clears_reattach() cover the
ways to attach a queue without a filter.

Signed-off-by: Rongguang Wei <weirongguang@kylinos.cn>
Reviewed-by: Willem de Bruijn <willemb@google.com>
---
 tools/testing/selftests/net/tun.c | 197 ++++++++++++++++++++++++++++++
 1 file changed, 197 insertions(+)

diff --git a/tools/testing/selftests/net/tun.c b/tools/testing/selftests/net/tun.c
index afaa81c9bac5..a4993a631957 100644
--- a/tools/testing/selftests/net/tun.c
+++ b/tools/testing/selftests/net/tun.c
@@ -8,9 +8,11 @@
 #include <stdlib.h>
 #include <string.h>
 #include <unistd.h>
+#include <linux/filter.h>
 #include <linux/if_tun.h>
 #include <netinet/tcp.h>
 #include <sys/ioctl.h>
+#include <sys/mman.h>
 #include <sys/socket.h>
 
 #include "kselftest_harness.h"
@@ -543,6 +545,201 @@ TEST_F(tun, reattach_close_delete)
 	EXPECT_EQ(tun_delete(self->ifname), 0);
 }
 
+/* accept: return skb->len */
+static const struct sock_filter filter_accept[] = {
+	BPF_STMT(BPF_LD | BPF_W | BPF_LEN, 0),
+	BPF_STMT(BPF_ALU | BPF_ADD | BPF_K, 0),
+	BPF_STMT(BPF_RET | BPF_A, 0),
+};
+
+/* Put the instructions in an anonymous mapping, so that the test can make the
+ * address unreadable afterwards.
+ */
+static void *filter_alloc(const struct sock_filter *insns, unsigned int len)
+{
+	void *p;
+
+	p = mmap(NULL, getpagesize(), PROT_READ | PROT_WRITE,
+		 MAP_PRIVATE | MAP_ANONYMOUS, -1, 0);
+	if (p == MAP_FAILED)
+		return NULL;
+
+	memcpy(p, insns, len * sizeof(*insns));
+
+	return p;
+}
+
+static int filter_attach(int fd, const struct sock_filter *insns, unsigned int len)
+{
+	struct sock_fprog fp = {
+		.len = len,
+		.filter = (struct sock_filter *)insns,
+	};
+
+	return ioctl(fd, TUNATTACHFILTER, (void *)&fp);
+}
+
+static int filter_get(int fd, struct sock_fprog *fp)
+{
+	return ioctl(fd, TUNGETFILTER, (void *)fp);
+}
+
+static int tun_get_iff_flags(int fd)
+{
+	struct ifreq ifr = { 0 };
+
+	if (ioctl(fd, TUNGETIFF, (void *)&ifr) < 0)
+		return -1;
+
+	return ifr.ifr_flags;
+}
+
+/*
+ * A queue can be attached long after the filter was configured, from a
+ * process that does not necessarily map the buffer the program was copied
+ * from, so the kernel has to keep its own copy of the program.  Here the
+ * mapping is made unreadable before the queue is attached again, and a
+ * second queue is then attached from a fresh socket, which starts without a
+ * filter of its own.
+ */
+TEST_F(tun, reattach_filter_without_user_buffer)
+{
+	struct sock_fprog gf = { 0 };
+	struct ifreq ifr = { 0 };
+	short flags = 0;
+	void *prog;
+	int fd, ret;
+
+	prog = filter_alloc(filter_accept, ARRAY_SIZE(filter_accept));
+	ASSERT_NE(prog, NULL);
+	ASSERT_EQ(filter_attach(self->fd, prog, ARRAY_SIZE(filter_accept)), 0);
+
+	EXPECT_EQ(tun_detach(self->fd, self->ifname), 0);
+
+	/* This memory is no longer readable */
+	ASSERT_EQ(mprotect(prog, getpagesize(), PROT_NONE), 0);
+
+	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.
+	 */
+	EXPECT_EQ(filter_get(self->fd, &gf), 0);
+	EXPECT_EQ(gf.len, ARRAY_SIZE(filter_accept));
+
+	/* A new queue starts without a filter, so a clear IFF_NOFILTER shows
+	 * that the filter was installed again from the kernel copy.
+	 */
+	fd = open("/dev/net/tun", O_RDWR);
+	ASSERT_GE(fd, 0);
+
+	strcpy(ifr.ifr_name, self->ifname);
+	ifr.ifr_flags = IFF_TAP | IFF_MULTI_QUEUE;
+	EXPECT_GE(ioctl(fd, TUNSETIFF, (void *)&ifr), 0);
+
+	flags = tun_get_iff_flags(fd);
+	EXPECT_GE(flags, 0);
+	EXPECT_EQ(flags & IFF_NOFILTER, 0);
+
+	close(fd);
+
+	ASSERT_EQ(mprotect(prog, getpagesize(), PROT_READ | PROT_WRITE), 0);
+	ASSERT_EQ(munmap(prog, getpagesize()), 0);
+}
+
+/* A new queue attached with IFF_NOFILTER must not get the filter that is
+ * configured on the device.
+ */
+TEST_F(tun, attach_filter_nofilter_flag)
+{
+	struct ifreq ifr = { 0 };
+	short flags = 0;
+	int fd;
+
+	ASSERT_EQ(filter_attach(self->fd, filter_accept, ARRAY_SIZE(filter_accept)), 0);
+
+	fd = open("/dev/net/tun", O_RDWR);
+	ASSERT_GE(fd, 0);
+
+	strcpy(ifr.ifr_name, self->ifname);
+	ifr.ifr_flags = IFF_TAP | IFF_MULTI_QUEUE | IFF_NOFILTER;
+	EXPECT_GE(ioctl(fd, TUNSETIFF, (void *)&ifr), 0);
+
+	flags = tun_get_iff_flags(fd);
+	EXPECT_GE(flags, 0);
+	EXPECT_EQ(flags & IFF_NOFILTER, IFF_NOFILTER);
+
+	close(fd);
+}
+
+/* After TUNDETACHFILTER a later attach must not install the filter again */
+TEST_F(tun, detach_filter_clears_reattach)
+{
+	short flags = 0;
+
+	ASSERT_EQ(filter_attach(self->fd, filter_accept, ARRAY_SIZE(filter_accept)), 0);
+	EXPECT_EQ(ioctl(self->fd, TUNDETACHFILTER, 0), 0);
+
+	flags = tun_get_iff_flags(self->fd);
+	EXPECT_GE(flags, 0);
+	EXPECT_EQ(flags & IFF_NOFILTER, IFF_NOFILTER);
+
+	EXPECT_EQ(tun_detach(self->fd, self->ifname), 0);
+	EXPECT_EQ(tun_attach(self->fd, self->ifname), 0);
+
+	flags = tun_get_iff_flags(self->fd);
+	EXPECT_GE(flags, 0);
+	EXPECT_EQ(flags & IFF_NOFILTER, IFF_NOFILTER);
+}
+
+/* A TUNATTACHFILTER with a bad length must leave the installed program and the
+ * filter state that TUNGETIFF reports alone.
+ */
+TEST_F(tun, attach_filter_bad_len_keeps_program)
+{
+	struct sock_fprog gf = { 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, filter_accept, 0), -1);
+	EXPECT_EQ(errno, EINVAL);
+
+	EXPECT_EQ(filter_get(self->fd, &gf), 0);
+	EXPECT_EQ(gf.len, ARRAY_SIZE(filter_accept));
+
+	flags = tun_get_iff_flags(self->fd);
+	EXPECT_GE(flags, 0);
+	EXPECT_EQ(flags & IFF_NOFILTER, 0);
+
+	/* A queue attached afterwards still gets the filter, so the rejected
+	 * ioctl left the saved program and filter_attached alone.
+	 */
+	fd = open("/dev/net/tun", O_RDWR);
+	ASSERT_GE(fd, 0);
+
+	strcpy(ifr.ifr_name, self->ifname);
+	ifr.ifr_flags = IFF_TAP | IFF_MULTI_QUEUE;
+	EXPECT_GE(ioctl(fd, TUNSETIFF, (void *)&ifr), 0);
+
+	flags = tun_get_iff_flags(fd);
+	EXPECT_GE(flags, 0);
+	EXPECT_EQ(flags & IFF_NOFILTER, 0);
+
+	close(fd);
+}
+
 FIXTURE(tun_vnet_gso)
 {
 	char ifname[IFNAMSIZ];
-- 
2.25.1


No virus found
		Checked by Hillstone Network AntiVirus

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

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found more severe
issue(s) in other patches of this series. This mail is only an FYI, in case
you want to address the nit picks below when you respin.

Low priority / nit picks for your consideration:

- [Low] The commit message says attach_filter_bad_len_keeps_program()
  checks that "a rejected TUNATTACHFILTER leaves the installed program…

--

selftests: net: add TAP socket filter attach tests

This adds four TAP socket filter tests to the tun selftest. The first
re-attaches a queue after the user buffer holding the program is made
unreadable. The others cover attaching with IFF_NOFILTER, re-attaching
after TUNDETACHFILTER, and a TUNATTACHFILTER rejected for a bad length.

> diff --git a/tools/testing/selftests/net/tun.c b/tools/testing/selftests/net/tun.c
> index afaa81c9bac53..a4993a6319572 100644
> --- a/tools/testing/selftests/net/tun.c
> +++ b/tools/testing/selftests/net/tun.c

[ ... ]

> +/* A TUNATTACHFILTER with a bad length must leave the installed program and the
> + * filter state that TUNGETIFF reports alone.
> + */
> +TEST_F(tun, attach_filter_bad_len_keeps_program)
> +{
> +	struct sock_fprog gf = { 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, filter_accept, 0), -1);
> +	EXPECT_EQ(errno, EINVAL);

[Severity: Low]

The commit message says:

  attach_filter_bad_len_keeps_program() checks that a rejected
  TUNATTACHFILTER leaves the installed program and the filter state that
  TUNGETIFF reports alone, and that a queue attached afterwards still gets
  the filter.

Does the test cover that much? The only rejection it exercises is
len == 0. tun_copy_filter() rejects that case before it changes any state:

drivers/net/tun.c:tun_copy_filter() {
    ...
	if (!fprog.len || fprog.len > BPF_MAXINSNS)
		return -EINVAL;
    ...
}

Now take a valid length with instructions that are not a valid classic BPF
program. In that case tun_copy_filter() replaces the saved program before
anything validates it:

	kfree(tun->fprog_kern.filter);
	tun->fprog_kern.len = fprog.len;
	tun->fprog_kern.filter = insns;
	tun->fprog = fprog;

sk_attach_filter_kern() then fails on queue 0, so tun_attach_filter()
calls tun_detach_filter(tun, 0). That call detaches no sockets, but it
does:

	kfree(tun->fprog_kern.filter);
	tun->fprog_kern.filter = NULL;
	tun->fprog_kern.len = 0;
	tun->filter_attached = false;

So after this kind of rejection:

  - the existing queues keep the old sk_filter, so TUNGETIFF still shows
    IFF_NOFILTER clear
  - TUNGETFILTER copies tun->fprog, so it returns the rejected program's
    length, not ARRAY_SIZE(filter_accept)
  - a queue attached later with TUNSETIFF gets no filter, because
    filter_attached is false

Two of the three properties in the commit message don't hold on this
path, and no test covers it.

A related path is also untested. Suppose TUNATTACHFILTER with an invalid
program is issued while numqueues == 0. The loop in tun_attach_filter()
never runs, so the program is never validated, and filter_attached is set
to true. Every later drivers/net/tun.c:tun_attach() then fails with
-EINVAL from sk_attach_filter_kern().

This kernel behaviour was already there before this series, and the
companion kernel patch only claims to fix the bad header case. The test
name and its in-code comment are scoped to a bad length.

Could the changelog say "a TUNATTACHFILTER with a bad length" instead of
"a rejected TUNATTACHFILTER"? Alternatively, a case with a valid length and
invalid instructions could be added.

> +
> +	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/20261016070654.170932-1-clementwei90%40163.com