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

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.

quoted

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

[ ... ]

quoted

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

[ ... ]

quoted

+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.