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

Commit Message

Toke Høiland-Jørgensen Aug. 31, 2022, 9:52 p.m. UTC

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.

Reported-by: [e-mail] # ZDI-CAN-18231
Fixes: e13e02a3c68d ("net_sched: SFB flow scheduler")
Signed-off-by: Toke Høiland-Jørgensen <[e-mail]>
---
v2:
- Instead of changing sch_cake to return NET_XMIT_SUCCESS | __NET_XMIT_STOLEN
  when freeing the skb, change sfb to not assume the skb is still alive after
  enqueue (which no other callers of qdisc_enqueue() do). This has the benefit
  of not breaking the usage of sch_cake as a child of sch_htb, which is a
  deployment seen in real-world use.

 net/sched/sch_sfb.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

Comments

[e-mail] Sept. 2, 2022, 11:30 a.m. UTC | #1

Hello:

This patch was applied to netdev/net.git (master)
by David S. Miller <[e-mail]>:

On Wed, 31 Aug 2022 23:52:18 +0200 you 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).
> 
> [...]

Here is the summary with links:
  - [net,v2] sch_sfb: Don't assume the skb is still around after enqueueing to child
    https://git.kernel.org/netdev/net/c/9efd23297cca

You are awesome, thank you!

Cong Wang Sept. 5, 2022, 5:55 p.m. UTC | #2

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.

Toke Høiland-Jørgensen Sept. 5, 2022, 7:05 p.m. UTC | #3

Cong Wang <[e-mail]> 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

Patch

diff --git a/net/sched/sch_sfb.c b/net/sched/sch_sfb.c
index 3d061a13d7ed..0d761f454ae8 100644
--- a/net/sched/sch_sfb.c
+++ b/net/sched/sch_sfb.c
@@ -135,15 +135,15 @@  static void increment_one_qlen(u32 sfbhash, u32 slot, struct sfb_sched_data *q)
 	}
 }
 
-static void increment_qlen(const struct sk_buff *skb, struct sfb_sched_data *q)
+static void increment_qlen(const struct sfb_skb_cb *cb, struct sfb_sched_data *q)
 {
 	u32 sfbhash;
 
-	sfbhash = sfb_hash(skb, 0);
+	sfbhash = cb->hashes[0];
 	if (sfbhash)
 		increment_one_qlen(sfbhash, 0, q);
 
-	sfbhash = sfb_hash(skb, 1);
+	sfbhash = cb->hashes[1];
 	if (sfbhash)
 		increment_one_qlen(sfbhash, 1, q);
 }
@@ -283,6 +283,7 @@  static int sfb_enqueue(struct sk_buff *skb, struct Qdisc *sch,
 	struct sfb_sched_data *q = qdisc_priv(sch);
 	struct Qdisc *child = q->qdisc;
 	struct tcf_proto *fl;
+	struct sfb_skb_cb cb;
 	int i;
 	u32 p_min = ~0;
 	u32 minqlen = ~0;
@@ -399,11 +400,12 @@  static int sfb_enqueue(struct sk_buff *skb, struct Qdisc *sch,
 	}
 
 enqueue:
+	memcpy(&cb, sfb_skb_cb(skb), sizeof(cb));
 	ret = qdisc_enqueue(skb, child, to_free);
 	if (likely(ret == NET_XMIT_SUCCESS)) {
 		qdisc_qstats_backlog_inc(sch, skb);
 		sch->q.qlen++;
-		increment_qlen(skb, q);
+		increment_qlen(&cb, q);
 	} else if (net_xmit_drop_count(ret)) {
 		q->stats.childdrop++;
 		qdisc_qstats_drop(sch);