[PATCH net-next] net: skbuff: replace ubuf_info ops ptr with type id

[PATCH net-next] net: skbuff: replace ubuf_info ops ptr with type id

[Willem de Bruijn]

From: Willem de Bruijn <willemb@google.com>

Avoid an easily corruptible callback pointer on skb linear
out-of-bounds write bugs.

skb_shinfo(skb)->destructor_arg points to a struct ubuf_info, whose
first member is a ubuf_info_ops vtable pointer. Corrupting
skb_shared_info gives a trivial one-hop indirect call primitive via
uarg->ops->complete.

Replace ubuf_info_ops with a u8 type ID and dispatch to the known
completion callbacks in net_zcopy_complete(). Aside from removing the
vtable pointer, this shrinks struct ubuf_info from 16 to 8 bytes and
avoids an indirect call in the hot path.

A few callback implementations can reside in modules, so require
dynamic registration. Add a concise registration helper and use
static_call to avoid indirect calls where supported.

The .link_skb callback is only used by io_uring. Convert that into a
direct call.

Signed-off-by: Willem de Bruijn <willemb@google.com>

---

This patch touches tun, tap, xen, vhost-net, io_uring. If preferred,
I can split it into a series.

msg_zerocopy_complete is so simple that it can conceivably be inlined,
along with part of net_zcopy_complete, but chose simplicity over that
additional optimization.
---
 drivers/net/tap.c                   |  8 ++-
 drivers/net/tun.c                   |  8 ++-
 drivers/net/xen-netback/common.h    |  3 --
 drivers/net/xen-netback/interface.c |  2 +-
 drivers/net/xen-netback/netback.c   | 10 ++--
 drivers/vhost/net.c                 | 18 ++++---
 include/linux/io_uring/net.h        | 20 ++++++++
 include/linux/skbuff.h              | 27 ++++++----
 io_uring/notif.c                    | 13 ++---
 io_uring/notif.h                    |  3 +-
 net/core/skbuff.c                   | 80 ++++++++++++++++++++++++++---
 net/packet/af_packet.c              | 10 ++--
 12 files changed, 142 insertions(+), 60 deletions(-)

