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

[Willem de Bruijn <willemdebruijn.kernel@gmail.com>]

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.