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

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

[Rongguang Wei]

From: Rongguang Wei <weirongguang@kylinos.cn>

This is v8 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 to work around the re-attach bug. The
program is verified with bpf_prog_validate_classic() when it is copied in,
and the copy is committed only after every queue accepted it, so a program
that is refused later, when it is built, does not replace the saved one.

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 TUNATTACHFILTER with a bad length or bad instructions 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

---
v8:
  - narrow the bpf_prog_validate_classic() contract to the classic
    verifier and note that it can also return -ENOMEM; a program can still
    be refused when it is built, and the commit message now says so
  - commit the new program only after every queue took it, so a late
    failure does not replace the saved one, and filter_attached is left
    alone
  - selftests: check the filter state and a newly attached queue in
    attach_filter_bad_prog_keeps_program() too
v7:
  - verify the instructions in tun_copy_filter() before anything is
    replaced, so an invalid program is rejected even when no queue is
    attached
  - fix the kernel-doc of sk_attach_filter_kern(): drop a reference to a
    function that does not exist and complete the Return section
  - reject a program whose length is out of range before allocating
    memory for it
  - document why the filter is dropped when a later step of tun_attach()
    fails, and why filter_accept[] has three instructions
  - selftests: add a case for a rejected program with valid length and
    bad instructions
v6:
  - add description for return value of sk_attach_filter_kern(); 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                 | 125 ++++++++++++++--
 include/linux/filter.h            |   2 +
 net/core/filter.c                 |  67 +++++++++
 tools/testing/selftests/net/tun.c | 249 ++++++++++++++++++++++++++++++
 4 files changed, 431 insertions(+), 12 deletions(-)


base-commit: d135ab453a7400a34930bdda1ef4bdc8acfac80e
-- 
2.43.0


No virus found
		Checked by Hillstone Network AntiVirus

[PATCH net v8 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 failure of the attach was not fatal: -EFAULT for an unmapped
address, -EINVAL for bytes that are not a valid classic BPF program, -EPERM
for a locked filter and -ENOMEM all ended up in "err = 0" for a new tfile,
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, which made tun_attach() return early when
sk_attach_filter_kern() succeeded instead of when it failed. 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.

The header and the instructions are checked in tun_copy_filter() before
anything is replaced: a TUNATTACHFILTER that the classic verifier rejects
now fails early and leaves the previously attached filter in place. The
instructions are verified with the new bpf_prog_validate_classic(), which
runs bpf_check_basics_ok() and bpf_check_classic() on the copied program.
That does not cover the s16 check in bpf_convert_filter() or the runtime
selection in bpf_prog_select_runtime(), so a program can still be refused
when it is built for a queue. Such a late failure does not replace the
saved program: the copy is committed to tun->fprog and tun->fprog_kern
only after tun_attach_filter() installed it on every queue. A queue that
already took the copy is rolled back, which leaves it without a filter,
because the program it ran before was already released; restoring it
would need an extra reference per queue.

The inverted check was discovered by manual code inspection first.

Fixes: 54f968d6efdb ("tuntap: move socket to tun_file")
Link: https://lore.kernel.org/netdev/179309578401.3134.11519975030981154705@kernel.org/
Signed-off-by: Rongguang Wei <weirongguang@kylinos.cn>
---
 drivers/net/tun.c      | 125 +++++++++++++++++++++++++++++++++++++----
 include/linux/filter.h |   2 +
 net/core/filter.c      |  67 ++++++++++++++++++++++
 3 files changed, 182 insertions(+), 12 deletions(-)

diff --git a/drivers/net/tun.c b/drivers/net/tun.c
index 242899f7fd071..13ce62aa0f142 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,67 @@ 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.
+ * @fprog_kern receives the copy, which belongs to the caller until it is
+ * installed or freed, and @fprog receives the header, with the user pointer
+ * kept for TUNGETFILTER.
+ */
+static int tun_copy_filter(struct sock_fprog __user *argp,
+			   struct sock_fprog *fprog,
+			   struct sock_fprog_kern *fprog_kern)
+{
+	struct sock_fprog fp;
+	struct sock_filter *insns;
+	int err;
+
+	if (copy_from_user(&fp, argp, sizeof(fp)))
+		return -EFAULT;
+
+	if (!fp.len || fp.len > BPF_MAXINSNS)
+		return -EINVAL;
+
+	insns = kmalloc_array(fp.len, sizeof(struct sock_filter),
+			      GFP_KERNEL_ACCOUNT);
+	if (!insns)
+		return -ENOMEM;
+
+	if (copy_from_user(insns, fp.filter,
+			   fp.len * sizeof(struct sock_filter))) {
+		kfree(insns);
+		return -EFAULT;
+	}
+
+	/* The instructions are only verified when a program is built from
+	 * them, which happens when the filter is attached to a socket. Run
+	 * the classic verifier here as well, so that a bad program is
+	 * refused before it is saved and before any queue is touched.
+	 * bpf_prog_create() can still refuse the program later, when it is
+	 * converted to eBPF or when no runtime can be selected for it; the
+	 * attach path returns that error, and the saved program is not
+	 * replaced.
+	 */
+	err = bpf_prog_validate_classic(insns, fp.len);
+	if (err) {
+		kfree(insns);
+		return err;
+	}
+
+	fprog->len = fp.len;
+	fprog->filter = fp.filter;
+	fprog_kern->len = fp.len;
+	fprog_kern->filter = insns;
+
+	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 +808,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 +874,17 @@ 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) {
+		/* The filter was installed before the rest of the attach
+		 * ran, so drop it again if a later step failed. Any
+		 * program it replaced is already gone: this leaves the
+		 * queue without a filter rather than restoring the
+		 * previous one.
+		 */
+		lock_sock(tfile->socket.sk);
+		sk_detach_filter(tfile->socket.sk);
+		release_sock(tfile->socket.sk);
+	}
 	return err;
 }
 
