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

["Toke Høiland-Jørgensen" <toke@toke.dk>]

Cong Wang <xiyou.wangcong@gmail.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