diff --git a/drivers/net/tap.c b/drivers/net/tap.c
index 2f19fb07bb2d..0f3ad8fd6ab6 100644
--- a/drivers/net/tap.c
+++ b/drivers/net/tap.c
@@ -665,12 +665,10 @@ static ssize_t tap_get_user(struct tap_queue *q, void *msg_control,
 		skb_set_network_header(skb, depth);
 
 	/* copy skb_ubuf_info for callback when skb has no error */
-	if (zerocopy) {
+	if (zerocopy)
 		skb_zcopy_init(skb, msg_control);
-	} else if (msg_control) {
-		struct ubuf_info *uarg = msg_control;
-		uarg->ops->complete(NULL, uarg, false);
-	}
+	else if (msg_control)
+		net_zcopy_complete(NULL, msg_control, false);
 
 	dev_queue_xmit(skb);
 	rcu_read_unlock();
diff --git a/drivers/net/tun.c b/drivers/net/tun.c
index dcec7311e601..8920c797acfd 100644
--- a/drivers/net/tun.c
+++ b/drivers/net/tun.c
@@ -1954,12 +1954,10 @@ static ssize_t tun_get_user(struct tun_struct *tun, struct tun_file *tfile,
 		skb->protocol = eth_type_trans(skb, tun->dev);
 
 	/* copy skb_ubuf_info for callback when skb has no error */
-	if (zerocopy) {
+	if (zerocopy)
 		skb_zcopy_init(skb, msg_control);
-	} else if (msg_control) {
-		struct ubuf_info *uarg = msg_control;
-		uarg->ops->complete(NULL, uarg, false);
-	}
+	else if (msg_control)
+		net_zcopy_complete(NULL, msg_control, false);
 
 	skb_reset_network_header(skb);
 	skb_probe_transport_header(skb);
diff --git a/drivers/net/xen-netback/common.h b/drivers/net/xen-netback/common.h
index 17421da139f2..78cc4365dd13 100644
--- a/drivers/net/xen-netback/common.h
+++ b/drivers/net/xen-netback/common.h
@@ -390,9 +390,6 @@ bool xenvif_rx_queue_tail(struct xenvif_queue *queue, struct sk_buff *skb);
 
 void xenvif_carrier_on(struct xenvif *vif);
 
-/* Callbacks from stack when TX packet can be released */
-extern const struct ubuf_info_ops xenvif_ubuf_ops;
-
 static inline pending_ring_idx_t nr_pending_reqs(struct xenvif_queue *queue)
 {
 	return MAX_PENDING_REQS -
diff --git a/drivers/net/xen-netback/interface.c b/drivers/net/xen-netback/interface.c
index a0a438881388..068ff15282e0 100644
--- a/drivers/net/xen-netback/interface.c
+++ b/drivers/net/xen-netback/interface.c
@@ -593,7 +593,7 @@ int xenvif_init_queue(struct xenvif_queue *queue)
 
 	for (i = 0; i < MAX_PENDING_REQS; i++) {
 		queue->pending_tx_info[i].callback_struct = (struct ubuf_info_msgzc)
-			{ { .ops = &xenvif_ubuf_ops },
+			{ { .type = UBUF_TYPE_XENVIF },
 			  { { .ctx = NULL,
 			      .desc = i } } };
 		queue->grant_tx_handle[i] = NETBACK_INVALID_HANDLE;
diff --git a/drivers/net/xen-netback/netback.c b/drivers/net/xen-netback/netback.c
index 3cdb7fd659db..176642363153 100644
--- a/drivers/net/xen-netback/netback.c
+++ b/drivers/net/xen-netback/netback.c
@@ -1158,7 +1158,7 @@ static int xenvif_handle_frag_list(struct xenvif_queue *queue, struct sk_buff *s
 	uarg = skb_shinfo(skb)->destructor_arg;
 	/* increase inflight counter to offset decrement in callback */
 	atomic_inc(&queue->inflight_packets);
-	uarg->ops->complete(NULL, uarg, true);
+	net_zcopy_complete(NULL, uarg, true);
 	skb_shinfo(skb)->destructor_arg = NULL;
 
 	/* Fill the skb with the new (local) frags. */
@@ -1315,10 +1315,6 @@ static void xenvif_zerocopy_callback(struct sk_buff *skb,
 	xenvif_skb_zerocopy_complete(queue);
 }
 
-const struct ubuf_info_ops xenvif_ubuf_ops = {
-	.complete = xenvif_zerocopy_callback,
-};
-
 static inline void xenvif_tx_dealloc_action(struct xenvif_queue *queue)
 {
 	struct gnttab_unmap_grant_ref *gop;
@@ -1760,6 +1756,8 @@ static int __init netback_init(void)
 		fatal_skb_slots = XEN_NETBK_LEGACY_SLOTS_MAX;
 	}
 
+	net_zcopy_register(UBUF_TYPE_XENVIF, xenvif_zerocopy_callback);
+
 	rc = xenvif_xenbus_init();
 	if (rc)
 		goto failed_init;
@@ -1771,6 +1769,7 @@ static int __init netback_init(void)
 	return 0;
 
 failed_init:
+	net_zcopy_register(UBUF_TYPE_XENVIF, NULL);
 	return rc;
 }
 
@@ -1782,6 +1781,7 @@ static void __exit netback_fini(void)
 	debugfs_remove_recursive(xen_netback_dbg_root);
 #endif /* CONFIG_DEBUG_FS */
 	xenvif_xenbus_fini();
+	net_zcopy_register(UBUF_TYPE_XENVIF, NULL);
 }
 module_exit(netback_fini);
 
diff --git a/drivers/vhost/net.c b/drivers/vhost/net.c
index 4570a4ffbab2..435d8ed3851f 100644
--- a/drivers/vhost/net.c
+++ b/drivers/vhost/net.c
@@ -431,10 +431,6 @@ static void vhost_zerocopy_complete(struct sk_buff *skb,
 	rcu_read_unlock_bh();
 }
 
-static const struct ubuf_info_ops vhost_ubuf_ops = {
-	.complete = vhost_zerocopy_complete,
-};
-
 static inline unsigned long busy_clock(void)
 {
 	return local_clock() >> 10;
@@ -926,7 +922,7 @@ static void handle_tx_zerocopy(struct vhost_net *net, struct socket *sock)
 			vq->heads[nvq->upend_idx].len = VHOST_DMA_IN_PROGRESS;
 			ubuf->ctx = nvq->ubufs;
 			ubuf->desc = nvq->upend_idx;
-			ubuf->ubuf.ops = &vhost_ubuf_ops;
+			ubuf->ubuf.type = UBUF_TYPE_VHOST;
 			ubuf->ubuf.flags = SKBFL_ZEROCOPY_FRAG;
 			refcount_set(&ubuf->ubuf.refcnt, 1);
 			msg.msg_control = &ctl;
@@ -1895,15 +1891,25 @@ static struct miscdevice vhost_net_misc = {
 
 static int __init vhost_net_init(void)
 {
+	int rc;
+
 	if (experimental_zcopytx)
 		vhost_net_enable_zcopy(VHOST_NET_VQ_TX);
-	return misc_register(&vhost_net_misc);
+
+	net_zcopy_register(UBUF_TYPE_VHOST, vhost_zerocopy_complete);
+
+	rc = misc_register(&vhost_net_misc);
+	if (rc)
+		net_zcopy_register(UBUF_TYPE_VHOST, NULL);
+
+	return rc;
 }
 module_init(vhost_net_init);
 
 static void __exit vhost_net_exit(void)
 {
 	misc_deregister(&vhost_net_misc);
+	net_zcopy_register(UBUF_TYPE_VHOST, NULL);
 }
 module_exit(vhost_net_exit);
 
diff --git a/include/linux/io_uring/net.h b/include/linux/io_uring/net.h
index b58f39fed4d5..1759b1c22597 100644
--- a/include/linux/io_uring/net.h
+++ b/include/linux/io_uring/net.h
@@ -2,10 +2,19 @@
 #ifndef _LINUX_IO_URING_NET_H
 #define _LINUX_IO_URING_NET_H
 
+#include <linux/errno.h>
+#include <linux/types.h>
+#include <net/net_debug.h>
+
 struct io_uring_cmd;
+struct sk_buff;
+struct ubuf_info;
 
 #if defined(CONFIG_IO_URING)
 int io_uring_cmd_sock(struct io_uring_cmd *cmd, unsigned int issue_flags);
+void io_tx_ubuf_complete(struct sk_buff *skb, struct ubuf_info *uarg,
+			 bool success);
+int io_link_skb(struct sk_buff *skb, struct ubuf_info *uarg);
 
 #else
 static inline int io_uring_cmd_sock(struct io_uring_cmd *cmd,
@@ -13,6 +22,17 @@ static inline int io_uring_cmd_sock(struct io_uring_cmd *cmd,
 {
 	return -EOPNOTSUPP;
 }
+
+static inline void io_tx_ubuf_complete(struct sk_buff *skb,
+				       struct ubuf_info *uarg, bool success)
+{
+	DEBUG_NET_WARN_ON_ONCE(1);
+}
+
+static inline int io_link_skb(struct sk_buff *skb, struct ubuf_info *uarg)
+{
+	return -EOPNOTSUPP;
+}
 #endif
 
 #endif
diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
index 6787fcf6ac3e..73365c5de6f9 100644
--- a/include/linux/skbuff.h
+++ b/include/linux/skbuff.h
@@ -534,13 +534,18 @@ enum {
 #define SKBFL_ALL_ZEROCOPY	(SKBFL_ZEROCOPY_FRAG | SKBFL_PURE_ZEROCOPY | \
 				 SKBFL_DONT_ORPHAN | SKBFL_MANAGED_FRAG_REFS)
 
-struct ubuf_info_ops {
-	void (*complete)(struct sk_buff *, struct ubuf_info *,
-			 bool zerocopy_success);
-	/* has to be compatible with skb_zcopy_set() */
-	int (*link_skb)(struct sk_buff *skb, struct ubuf_info *uarg);
+enum ubuf_info_type {
+	UBUF_TYPE_NONE = 0,
+	UBUF_TYPE_MSG_ZEROCOPY,
+	UBUF_TYPE_IO_URING,
+	UBUF_TYPE_TPACKET,
+	UBUF_TYPE_VHOST,
+	UBUF_TYPE_XENVIF,
 };
 
+typedef void (*ubuf_complete_fn)(struct sk_buff *skb, struct ubuf_info *uarg,
+				 bool zerocopy_success);
+
 /*
  * The callback notifies userspace to release buffers when skb DMA is done in
  * lower device, the skb last reference should be 0 when calling this.
@@ -550,8 +555,8 @@ struct ubuf_info_ops {
  * The desc field is used to track userspace buffer index.
  */
 struct ubuf_info {
-	const struct ubuf_info_ops *ops;
 	refcount_t refcnt;
+	u8 type;
 	u8 flags;
 };
 
@@ -1757,7 +1762,9 @@ static inline void skb_set_end_offset(struct sk_buff *skb, unsigned int offset)
 }
 #endif
 
-extern const struct ubuf_info_ops msg_zerocopy_ubuf_ops;
+void net_zcopy_register(enum ubuf_info_type type, ubuf_complete_fn fn);
+void net_zcopy_complete(struct sk_buff *skb, struct ubuf_info *uarg,
+			bool zerocopy_success);
 
 struct ubuf_info *msg_zerocopy_realloc(struct sock *sk, size_t size,
 				       struct ubuf_info *uarg, bool devmem);
@@ -1843,13 +1850,13 @@ static inline void skb_zcopy_set(struct sk_buff *skb, struct ubuf_info *uarg,
 static inline void net_zcopy_put(struct ubuf_info *uarg)
 {
 	if (uarg)
-		uarg->ops->complete(NULL, uarg, true);
+		net_zcopy_complete(NULL, uarg, true);
 }
 
 static inline void net_zcopy_put_abort(struct ubuf_info *uarg, bool have_uref)
 {
 	if (uarg) {
-		if (uarg->ops == &msg_zerocopy_ubuf_ops)
+		if (uarg->type == UBUF_TYPE_MSG_ZEROCOPY)
 			msg_zerocopy_put_abort(uarg, have_uref);
 		else if (have_uref)
 			net_zcopy_put(uarg);
@@ -1862,7 +1869,7 @@ static inline void skb_zcopy_clear(struct sk_buff *skb, bool zerocopy_success)
 	struct ubuf_info *uarg = skb_zcopy(skb);
 
 	if (uarg) {
-		uarg->ops->complete(skb, uarg, zerocopy_success);
+		net_zcopy_complete(skb, uarg, zerocopy_success);
 
 		skb_shinfo(skb)->flags &= ~SKBFL_ALL_ZEROCOPY;
 	}
diff --git a/io_uring/notif.c b/io_uring/notif.c
index 6c49aff515df..cb2b8afd400c 100644
--- a/io_uring/notif.c
+++ b/io_uring/notif.c
@@ -10,8 +10,6 @@
 #include "notif.h"
 #include "rsrc.h"
 
-static const struct ubuf_info_ops io_ubuf_ops;
-
 static void io_notif_tw_complete(struct io_tw_req tw_req, io_tw_token_t tw)
 {
 	struct io_kiocb *notif = tw_req.req;
@@ -68,7 +66,7 @@ void io_tx_ubuf_complete(struct sk_buff *skb, struct ubuf_info *uarg,
 	__io_req_task_work_add(notif, tw_flags);
 }
 
-static int io_link_skb(struct sk_buff *skb, struct ubuf_info *uarg)
+int io_link_skb(struct sk_buff *skb, struct ubuf_info *uarg)
 {
 	struct io_notif_data *nd, *prev_nd;
 	struct io_kiocb *prev_notif, *notif;
@@ -89,7 +87,7 @@ static int io_link_skb(struct sk_buff *skb, struct ubuf_info *uarg)
 	if (unlikely(nd->head != nd || nd->next))
 		return -EEXIST;
 	/* don't mix zc providers */
-	if (unlikely(prev_uarg->ops != &io_ubuf_ops))
+	if (unlikely(prev_uarg->type != UBUF_TYPE_IO_URING))
 		return -EEXIST;
 
 	prev_nd = container_of(prev_uarg, struct io_notif_data, uarg);
@@ -107,11 +105,6 @@ static int io_link_skb(struct sk_buff *skb, struct ubuf_info *uarg)
 	return 0;
 }
 
-static const struct ubuf_info_ops io_ubuf_ops = {
-	.complete = io_tx_ubuf_complete,
-	.link_skb = io_link_skb,
-};
-
 struct io_kiocb *io_alloc_notif(struct io_ring_ctx *ctx)
 	__must_hold(&ctx->uring_lock)
 {
@@ -137,7 +130,7 @@ struct io_kiocb *io_alloc_notif(struct io_ring_ctx *ctx)
 	nd->head = nd;
 
 	nd->uarg.flags = IO_NOTIF_UBUF_FLAGS;
-	nd->uarg.ops = &io_ubuf_ops;
+	nd->uarg.type = UBUF_TYPE_IO_URING;
 	refcount_set(&nd->uarg.refcnt, 1);
 	return notif;
 }
diff --git a/io_uring/notif.h b/io_uring/notif.h
index f3589cfef4a9..b0e0e179a80f 100644
--- a/io_uring/notif.h
+++ b/io_uring/notif.h
@@ -4,6 +4,7 @@
 #include <linux/uio.h>
 #include <net/sock.h>
 #include <linux/nospec.h>
+#include <linux/io_uring/net.h>
 
 #include "rsrc.h"
 
@@ -24,8 +25,6 @@ struct io_notif_data {
 };
 
 struct io_kiocb *io_alloc_notif(struct io_ring_ctx *ctx);
-void io_tx_ubuf_complete(struct sk_buff *skb, struct ubuf_info *uarg,
-			 bool success);
 
 static inline struct io_notif_data *io_notif_to_data(struct io_kiocb *notif)
 {
diff --git a/net/core/skbuff.c b/net/core/skbuff.c
index 48f676411724..3ec9851d0a10 100644
--- a/net/core/skbuff.c
+++ b/net/core/skbuff.c
@@ -92,6 +92,8 @@
 #include <linux/capability.h>
 #include <linux/user_namespace.h>
 #include <linux/indirect_call_wrapper.h>
+#include <linux/io_uring/net.h>
+#include <linux/static_call.h>
 #include <linux/textsearch.h>
 
 #include "dev.h"
@@ -1737,7 +1739,7 @@ static struct ubuf_info *msg_zerocopy_alloc(struct sock *sk, size_t size,
 		return NULL;
 	}
 
-	uarg->ubuf.ops = &msg_zerocopy_ubuf_ops;
+	uarg->ubuf.type = UBUF_TYPE_MSG_ZEROCOPY;
 	uarg->id = ((u32)atomic_inc_return(&sk->sk_zckey)) - 1;
 	uarg->len = 1;
 	uarg->bytelen = size;
@@ -1763,7 +1765,7 @@ struct ubuf_info *msg_zerocopy_realloc(struct sock *sk, size_t size,
 		u32 bytelen, next;
 
 		/* there might be non MSG_ZEROCOPY users */
-		if (uarg->ops != &msg_zerocopy_ubuf_ops)
+		if (uarg->type != UBUF_TYPE_MSG_ZEROCOPY)
 			return NULL;
 
 		/* realloc only when socket is locked (TCP, UDP cork),
@@ -1898,10 +1900,72 @@ void msg_zerocopy_put_abort(struct ubuf_info *uarg, bool have_uref)
 }
 EXPORT_SYMBOL_GPL(msg_zerocopy_put_abort);
 
-const struct ubuf_info_ops msg_zerocopy_ubuf_ops = {
-	.complete = msg_zerocopy_complete,
-};
-EXPORT_SYMBOL_GPL(msg_zerocopy_ubuf_ops);
+static void net_zcopy_unregistered(struct sk_buff *skb, struct ubuf_info *uarg,
+				   bool zerocopy_success)
+{
+	DEBUG_NET_WARN_ON_ONCE(1);
+}
+
+DEFINE_STATIC_CALL(ubuf_complete_tpacket, net_zcopy_unregistered);
+DEFINE_STATIC_CALL(ubuf_complete_vhost, net_zcopy_unregistered);
+DEFINE_STATIC_CALL(ubuf_complete_xenvif, net_zcopy_unregistered);
+
+void net_zcopy_register(enum ubuf_info_type type, ubuf_complete_fn fn)
+{
+	ubuf_complete_fn cur;
+	bool reg = fn;
+
+	if (!fn)
+		fn = net_zcopy_unregistered;
+
+	switch (type) {
+	case UBUF_TYPE_TPACKET:
+		cur = static_call_query(ubuf_complete_tpacket);
+		static_call_update(ubuf_complete_tpacket, fn);
+		break;
+	case UBUF_TYPE_VHOST:
+		cur = static_call_query(ubuf_complete_vhost);
+		static_call_update(ubuf_complete_vhost, fn);
+		break;
+	case UBUF_TYPE_XENVIF:
+		cur = static_call_query(ubuf_complete_xenvif);
+		static_call_update(ubuf_complete_xenvif, fn);
+		break;
+	default:
+		WARN_ON_ONCE(1);
+		return;
+	}
+
+	WARN_ON_ONCE(reg ? cur != net_zcopy_unregistered :
+			   cur == net_zcopy_unregistered);
+}
+EXPORT_SYMBOL_GPL(net_zcopy_register);
+
+void net_zcopy_complete(struct sk_buff *skb, struct ubuf_info *uarg,
+			bool zerocopy_success)
+{
+	switch (uarg->type) {
+	case UBUF_TYPE_MSG_ZEROCOPY:
+		msg_zerocopy_complete(skb, uarg, zerocopy_success);
+		break;
+	case UBUF_TYPE_IO_URING:
+		io_tx_ubuf_complete(skb, uarg, zerocopy_success);
+		break;
+	case UBUF_TYPE_TPACKET:
+		static_call(ubuf_complete_tpacket)(skb, uarg, zerocopy_success);
+		break;
+	case UBUF_TYPE_VHOST:
+		static_call(ubuf_complete_vhost)(skb, uarg, zerocopy_success);
+		break;
+	case UBUF_TYPE_XENVIF:
+		static_call(ubuf_complete_xenvif)(skb, uarg, zerocopy_success);
+		break;
+	default:
+		DEBUG_NET_WARN_ON_ONCE(1);
+		break;
+	}
+}
+EXPORT_SYMBOL_GPL(net_zcopy_complete);
 
 int skb_zerocopy_iter_stream(struct sock *sk, struct sk_buff *skb,
 			     struct msghdr *msg, int len,
@@ -1910,8 +1974,8 @@ int skb_zerocopy_iter_stream(struct sock *sk, struct sk_buff *skb,
 {
 	int err, orig_len = skb->len;
 
-	if (uarg->ops->link_skb) {
-		err = uarg->ops->link_skb(skb, uarg);
+	if (uarg->type == UBUF_TYPE_IO_URING) {
+		err = io_link_skb(skb, uarg);
 		if (err)
 			return err;
 	} else {
diff --git a/net/packet/af_packet.c b/net/packet/af_packet.c
index cf6429f09477..e08d5e315b65 100644
--- a/net/packet/af_packet.c
+++ b/net/packet/af_packet.c
@@ -2606,10 +2606,6 @@ static void tpacket_ubuf_complete(struct sk_buff *skb, struct ubuf_info *uarg,
 	sk_free(&po->sk);
 }
 
-static const struct ubuf_info_ops tpacket_ubuf_ops = {
-	.complete = tpacket_ubuf_complete,
-};
-
 static int tpacket_fill_skb(struct packet_sock *po, struct sk_buff *skb,
 		struct net_device *dev, void *data, int tp_len,
 		__be16 proto, unsigned char *addr, int hlen, int copylen,
@@ -2942,7 +2938,7 @@ static int tpacket_snd(struct packet_sock *po, struct msghdr *msg)
 		}
 		uarg->po = po;
 		uarg->ph = ph;
-		uarg->ubuf.ops = &tpacket_ubuf_ops;
+		uarg->ubuf.type = UBUF_TYPE_TPACKET;
 		uarg->ubuf.flags = SKBFL_ZEROCOPY_FRAG;
 		refcount_set(&uarg->ubuf.refcnt, 1);
 
@@ -4931,6 +4927,7 @@ static void __exit packet_exit(void)
 	proto_unregister(&packet_proto);
 	unregister_netdevice_notifier(&packet_netdev_notifier);
 	unregister_pernet_subsys(&packet_net_ops);
+	net_zcopy_register(UBUF_TYPE_TPACKET, NULL);
 }
 
 static int __init packet_init(void)
@@ -4946,6 +4943,8 @@ static int __init packet_init(void)
 	rc = proto_register(&packet_proto, 0);
 	if (rc)
 		goto out_notifier;
+
+	net_zcopy_register(UBUF_TYPE_TPACKET, tpacket_ubuf_complete);
 	rc = sock_register(&packet_family_ops);
 	if (rc)
 		goto out_proto;
@@ -4953,6 +4952,7 @@ static int __init packet_init(void)
 	return 0;
 
 out_proto:
+	net_zcopy_register(UBUF_TYPE_TPACKET, NULL);
 	proto_unregister(&packet_proto);
 out_notifier:
 	unregister_netdevice_notifier(&packet_netdev_notifier);
-- 
2.56.0.385.gac671c819a-goog

Re: [PATCH net-next] net: skbuff: replace ubuf_info ops ptr with type id

[Jakub Kicinski]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
---
net: skbuff: replace ubuf_info ops ptr with type id

This commit replaces the ubuf_info_ops vtable pointer in struct ubuf_info
with a u8 type ID. net_zcopy_complete() switches on that ID to pick one of
the known completion callbacks, and callbacks that live in modules are
registered through static_call. The io_uring link_skb callback becomes a
direct call.

> diff --git a/net/core/skbuff.c b/net/core/skbuff.c
> index 48f6764117243..3ec9851d0a10e 100644
> --- a/net/core/skbuff.c
> +++ b/net/core/skbuff.c

[ ... ]

> @@ -1898,10 +1900,72 @@ void msg_zerocopy_put_abort(struct ubuf_info *uarg, bool have_uref)

[ ... ]

> +void net_zcopy_complete(struct sk_buff *skb, struct ubuf_info *uarg,
> +			bool zerocopy_success)
> +{
> +	switch (uarg->type) {
> +	case UBUF_TYPE_MSG_ZEROCOPY:
> +		msg_zerocopy_complete(skb, uarg, zerocopy_success);
> +		break;

[Severity: High]
Is the indirect call primitive described in the commit message still
reachable through this path?

The commit message says:

  Corrupting skb_shared_info gives a trivial one-hop indirect call
  primitive via uarg->ops->complete.

Take the same threat model: an out-of-bounds write that overwrites
destructor_arg and flags in skb_shared_info. A forged blob then needs only
refcnt == 1, type == UBUF_TYPE_MSG_ZEROCOPY, mmp.user == NULL and len == 0.
Completion goes:

skb_zcopy_clear()
  net_zcopy_complete()
    msg_zerocopy_complete()     /* refcount_dec_and_test() succeeds */
      __msg_zerocopy_callback()

__msg_zerocopy_callback() derives the skb from the uarg address. A real
msgzc uarg sits inside skb->cb, so the derived skb lands in the same
forged region:

	struct sk_buff *tail, *skb = skb_from_uarg(uarg);
	struct sock_exterr_skb *serr;
	struct sock *sk = skb->sk;
	...
	mm_unaccount_pinned_pages(&uarg->mmp);
	...
	if (!uarg->len || sock_flag(sk, SOCK_DEAD))
		goto release;
	...
release:
	consume_skb(skb);
	sock_put(sk);

With len == 0, sk is not dereferenced before consume_skb() runs on the
forged skb. consume_skb() then goes through
__kfree_skb()->skb_release_all()->skb_release_head_state(), which calls
skb->destructor(skb).

That destructor pointer comes from attacker memory at a fixed offset just
past cb[]. sock_put(sk) only runs after the indirect call.

So forging costs about the same as before: one controlled region at a
known address plus a target. Before this patch, the same route could be
reached by pointing a forged ops at &msg_zerocopy_ubuf_ops. Now the forged
ops pointer is a single byte, and no data symbol address is needed.

The other type IDs also hand forged memory to callbacks that trust
container_of():

  UBUF_TYPE_TPACKET: tpacket_ubuf_complete() writes status through the
  forged tu->po and tu->ph, then calls sk_free(&po->sk).

  UBUF_TYPE_VHOST: vhost_zerocopy_complete() writes
  vq->heads[ubuf->desc].len through forged ctx and vq pointers.

  UBUF_TYPE_IO_URING: io_tx_ubuf_complete() queues task_work on a notif
  derived from forged memory.

Would closing this need one of two changes? One is checking that
destructor_arg points to a real object of the claimed type. The other is
stopping msg_zerocopy completion from deriving and freeing an skb (and
calling its destructor) from the uarg address.

> +	case UBUF_TYPE_IO_URING:
> +		io_tx_ubuf_complete(skb, uarg, zerocopy_success);
> +		break;

[ ... ]

Re: [PATCH net-next] net: skbuff: replace ubuf_info ops ptr with type id

[Willem de Bruijn]

Jakub Kicinski wrote:
> This is an AI-generated review of your patch. The human sending this
> email has considered the AI review valid, or at least plausible.
> ---
> net: skbuff: replace ubuf_info ops ptr with type id
> 
> This commit replaces the ubuf_info_ops vtable pointer in struct ubuf_info
> with a u8 type ID. net_zcopy_complete() switches on that ID to pick one of
> the known completion callbacks, and callbacks that live in modules are
> registered through static_call. The io_uring link_skb callback becomes a
> direct call.
> 
> > diff --git a/net/core/skbuff.c b/net/core/skbuff.c
> > index 48f6764117243..3ec9851d0a10e 100644
> > --- a/net/core/skbuff.c
> > +++ b/net/core/skbuff.c
> 
> [ ... ]
> 
> > @@ -1898,10 +1900,72 @@ void msg_zerocopy_put_abort(struct ubuf_info *uarg, bool have_uref)
> 
> [ ... ]
> 
> > +void net_zcopy_complete(struct sk_buff *skb, struct ubuf_info *uarg,
> > +			bool zerocopy_success)
> > +{
> > +	switch (uarg->type) {
> > +	case UBUF_TYPE_MSG_ZEROCOPY:
> > +		msg_zerocopy_complete(skb, uarg, zerocopy_success);
> > +		break;
> 
> [Severity: High]
> Is the indirect call primitive described in the commit message still
> reachable through this path?
> 
> The commit message says:
> 
>   Corrupting skb_shared_info gives a trivial one-hop indirect call
>   primitive via uarg->ops->complete.
> 
> Take the same threat model: an out-of-bounds write that overwrites
> destructor_arg and flags in skb_shared_info. A forged blob then needs only
> refcnt == 1, type == UBUF_TYPE_MSG_ZEROCOPY, mmp.user == NULL and len == 0.
> Completion goes:
> 
> skb_zcopy_clear()
>   net_zcopy_complete()
>     msg_zerocopy_complete()     /* refcount_dec_and_test() succeeds */
>       __msg_zerocopy_callback()
> 
> __msg_zerocopy_callback() derives the skb from the uarg address. A real
> msgzc uarg sits inside skb->cb, so the derived skb lands in the same
> forged region:
> 
> 	struct sk_buff *tail, *skb = skb_from_uarg(uarg);
> 	struct sock_exterr_skb *serr;
> 	struct sock *sk = skb->sk;
> 	...
> 	mm_unaccount_pinned_pages(&uarg->mmp);
> 	...
> 	if (!uarg->len || sock_flag(sk, SOCK_DEAD))
> 		goto release;
> 	...
> release:
> 	consume_skb(skb);
> 	sock_put(sk);
> 
> With len == 0, sk is not dereferenced before consume_skb() runs on the
> forged skb. consume_skb() then goes through
> __kfree_skb()->skb_release_all()->skb_release_head_state(), which calls
> skb->destructor(skb).
> 
> That destructor pointer comes from attacker memory at a fixed offset just
> past cb[]. sock_put(sk) only runs after the indirect call.
> 
> So forging costs about the same as before: one controlled region at a
> known address plus a target. Before this patch, the same route could be
> reached by pointing a forged ops at &msg_zerocopy_ubuf_ops. Now the forged
> ops pointer is a single byte, and no data symbol address is needed.
> 
> The other type IDs also hand forged memory to callbacks that trust
> container_of():
> 
>   UBUF_TYPE_TPACKET: tpacket_ubuf_complete() writes status through the
>   forged tu->po and tu->ph, then calls sk_free(&po->sk).
> 
>   UBUF_TYPE_VHOST: vhost_zerocopy_complete() writes
>   vq->heads[ubuf->desc].len through forged ctx and vq pointers.
> 
>   UBUF_TYPE_IO_URING: io_tx_ubuf_complete() queues task_work on a notif
>   derived from forged memory.
> 
> Would closing this need one of two changes? One is checking that
> destructor_arg points to a real object of the claimed type. The other is
> stopping msg_zerocopy completion from deriving and freeing an skb (and
> calling its destructor) from the uarg address.

Thanks. I was aware of the ability to forge the sk. But not of all paths.

This patch intends to fix one path, before moving on to the next.

Alternatively, I can try to send a single series that addresses all. It
probably offers little protection otherwise.

Though this also just removes an indirect function call from the hot
path, so arguably it can stand on its own for that too.

Re: [PATCH net-next] net: skbuff: replace ubuf_info ops ptr with type id

[patchwork-bot+netdevbpf]

Hello:

This patch was applied to netdev/net-next.git (main)
by Jakub Kicinski <kuba@kernel.org>:

On Tue,  3 Nov 2026 14:46:20 -0500 you wrote:
> From: Willem de Bruijn <willemb@google.com>
> 
> Avoid an easily corruptible callback pointer on skb linear
> out-of-bounds write bugs.
> 
> skb_shinfo(skb)->destructor_arg points to a struct ubuf_info, whose
> first member is a ubuf_info_ops vtable pointer. Corrupting
> skb_shared_info gives a trivial one-hop indirect call primitive via
> uarg->ops->complete.
> 
> [...]

Here is the summary with links:
  - [net-next] net: skbuff: replace ubuf_info ops ptr with type id
    https://git.kernel.org/netdev/net-next/c/ecca9bb2c855

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html