@@ -2400,6 +2468,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,10 +3133,14 @@ 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;
 }
 
-static int tun_attach_filter(struct tun_struct *tun)
+static int tun_attach_filter(struct tun_struct *tun,
+			     struct sock_fprog_kern *fprog_kern)
 {
 	int i, ret = 0;
 	struct tun_file *tfile;
@@ -3075,15 +3148,29 @@ 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(fprog_kern, tfile->socket.sk);
 		release_sock(tfile->socket.sk);
-		if (ret) {
-			tun_detach_filter(tun, i);
-			return ret;
-		}
+		if (ret)
+			goto out_detach;
 	}
 
 	tun->filter_attached = true;
+	return 0;
+
+out_detach:
+	/* Drop the copies already installed on queues 0..i-1, which leaves
+	 * them without a filter: their previous sk_filter, if any, was
+	 * already replaced and released. The caller does not replace the
+	 * saved program when this fails, so tun->fprog, tun->fprog_kern and
+	 * filter_attached keep their previous values.
+	 */
+	while (i--) {
+		tfile = rtnl_dereference(tun->tfiles[i]);
+		lock_sock(tfile->socket.sk);
+		sk_detach_filter(tfile->socket.sk);
+		release_sock(tfile->socket.sk);
+	}
+
 	return ret;
 }
 
@@ -3418,17 +3505,31 @@ static long __tun_chr_ioctl(struct file *file, unsigned int cmd,
 		tun_set_sndbuf(tun);
 		break;
 
-	case TUNATTACHFILTER:
+	case TUNATTACHFILTER: {
+		struct sock_fprog_kern fprog_kern;
+		struct sock_fprog fprog;
+
 		/* Can be set only for TAPs */
 		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(argp, &fprog, &fprog_kern);
+		if (ret)
+			break;
+
+		ret = tun_attach_filter(tun, &fprog_kern);
+		if (ret) {
+			kfree(fprog_kern.filter);
 			break;
+		}
 
-		ret = tun_attach_filter(tun);
+		/* Install the copy only after every queue accepted it. */
+		kfree(tun->fprog_kern.filter);
+		tun->fprog_kern = fprog_kern;
+		tun->fprog = fprog;
 		break;
+	}
 
 	case TUNDETACHFILTER:
 		/* Can be set only for TAPs */
diff --git a/include/linux/filter.h b/include/linux/filter.h
index 39decde7fc730..e6f8506de3e2f 100644
--- a/include/linux/filter.h
+++ b/include/linux/filter.h
@@ -1216,8 +1216,10 @@ int bpf_prog_create(struct bpf_prog **pfp, struct sock_fprog_kern *fprog);
 int bpf_prog_create_from_user(struct bpf_prog **pfp, struct sock_fprog *fprog,
 			      bpf_aux_classic_check_t trans, bool save_orig);
 void bpf_prog_destroy(struct bpf_prog *fp);
+int bpf_prog_validate_classic(const struct sock_filter *insns, unsigned int len);
 
 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 70dc621672f2e..9e23804e379c6 100644
--- a/net/core/filter.c
+++ b/net/core/filter.c
@@ -1476,6 +1476,32 @@ void bpf_prog_destroy(struct bpf_prog *fp)
 }
 EXPORT_SYMBOL_GPL(bpf_prog_destroy);
 
