netdev - Re: [PATCH net v2 2/2] net: netem: fix use after free and double free with packet corruption

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 for Android: free password hash cracker in your pocket

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

Message-ID: <CAM_iQpWakh+Tv6URcGtD4FJ-TvOLzf8p2EvYdv8trebfHEk_8g@mail.gmail.com>
Date:   Mon, 17 Jun 2019 15:33:49 -0700
From:   Cong Wang <xiyou.wangcong@...il.com>
To:     Jakub Kicinski <jakub.kicinski@...ronome.com>
Cc:     David Miller <davem@...emloft.net>,
        Stephen Hemminger <stephen@...workplumber.org>,
        Jamal Hadi Salim <jhs@...atatu.com>,
        Jiri Pirko <jiri@...nulli.us>,
        netem@...ts.linux-foundation.org,
        Linux Kernel Network Developers <netdev@...r.kernel.org>,
        oss-drivers@...ronome.com, Eric Dumazet <edumazet@...gle.com>,
        posk@...gle.com, Neil Horman <nhorman@...driver.com>
Subject: Re: [PATCH net v2 2/2] net: netem: fix use after free and double free
 with packet corruption

On Mon, Jun 17, 2019 at 11:11 AM Jakub Kicinski
<jakub.kicinski@...ronome.com> wrote:
>
> Brendan reports that the use of netem's packet corruption capability
> leads to strange crashes.  This seems to be caused by
> commit d66280b12bd7 ("net: netem: use a list in addition to rbtree")
> which uses skb->next pointer to construct a fast-path queue of
> in-order skbs.
>
> Packet corruption code has to invoke skb_gso_segment() in case
> of skbs in need of GSO.  skb_gso_segment() returns a list of
> skbs.  If next pointers of the skbs on that list do not get cleared
> fast path list may point to freed skbs or skbs which are also on
> the RB tree.
>
> Let's say skb gets segmented into 3 frames:
>
> A -> B -> C
>
> A gets hooked to the t_head t_tail list by tfifo_enqueue(), but it's
> next pointer didn't get cleared so we have:
>
> h t
> |/
> A -> B -> C
>
> Now if B and C get also get enqueued successfully all is fine, because
> tfifo_enqueue() will overwrite the list in order.  IOW:
>
> Enqueue B:
>
> h    t
> |    |
> A -> B    C
>
> Enqueue C:
>
> h         t
> |         |
> A -> B -> C
>
> But if B and C get reordered we may end up with:
>
> h t            RB tree
> |/                |
> A -> B -> C       B
>                    \
>                     C
>
> Or if they get dropped just:
>
> h t
> |/
> A -> B -> C
>
> where A and B are already freed.
>
> To reproduce either limit has to be set low to cause freeing of
> segs or reorders have to happen (due to delay jitter).
>
> Note that we only have to mark the first segment as not on the
> list, "finish_segs" handling of other frags already does that.
>
> Another caveat is that qdisc_drop_all() still has to free all
> segments correctly in case of drop of first segment, therefore
> we re-link segs before calling it.


Acked-by: Cong Wang <xiyou.wangcong@...il.com>

Thanks for the detailed description!