Re: [PATCH net v3] net: gro: don't merge zcopy skbs

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

Willem de Bruijn wrote:
> Sabrina Dubroca wrote:
> > skb_gro_receive() can currently copy frags between the source and GRO
> > skb, without checking the zerocopy status, and in particular the
> > SKBFL_MANAGED_FRAG_REFS flag.
> > 
> > When SKBFL_MANAGED_FRAG_REFS is set, the skb doesn't hold a reference
> > on the pages in shinfo->frags. Appending those frags to another skb's
> > frags without fixing up the page refcount can lead to UAF.
> > 
> > When either the last skb in the GRO chain (the one we would append
> > frags to) or the source skb is zerocopy, don't merge the skbs.
> > 
> > Fixes: 753f1ca4e1e5 ("net: introduce managed frags infrastructure")
> > Reported-by: Huzaifa Sidhpurwala <huzaifas@redhat.com>
> > Assisted-by: Claude:claude-mythos-preview
> > Signed-off-by: Sabrina Dubroca <sd@queasysnail.net>
> > ---
> >  net/core/gro.c | 3 +++
> >  1 file changed, 3 insertions(+)
> > 
> > Huzaifa has found this to be exploitable to overwrite the page cache
> > 
> > v3: maybe no stupid mistake this time. sorry again.
> > v2: as Eric suggested, don't merge those skbs
> >     https://lore.kernel.org/netdev/9f5afc14ea4ecd22c70d6eaf279a94d10fe29448.1779289597.git.sd@queasysnail.net/
> > v1: https://lore.kernel.org/netdev/4d583fc5401298453d0a2f1b4719a15be30c8e49.1779194090.git.sd@queasysnail.net/
> > 
> > diff --git a/net/core/gro.c b/net/core/gro.c
> > index 31d21de5b15a..bae89062a053 100644
> > --- a/net/core/gro.c
> > +++ b/net/core/gro.c
> > @@ -109,6 +109,9 @@ int skb_gro_receive(struct sk_buff *p, struct sk_buff *skb)
> >  	if (p->pp_recycle != skb->pp_recycle)
> >  		return -ETOOMANYREFS;
> >  
> > +	if (skb_zcopy(p) || skb_zcopy(skb))
> > +		return -ETOOMANYREFS;
> > +
> 
> As Pavel pointed out in a previous version, MSG_ZEROCOPY skbs are not
> looped onto the receive path. From that PoV it's fine to add this
> check as it will not match them.
> 
> What is the path that leads to managed frag skbs in the receive path?
> I can believe that it exists, e.g., a page pool. But since Pavel also
> brought up this question and knows the managed frag use much better
> than I do.

The key is in skb_gro_receive. That can get called earlier in the rx
path than the skb_orphan_frags_rx check in __netif_receive_skb_core.

Not coalescing them LGTM. Thanks.

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