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

[Cong Wang <xiyou.wangcong@gmail.com>]

On Mon, Jun 17, 2019 at 11:11 AM Jakub Kicinski
<jakub.kicinski@netronome.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@gmail.com>

Thanks for the detailed description!