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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAF=yD-+XLDTzzBsPsMW-s9t0Ur3ux8w93VOAyHJ91E_cZLQS7w@mail.gmail.com>
Date:   Thu, 6 May 2021 10:32:16 -0400
From:   Willem de Bruijn <willemdebruijn.kernel@...il.com>
To:     Paolo Abeni <pabeni@...hat.com>
Cc:     Network Development <netdev@...r.kernel.org>,
        "David S. Miller" <davem@...emloft.net>,
        Jakub Kicinski <kuba@...nel.org>,
        Steffen Klassert <steffen.klassert@...unet.com>,
        Willem de Bruijn <willemb@...gle.com>,
        Miaohe Lin <linmiaohe@...wei.com>
Subject: Re: [PATCH net 1/4] net: fix double-free on fraglist GSO skbs

On Thu, May 6, 2021 at 7:07 AM Paolo Abeni <pabeni@...hat.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@...hat.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@...hat.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@...hat.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.