Re: [PATCH net 1/4] net: fix double-free on fraglist GSO skbs

[Paolo Abeni <pabeni@redhat.com>]

On Wed, 2021-05-05 at 12:13 -0400, Willem de Bruijn wrote:
> On Wed, May 5, 2021 at 11:37 AM Paolo Abeni <pabeni@redhat.com> wrote:
> > While segmenting a SKB_GSO_FRAGLIST GSO packet, if the destructor
> > callback is available, the skb destructor is invoked on each
> > aggregated packet via skb_release_head_state().
> > 
> > Such field (and the pairer skb->sk) is left untouched, so the same
> > destructor is invoked again when the segmented skbs are freed, leading
> > to double-free/UaF of the relevant socket.
> 
> Similar to skb_segment, should the destructor be swapped with the last
> segment and callback delayed, instead of called immediately as part of
> segmentation?
> 
>         /* Following permits correct backpressure, for protocols
>          * using skb_set_owner_w().
>          * Idea is to tranfert ownership from head_skb to last segment.
>          */
>         if (head_skb->destructor == sock_wfree) {
>                 swap(tail->truesize, head_skb->truesize);
>                 swap(tail->destructor, head_skb->destructor);
>                 swap(tail->sk, head_skb->sk);
>         }

My understanding is that one assumption in the original
SKB_GSO_FRAGLIST implementation was that SKB_GSO_FRAGLIST skbs are not
owned by any socket. 

AFAICS the above assumption was true until:

commit c75fb320d482a5ce6e522378d137fd2c3bf79225
Author: Paolo Abeni <pabeni@redhat.com>
Date:   Fri Apr 9 13:04:37 2021 +0200

    veth: use skb_orphan_partial instead of skb_orphan

after that, if the skb is owned, skb->destructor is sock_efree(), so
the above code should not trigger.

More importantly SKB_GSO_FRAGLIST can only be applied if the inner-
most protocol is UDP, so
commit 432c856fcf45c468fffe2e5029cb3f95c7dc9475
and d6a4a10411764cf1c3a5dad4f06c5ebe5194488b should not be relevant. 

Thanks!

Paolo