sch_cake: Return __NET_XMIT_STOLEN when consuming enqueued skb

When the GSO splitting feature of sch_cake is enabled, GSO superpackets will be broken up and the resulting segments enqueued in place of the original skb. In this case, CAKE calls consume_skb() on the original skb, but still returns NET_XMIT_SUCCESS. This can confuse parent qdiscs into assuming the original skb still exists, when it really has been freed. Fix this by adding the __NET_XMIT_STOLEN flag to the return value in this case.

Fixes 0c850344d388 ("sch_cake: Conditionally split GSO segments")

Signed-off-by Toke Høiland-Jørgensen <[email protected]>

---
 net/sched/sch_cake.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

net/sched/sch_cake.c +3 -2

index a43a58a73d09..a04928082e4a 100644
@@ -1713,6 +1713,7 @@ static s32 cake_enqueue(struct sk_buff *skb, struct Qdisc *sch,
        }
        idx--;
        flow = &b->flows[idx];
+       ret = NET_XMIT_SUCCESS;
 
        /* ensure shaper state isn't stale */
        if (!b->tin_backlog) {

@@ -1771,6 +1772,7 @@ static s32 cake_enqueue(struct sk_buff *skb, struct Qdisc *sch,
 
                qdisc_tree_reduce_backlog(sch, 1-numsegs, len-slen);
                consume_skb(skb);
+               ret |= __NET_XMIT_STOLEN;
        } else {
                /* not splitting */
                cobalt_set_enqueue_time(skb, now);

@@ -1904,7 +1906,7 @@ static s32 cake_enqueue(struct sk_buff *skb, struct Qdisc *sch,
                }
                b->drop_overlimit += dropped;
        }
-       return NET_XMIT_SUCCESS;
+       return ret;
 }
 
 static struct sk_buff *cake_dequeue_one(struct Qdisc *sch)
--

2.37.2

Quoted 8 lines · Toke Høiland-Jørgensen

When the GSO splitting feature of sch_cake is enabled, GSO superpackets will be broken up and the resulting segments enqueued in place of the original skb. In this case, CAKE calls consume_skb() on the original skb, but still returns NET_XMIT_SUCCESS. This can confuse parent qdiscs into assuming the original skb still exists, when it really has been freed. Fix this by adding the __NET_XMIT_STOLEN flag to the return value in this case.

I think you forgot to give credits to the team who discovered this issue.

Something like this

Reported-by [email protected] # ZDI-CAN-18231

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.

Quoted 8 lines · Toke Høiland-Jørgensen

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

Quoted 4 lines

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.

Hello:

This patch was applied to netdev/net.git (master)
by Jakub Kicinski <[email protected]>:

Quoted 7 lines · On Wed, 31 Aug 2022 11:21:03 +0200 you wrote:

When the GSO splitting feature of sch_cake is enabled, GSO superpackets will be broken up and the resulting segments enqueued in place of the original skb. In this case, CAKE calls consume_skb() on the original skb, but still returns NET_XMIT_SUCCESS. This can confuse parent qdiscs into assuming the original skb still exists, when it really has been freed. Fix this by adding the __NET_XMIT_STOLEN flag to the return value in this case.

Quoted 1 line

[...]

Here is the summary with links:
- [net] sch_cake: Return __NET_XMIT_STOLEN when consuming enqueued skb

    https://git.kernel.org/netdev/net/c/90fabae8a2c2

Ah, crossed streams (just sent v2[0]).

Hmm, okay, so as noted in the changelog to v2, just this patch will break htb+cake (because htb will now skip htb_activate()); do you prefer that I send a follow-up to fix HTB in this mode, or to revert this and apply the fix to sfb in v2 instead?

Quoted 1 line · On Thu, 01 Sep 2022 00:13:24 +0200 Toke Høiland-Jørgensen wrote:

Ah, crossed streams (just sent v2[0]).

Sorry about that, traveling knocked out my sense of time and I kept thinking it's Thursday, and the discussion happened yesterday :S

Quoted 4 lines

Hmm, okay, so as noted in the changelog to v2, just this patch will break htb+cake (because htb will now skip htb_activate()); do you prefer that I send a follow-up to fix HTB in this mode, or to revert this and apply the fix to sfb in v2 instead?

Reverted. Let's review v2 as if v1 was not applied.