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] [day] [month] [year] [list]

Message-ID: <87k06hzlo4.fsf@toke.dk>
Date:   Mon, 05 Sep 2022 21:05:47 +0200
From:   Toke Høiland-Jørgensen <toke@...e.dk>
To:     Cong Wang <xiyou.wangcong@...il.com>
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

Cong Wang <xiyou.wangcong@...il.com> writes:

> 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.

Ah, oops, didn't realise qdisc_pkt_len() also used the cb field; will
send another follow-up, thanks for spotting this!

-Toke