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

[Paolo Abeni <pabeni@redhat.com>]

On Thu, 2021-05-06 at 10:32 -0400, Willem de Bruijn wrote:
> On Thu, May 6, 2021 at 7:07 AM Paolo Abeni <pabeni@redhat.com> wrote:
> > On Wed, 2021-05-05 at 13:30 -0400, Willem de Bruijn wrote:
> > > On Wed, May 5, 2021 at 1:28 PM Paolo Abeni <pabeni@redhat.com> wrote:
> > > > 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.
> > > 
> > > Okay, great.
> > > 
> > > > 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.
> > > 
> > > I think the first does apply, as it applies to any protocol that uses
> > > sock_wfree, not just tcp_wfree? Anyway, the point is moot indeed.
> > 
> > If we want to be safe about future possible sock_wfree users, I think
> > the approach here should be different: in skb_segment(), tail-
> > > destructor is expected to be NULL, while skb_segment_list(), all the
> > list skbs can be owned by the same socket. Possibly we could open-
> > code skb_release_head_state(), omitting the skb orphaning part
> > for sock_wfree() destructor.
> > 
> > Note that the this is not currently needed - sock_wfree destructor
> > can't reach there.
> > 
> > Given all the above, I'm unsure if you are fine with (or at least do
> > not oppose to) the code proposed in this patch?
> 
> Yes. Thanks for clarifying, Paolo.

Thank you for reviewing!

@David, @Jakub: I see this series is already archived as "change
requested", should I repost?

Thanks!

Paolo