+/**
+ * bpf_prog_validate_classic - run the classic BPF verifier on kernel memory
+ * @insns: instruction array
+ * @len: number of instructions in @insns
+ *
+ * Run bpf_check_basics_ok() and bpf_check_classic() on a program that is
+ * already in kernel memory, without building a struct bpf_prog.  These are
+ * the checks bpf_prog_create() runs before it converts the program, so
+ * callers can reject a bad program when it is configured instead of when it
+ * is first attached to a socket.
+ *
+ * A program that passes here can still be refused by bpf_prog_create(), when
+ * it is converted to eBPF or when no runtime can be selected for it.
+ *
+ * Return: 0 if the program passes the classic verifier, -EINVAL if it does
+ * not, or -ENOMEM if the verifier cannot allocate its state.
+ */
+int bpf_prog_validate_classic(const struct sock_filter *insns, unsigned int len)
+{
+	if (!bpf_check_basics_ok(insns, len))
+		return -EINVAL;
+
+	return bpf_check_classic(insns, len);
+}
+EXPORT_SYMBOL_GPL(bpf_prog_validate_classic);
+
 static int __sk_attach_prog(struct bpf_prog *prog, struct sock *sk)
 {
 	struct sk_filter *fp, *old_fp;
@@ -1567,6 +1593,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, so sk_get_filter() returns -EACCES and sock_diag
+ * omits the filter for a socket that uses this helper.
+ *
+ * Return: 0 on success, or a negative errno: -EPERM if @sk has
+ * SOCK_FILTER_LOCKED set, otherwise whatever bpf_prog_create() or
+ * __sk_attach_prog() returned (for example -EINVAL for an invalid classic
+ * BPF program, -ENOMEM, or -ENOTSUPP when a JIT is required but is not
+ * available).
+ */
+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.43.0


No virus found
		Checked by Hillstone Network AntiVirus

[PATCH net v8 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 TUNATTACHFILTER with a
bad length leaves the installed program and the filter state that
TUNGETIFF reports alone, and that a queue attached afterwards still gets
the filter.  attach_filter_bad_prog_keeps_program() refuses a program with
a valid length and instructions that bpf_check_classic() rejects, and
checks the same properties: the filter state on the queue, and a queue
attached afterwards still gets the saved program.

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>
---
 tools/testing/selftests/net/tun.c | 249 ++++++++++++++++++++++++++++++
 1 file changed, 249 insertions(+)

diff --git a/tools/testing/selftests/net/tun.c b/tools/testing/selftests/net/tun.c
index afaa81c9bac53..f27df2734dad1 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,253 @@ TEST_F(tun, reattach_close_delete)
 	EXPECT_EQ(tun_delete(self->ifname), 0);
 }
 
+/* accept: return skb->len.  Three instructions on purpose, so that the
+ * length read back through TUNGETFILTER tells it apart from a rejected
+ * program that is shorter.
+ */
+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);
+}
+
+/* A TUNATTACHFILTER with instructions that bpf_check_classic() rejects must
+ * be refused, and must leave the program installed already in place.
+ */
+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);
+
+	/* the program attached before is untouched */
+	EXPECT_EQ(filter_get(self->fd, &gf), 0);
+	EXPECT_EQ(gf.len, ARRAY_SIZE(filter_accept));
+
+	/* IFF_NOFILTER comes from the socket's sk_filter, not from the saved
+	 * header, so a clear flag shows the filter is still installed.
+	 */
+	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
+	 * program never replaced the saved one.
+	 */
+	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.43.0


No virus found
		Checked by Hillstone Network AntiVirus

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

[Willem de Bruijn]

