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

[Jakub Kicinski <kuba@kernel.org>]

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;

[ ... ]