netdev - Re: [PATCH net v2] sch_sfb: Don't assume the skb is still around after enqueueing to child

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: <YxY4DR8hoMUDgpxu@pop-os.localdomain>
Date:   Mon, 5 Sep 2022 10:55:25 -0700
From:   Cong Wang <xiyou.wangcong@...il.com>
To:     Toke Høiland-Jørgensen <toke@...e.dk>
Cc:     Jamal Hadi Salim <jhs@...atatu.com>, Jiri Pirko <jiri@...nulli.us>,
        Eric Dumazet <eric.dumazet@...il.com>,
        "David S. Miller" <davem@...emloft.net>,
        zdi-disclosures@...ndmicro.com, Eric Dumazet <edumazet@...gle.com>,
        Jakub Kicinski <kuba@...nel.org>,
        Paolo Abeni <pabeni@...hat.com>, netdev@...r.kernel.org
Subject: Re: [PATCH net v2] sch_sfb: Don't assume the skb is still around
 after enqueueing to child

On Wed, Aug 31, 2022 at 11:52:18PM +0200, Toke Høiland-Jørgensen wrote:
> The sch_sfb enqueue() routine assumes the skb is still alive after it has
> been enqueued into a child qdisc, using the data in the skb cb field in the
> increment_qlen() routine after enqueue. However, the skb may in fact have
> been freed, causing a use-after-free in this case. In particular, this
> happens if sch_cake is used as a child of sfb, and the GSO splitting mode
> of CAKE is enabled (in which case the skb will be split into segments and
> the original skb freed).
> 
> Fix this by copying the sfb cb data to the stack before enqueueing the skb,
> and using this stack copy in increment_qlen() instead of the skb pointer
> itself.
> 

I am not sure if I understand this correctly, but clearly there is
another use of skb right before increment_qlen()... See line 406 below:

402 enqueue:
403         memcpy(&cb, sfb_skb_cb(skb), sizeof(cb));
404         ret = qdisc_enqueue(skb, child, to_free);
405         if (likely(ret == NET_XMIT_SUCCESS)) {
406                 qdisc_qstats_backlog_inc(sch, skb);  // <== HERE
407                 sch->q.qlen++;
408                 increment_qlen(&cb, q);

It also uses skb->cb actually... You probably want to save qdisc_pkt_len(skb)
too.

Thanks.