Rongguang Wei wrote:
> 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 failure of the attach was not fatal: -EFAULT for an unmapped
> address, -EINVAL for bytes that are not a valid classic BPF program, -EPERM
> for a locked filter and -ENOMEM all ended up in "err = 0" for a new tfile,
> 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, which made tun_attach() return early when
> sk_attach_filter_kern() succeeded instead of when it failed. 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.
> 
> The header and the instructions are checked in tun_copy_filter() before
> anything is replaced: a TUNATTACHFILTER that the classic verifier rejects
> now fails early and leaves the previously attached filter in place. The
> instructions are verified with the new bpf_prog_validate_classic(), which
> runs bpf_check_basics_ok() and bpf_check_classic() on the copied program.
> That does not cover the s16 check in bpf_convert_filter() or the runtime
> selection in bpf_prog_select_runtime(), so a program can still be refused
> when it is built for a queue. Such a late failure does not replace the
> saved program: the copy is committed to tun->fprog and tun->fprog_kern
> only after tun_attach_filter() installed it on every queue. A queue that
> already took the copy is rolled back, which leaves it without a filter,
> because the program it ran before was already released; restoring it
> would need an extra reference per queue.
> 
> The inverted check was discovered by manual code inspection first.
> 
> Fixes: 54f968d6efdb ("tuntap: move socket to tun_file")
> Link: https://lore.kernel.org/netdev/179309578401.3134.11519975030981154705@kernel.org/
> Signed-off-by: Rongguang Wei <weirongguang@kylinos.cn>
> ---
>  drivers/net/tun.c      | 125 +++++++++++++++++++++++++++++++++++++----
>  include/linux/filter.h |   2 +
>  net/core/filter.c      |  67 ++++++++++++++++++++++
>  3 files changed, 182 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/net/tun.c b/drivers/net/tun.c
> index 242899f7fd071..13ce62aa0f142 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,67 @@ 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.
> + * @fprog_kern receives the copy, which belongs to the caller until it is
> + * installed or freed, and @fprog receives the header, with the user pointer
> + * kept for TUNGETFILTER.
> + */
> +static int tun_copy_filter(struct sock_fprog __user *argp,
> +			   struct sock_fprog *fprog,
> +			   struct sock_fprog_kern *fprog_kern)
> +{
> +	struct sock_fprog fp;
> +	struct sock_filter *insns;
> +	int err;
> +
> +	if (copy_from_user(&fp, argp, sizeof(fp)))
> +		return -EFAULT;
> +
> +	if (!fp.len || fp.len > BPF_MAXINSNS)
> +		return -EINVAL;
> +
> +	insns = kmalloc_array(fp.len, sizeof(struct sock_filter),
> +			      GFP_KERNEL_ACCOUNT);
> +	if (!insns)
> +		return -ENOMEM;
> +
> +	if (copy_from_user(insns, fp.filter,
> +			   fp.len * sizeof(struct sock_filter))) {
> +		kfree(insns);
> +		return -EFAULT;
> +	}
> +
> +	/* The instructions are only verified when a program is built from
> +	 * them, which happens when the filter is attached to a socket. Run
> +	 * the classic verifier here as well, so that a bad program is
> +	 * refused before it is saved and before any queue is touched.
> +	 * bpf_prog_create() can still refuse the program later, when it is
> +	 * converted to eBPF or when no runtime can be selected for it; the
> +	 * attach path returns that error, and the saved program is not
> +	 * replaced.
> +	 */
> +	err = bpf_prog_validate_classic(insns, fp.len);
> +	if (err) {
> +		kfree(insns);
> +		return err;
> +	}
> +
> +	fprog->len = fp.len;
> +	fprog->filter = fp.filter;
> +	fprog_kern->len = fp.len;
> +	fprog_kern->filter = insns;
> +
> +	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 +808,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 +874,17 @@ 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) {
> +		/* The filter was installed before the rest of the attach
> +		 * ran, so drop it again if a later step failed. Any
> +		 * program it replaced is already gone: this leaves the
> +		 * queue without a filter rather than restoring the
> +		 * previous one.
> +		 */
> +		lock_sock(tfile->socket.sk);
> +		sk_detach_filter(tfile->socket.sk);
> +		release_sock(tfile->socket.sk);
> +	}
>  	return err;
>  }
>  
> @@ -2400,6 +2468,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,10 +3133,14 @@ 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;
>  }
>  
> -static int tun_attach_filter(struct tun_struct *tun)
> +static int tun_attach_filter(struct tun_struct *tun,
> +			     struct sock_fprog_kern *fprog_kern)
>  {
>  	int i, ret = 0;
>  	struct tun_file *tfile;
> @@ -3075,15 +3148,29 @@ 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(fprog_kern, tfile->socket.sk);
>  		release_sock(tfile->socket.sk);
> -		if (ret) {
> -			tun_detach_filter(tun, i);
> -			return ret;
> -		}
> +		if (ret)
> +			goto out_detach;
>  	}
>  
>  	tun->filter_attached = true;
> +	return 0;
> +
> +out_detach:
> +	/* Drop the copies already installed on queues 0..i-1, which leaves
> +	 * them without a filter: their previous sk_filter, if any, was
> +	 * already replaced and released. The caller does not replace the
> +	 * saved program when this fails, so tun->fprog, tun->fprog_kern and
> +	 * filter_attached keep their previous values.
> +	 */
> +	while (i--) {
> +		tfile = rtnl_dereference(tun->tfiles[i]);
> +		lock_sock(tfile->socket.sk);
> +		sk_detach_filter(tfile->socket.sk);
> +		release_sock(tfile->socket.sk);
> +	}
> +
>  	return ret;
>  }
>  
> @@ -3418,17 +3505,31 @@ static long __tun_chr_ioctl(struct file *file, unsigned int cmd,
>  		tun_set_sndbuf(tun);
>  		break;
>  
> -	case TUNATTACHFILTER:
> +	case TUNATTACHFILTER: {
> +		struct sock_fprog_kern fprog_kern;
> +		struct sock_fprog fprog;
> +
>  		/* Can be set only for TAPs */
>  		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(argp, &fprog, &fprog_kern);
> +		if (ret)
> +			break;
> +
> +		ret = tun_attach_filter(tun, &fprog_kern);
> +		if (ret) {
> +			kfree(fprog_kern.filter);
>  			break;
> +		}
>  
> -		ret = tun_attach_filter(tun);
> +		/* Install the copy only after every queue accepted it. */
> +		kfree(tun->fprog_kern.filter);
> +		tun->fprog_kern = fprog_kern;
> +		tun->fprog = fprog;

    struct sock_fprog has padding between @len and @filter (6 bytes on
    64-bit). Because @fprog is uninitialized on the stack and
    tun_copy_filter() only writes its two fields, `tun->fprog = fprog` can
    copy uninitialized kernel stack bytes into tun->fprog, which
    TUNGETFILTER then leaks to userspace via copy_to_user(...,
    sizeof(tun->fprog)) when CONFIG_INIT_STACK_ALL_ZERO is not enabled
    (KMSAN kernel-infoleak).

    Zero-initialize @fprog (e.g. `struct sock_fprog fprog = {};` or
    memset() in tun_copy_filter()).

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

[Rongguang Wei]

Willem de Bruijn wrote:
> Rongguang Wei wrote:
>> 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 failure of the attach was not fatal: -EFAULT for an unmapped
>> address, -EINVAL for bytes that are not a valid classic BPF program, -EPERM
>> for a locked filter and -ENOMEM all ended up in "err = 0" for a new tfile,
>> 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, which made tun_attach() return early when
>> sk_attach_filter_kern() succeeded instead of when it failed. 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.
>>
>> The header and the instructions are checked in tun_copy_filter() before
>> anything is replaced: a TUNATTACHFILTER that the classic verifier rejects
>> now fails early and leaves the previously attached filter in place. The
>> instructions are verified with the new bpf_prog_validate_classic(), which
>> runs bpf_check_basics_ok() and bpf_check_classic() on the copied program.
>> That does not cover the s16 check in bpf_convert_filter() or the runtime
>> selection in bpf_prog_select_runtime(), so a program can still be refused
>> when it is built for a queue. Such a late failure does not replace the
>> saved program: the copy is committed to tun->fprog and tun->fprog_kern
>> only after tun_attach_filter() installed it on every queue. A queue that
>> already took the copy is rolled back, which leaves it without a filter,
>> because the program it ran before was already released; restoring it
>> would need an extra reference per queue.
>>
>> The inverted check was discovered by manual code inspection first.
>>
>> Fixes: 54f968d6efdb ("tuntap: move socket to tun_file")
>> Link: https://lore.kernel.org/netdev/179309578401.3134.11519975030981154705@kernel.org/
>> Signed-off-by: Rongguang Wei <weirongguang@kylinos.cn>
>> ---
>>  drivers/net/tun.c      | 125 +++++++++++++++++++++++++++++++++++++----
>>  include/linux/filter.h |   2 +
>>  net/core/filter.c      |  67 ++++++++++++++++++++++
>>  3 files changed, 182 insertions(+), 12 deletions(-)
>>
>> diff --git a/drivers/net/tun.c b/drivers/net/tun.c
>> index 242899f7fd071..13ce62aa0f142 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,67 @@ 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.
>> + * @fprog_kern receives the copy, which belongs to the caller until it is
>> + * installed or freed, and @fprog receives the header, with the user pointer
>> + * kept for TUNGETFILTER.
>> + */
>> +static int tun_copy_filter(struct sock_fprog __user *argp,
>> +			   struct sock_fprog *fprog,
>> +			   struct sock_fprog_kern *fprog_kern)
>> +{
>> +	struct sock_fprog fp;
>> +	struct sock_filter *insns;
>> +	int err;
>> +
>> +	if (copy_from_user(&fp, argp, sizeof(fp)))
>> +		return -EFAULT;
>> +
>> +	if (!fp.len || fp.len > BPF_MAXINSNS)
>> +		return -EINVAL;
>> +
>> +	insns = kmalloc_array(fp.len, sizeof(struct sock_filter),
>> +			      GFP_KERNEL_ACCOUNT);
>> +	if (!insns)
>> +		return -ENOMEM;
>> +
>> +	if (copy_from_user(insns, fp.filter,
>> +			   fp.len * sizeof(struct sock_filter))) {
>> +		kfree(insns);
>> +		return -EFAULT;
>> +	}
>> +
>> +	/* The instructions are only verified when a program is built from
>> +	 * them, which happens when the filter is attached to a socket. Run
>> +	 * the classic verifier here as well, so that a bad program is
>> +	 * refused before it is saved and before any queue is touched.
>> +	 * bpf_prog_create() can still refuse the program later, when it is
>> +	 * converted to eBPF or when no runtime can be selected for it; the
>> +	 * attach path returns that error, and the saved program is not
>> +	 * replaced.
>> +	 */
>> +	err = bpf_prog_validate_classic(insns, fp.len);
>> +	if (err) {
>> +		kfree(insns);
>> +		return err;
>> +	}
>> +
>> +	fprog->len = fp.len;
>> +	fprog->filter = fp.filter;
>> +	fprog_kern->len = fp.len;
>> +	fprog_kern->filter = insns;
>> +
>> +	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 +808,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 +874,17 @@ 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) {
>> +		/* The filter was installed before the rest of the attach
>> +		 * ran, so drop it again if a later step failed. Any
>> +		 * program it replaced is already gone: this leaves the
>> +		 * queue without a filter rather than restoring the
>> +		 * previous one.
>> +		 */
>> +		lock_sock(tfile->socket.sk);
>> +		sk_detach_filter(tfile->socket.sk);
>> +		release_sock(tfile->socket.sk);
>> +	}
>>  	return err;
>>  }
>>  
>> @@ -2400,6 +2468,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,10 +3133,14 @@ 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;
>>  }
>>  
>> -static int tun_attach_filter(struct tun_struct *tun)
>> +static int tun_attach_filter(struct tun_struct *tun,
>> +			     struct sock_fprog_kern *fprog_kern)
>>  {
>>  	int i, ret = 0;
>>  	struct tun_file *tfile;
>> @@ -3075,15 +3148,29 @@ 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(fprog_kern, tfile->socket.sk);
>>  		release_sock(tfile->socket.sk);
>> -		if (ret) {
>> -			tun_detach_filter(tun, i);
>> -			return ret;
>> -		}
>> +		if (ret)
>> +			goto out_detach;
>>  	}
>>  
>>  	tun->filter_attached = true;
>> +	return 0;
>> +
>> +out_detach:
>> +	/* Drop the copies already installed on queues 0..i-1, which leaves
>> +	 * them without a filter: their previous sk_filter, if any, was
>> +	 * already replaced and released. The caller does not replace the
>> +	 * saved program when this fails, so tun->fprog, tun->fprog_kern and
>> +	 * filter_attached keep their previous values.
>> +	 */
>> +	while (i--) {
>> +		tfile = rtnl_dereference(tun->tfiles[i]);
>> +		lock_sock(tfile->socket.sk);
>> +		sk_detach_filter(tfile->socket.sk);
>> +		release_sock(tfile->socket.sk);
>> +	}
>> +
>>  	return ret;
>>  }
>>  
>> @@ -3418,17 +3505,31 @@ static long __tun_chr_ioctl(struct file *file, unsigned int cmd,
>>  		tun_set_sndbuf(tun);
>>  		break;
>>  
>> -	case TUNATTACHFILTER:
>> +	case TUNATTACHFILTER: {
>> +		struct sock_fprog_kern fprog_kern;
>> +		struct sock_fprog fprog;
>> +
>>  		/* Can be set only for TAPs */
>>  		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(argp, &fprog, &fprog_kern);
>> +		if (ret)
>> +			break;
>> +
>> +		ret = tun_attach_filter(tun, &fprog_kern);
>> +		if (ret) {
>> +			kfree(fprog_kern.filter);
>>  			break;
>> +		}
>>  
>> -		ret = tun_attach_filter(tun);
>> +		/* Install the copy only after every queue accepted it. */
>> +		kfree(tun->fprog_kern.filter);
>> +		tun->fprog_kern = fprog_kern;
>> +		tun->fprog = fprog;
> 
>     struct sock_fprog has padding between @len and @filter (6 bytes on
>     64-bit). Because @fprog is uninitialized on the stack and
>     tun_copy_filter() only writes its two fields, `tun->fprog = fprog` can
>     copy uninitialized kernel stack bytes into tun->fprog, which
>     TUNGETFILTER then leaks to userspace via copy_to_user(...,
>     sizeof(tun->fprog)) when CONFIG_INIT_STACK_ALL_ZERO is not enabled
>     (KMSAN kernel-infoleak).
> 
>     Zero-initialize @fprog (e.g. `struct sock_fprog fprog = {};` or
>     memset() in tun_copy_filter()).
Thanks Willem.
In v9, the zero-initializes the caller's header in tun_copy_filter() before the
two fields are assigned:
	memset(fprog, 0, sizeof(*fprog));
so the padding that TUNGETFILTER copies is zero instead of uninitialized
stack. Everything else about the header is unchanged.

Re: [PATCH net v8 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: 3 (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] If the device has no active queues, TUNATTACHFILTER saves a
  program that has never been built.
- [Medium] If TUNATTACHFILTER fails part-way, the device's filter record
  no longer matches what its queues enforce.
- [Medium] TUNATTACHFILTER now fills tun->fprog from an uninitialized
  stack variable.

Pre-existing issues:
- [Medium] Disabled (detached) queues keep their own sk_filter when
  TUNDETACHFILTER runs.

--

tun: keep a kernel copy of the socket filter program

This commit makes TUNATTACHFILTER keep a kernel copy of the classic BPF
program in tun->fprog_kern. Each queue now gets its program from that copy
through a new sk_attach_filter_kern() helper, and the user pointer is no
longer read again. It also fixes the inverted error check in tun_attach()
and checks the program early with a new bpf_prog_validate_classic() helper.

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

> diff --git a/drivers/net/tun.c b/drivers/net/tun.c
> index 439565a264ee7..ea6743e8ae600 100644
> --- a/drivers/net/tun.c
> +++ b/drivers/net/tun.c

[ ... ]

> @@ -726,12 +727,67 @@ static void tun_force_wake_queue(struct tun_struct *tun,

[ ... ]

> +	err = bpf_prog_validate_classic(insns, fp.len);
> +	if (err) {
> +		kfree(insns);
> +		return err;
> +	}
> +
> +	fprog->len = fp.len;
> +	fprog->filter = fp.filter;

[Severity: Medium]
Can this leak kernel stack bytes through TUNGETFILTER?

In __tun_chr_ioctl(), the caller's struct sock_fprog is a stack variable
with no initializer:

	case TUNATTACHFILTER: {
		struct sock_fprog_kern fprog_kern;
		struct sock_fprog fprog;

Only len and filter are written here. The padding between the unsigned
short len and the filter pointer (6 bytes on 64-bit) is never set.

The ioctl then copies the whole struct into device state with
tun->fprog = fprog, and TUNGETFILTER copies all of it back to user space:

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

Before this patch, copy_from_user() filled every byte of tun->fprog.

Would using *fprog = fp; here, or initializing fprog with {} in the ioctl,
avoid this?

> +	fprog_kern->len = fp.len;
> +	fprog_kern->filter = insns;
> +
> +	return 0;
> +}

[ ... ]

> @@ -3070,10 +3139,14 @@ 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 what happens to queues that
are disabled when TUNDETACHFILTER runs?

tun_detach_filter() only walks the active queues in tun->tfiles[0..n-1].
The non-clean path of __tun_detach() also leaves the socket filter in
place:

	} else {
		tun_disable_queue(tun, tfile);
		tun_napi_disable(tfile);
	}

After TUNDETACHFILTER, filter_attached is false. When that queue comes back
through TUNSETQUEUE IFF_ATTACH_QUEUE, tun_attach() skips the filter block:

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

Will the re-attached queue keep filtering with a program that userspace has
already removed?

>  
> -static int tun_attach_filter(struct tun_struct *tun)
> +static int tun_attach_filter(struct tun_struct *tun,
> +			     struct sock_fprog_kern *fprog_kern)
>  {
>  	int i, ret = 0;
>  	struct tun_file *tfile;
> @@ -3081,15 +3154,29 @@ 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(fprog_kern, tfile->socket.sk);
>  		release_sock(tfile->socket.sk);
> -		if (ret) {
> -			tun_detach_filter(tun, i);
> -			return ret;
> -		}
> +		if (ret)
> +			goto out_detach;
>  	}
>  
>  	tun->filter_attached = true;
> +	return 0;

[Severity: Medium]
What happens here when tun->numqueues is 0?

The commit message says:

  Such a late failure does not replace the saved program: the copy is
  committed to tun->fprog and tun->fprog_kern only after
  tun_attach_filter() installed it on every queue.

Take a multiqueue TAP whose queues have all been detached with TUNSETQUEUE
IFF_DETACH_QUEUE. tun_set_queue()->__tun_detach(tfile, false) lowers
numqueues but keeps tfile->tun set, so the detached fd can still issue
TUNATTACHFILTER. Then:

  tun_copy_filter()
    bpf_prog_validate_classic()     <- classic checks only
  tun_attach_filter()
    for loop runs zero times
    tun->filter_attached = true; return 0;
  __tun_chr_ioctl()
    tun->fprog_kern = fprog_kern;
    tun->fprog = fprog;

The saved program has never been through bpf_prog_create().

The commit message notes that bpf_prog_create() can still refuse a program
that passes bpf_check_classic(). Examples are the s16 offset check in
bpf_convert_filter(), and bpf_prog_select_runtime() when a JIT is required.

tun_attach() now has the corrected check. After this, wouldn't every later
filtered attach fail with that error until TUNDETACHFILTER is issued? That
covers TUNSETQUEUE IFF_ATTACH_QUEUE and TUNSETIFF without IFF_NOFILTER.

Would a trial bpf_prog_create()/bpf_prog_destroy() in tun_copy_filter()
catch these failures before anything is committed? The new
bpf_prog_validate_classic() export would then not be needed. Its own
kernel-doc says it does not cover these cases.

> +
> +out_detach:
> +	/* Drop the copies already installed on queues 0..i-1, which leaves
> +	 * them without a filter: their previous sk_filter, if any, was
> +	 * already replaced and released. The caller does not replace the
> +	 * saved program when this fails, so tun->fprog, tun->fprog_kern and
> +	 * filter_attached keep their previous values.
> +	 */
> +	while (i--) {
> +		tfile = rtnl_dereference(tun->tfiles[i]);
> +		lock_sock(tfile->socket.sk);
> +		sk_detach_filter(tfile->socket.sk);
> +		release_sock(tfile->socket.sk);
> +	}

[Severity: Medium]
Does this leave the device filter state out of sync with what the queues
enforce?

__sk_attach_prog() releases each queue's old sk_filter as soon as it
installs the new one. Suppose sk_attach_filter_kern() fails on queue i > 0.
This loop then leaves queues 0..i-1 with no filter, while queues i..n-1
still run the old program.

tun->filter_attached, tun->fprog and tun->fprog_kern still describe the old
program. TUNGETFILTER reports it as attached and queues attached later get
it. Meanwhile, active queues 0..i-1 pass every packet unfiltered. Before
this patch, the failure path went through tun_detach_filter(tun, i), which
cleared filter_attached.

A failure at i > 0 looks reachable in two ways:

- -ENOMEM from bpf_prog_alloc() in bpf_prog_create(), or from
  kmalloc_obj() in __sk_attach_prog().
- __sk_filter_charge(). The new filter is charged before the old one is
  uncharged, so a queue that already holds a large filter needs optmem for
  both.

The commit message says restoring the previous program "would need an
extra reference per queue". When a filter was attached before, the old
instructions are still in tun->fprog_kern at this point. Could the rollback
rebuild them with sk_attach_filter_kern(&tun->fprog_kern, sk) instead of
calling sk_detach_filter()?

> +
> +	return ret;
>  }

[ ... ]

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

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

[netdev-bot+sashiko]

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