* Re: [PATCH net] sch_cake: Return __NET_XMIT_STOLEN when consuming enqueued skb
2022-08-31 9:21 [PATCH net] sch_cake: Return __NET_XMIT_STOLEN when consuming enqueued skb Toke Høiland-Jørgensen
@ 2022-08-31 17:08 ` Eric Dumazet
2022-08-31 21:31 ` Toke Høiland-Jørgensen
2022-08-31 21:52 ` [PATCH net v2] sch_sfb: Don't assume the skb is still around after enqueueing to child Toke Høiland-Jørgensen
2022-08-31 22:00 ` [PATCH net] sch_cake: Return __NET_XMIT_STOLEN when consuming enqueued skb patchwork-bot+netdevbpf
2 siblings, 1 reply; 11+ messages in thread
From: Eric Dumazet @ 2022-08-31 17:08 UTC (permalink / raw)
To: Toke Høiland-Jørgensen
Cc: Jamal Hadi Salim, Cong Wang, Jiri Pirko, David S. Miller,
Jakub Kicinski, Paolo Abeni, cake, netdev
On Wed, Aug 31, 2022 at 2:25 AM Toke Høiland-Jørgensen <toke@toke.dk> 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.
>
I think you forgot to give credits to the team who discovered this issue.
Something like this
Reported-by: zdi-disclosures@trendmicro.com # ZDI-CAN-18231
> Fixes: 0c850344d388 ("sch_cake: Conditionally split GSO segments")
> Signed-off-by: Toke Høiland-Jørgensen <toke@toke.dk>
> ---
> net/sched/sch_cake.c | 4 +++-
> 1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/net/sched/sch_cake.c b/net/sched/sch_cake.c
> index a43a58a73d09..a04928082e4a 100644
> --- a/net/sched/sch_cake.c
> +++ b/net/sched/sch_cake.c
> @@ -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
>
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH net] sch_cake: Return __NET_XMIT_STOLEN when consuming enqueued skb
2022-08-31 17:08 ` Eric Dumazet
@ 2022-08-31 21:31 ` Toke Høiland-Jørgensen
0 siblings, 0 replies; 11+ messages in thread
From: Toke Høiland-Jørgensen @ 2022-08-31 21:31 UTC (permalink / raw)
To: Eric Dumazet
Cc: Jamal Hadi Salim, Cong Wang, Jiri Pirko, David S. Miller,
Jakub Kicinski, Paolo Abeni, cake, netdev
Eric Dumazet <edumazet@google.com> writes:
> On Wed, Aug 31, 2022 at 2:25 AM Toke Høiland-Jørgensen <toke@toke.dk> 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.
>>
>
> I think you forgot to give credits to the team who discovered this issue.
>
> Something like this
>
> Reported-by: zdi-disclosures@trendmicro.com # ZDI-CAN-18231
Ah, right; apologies, will respin!
It also looks like fixing it this way will actually break other things
(most notably sch_cake as a child of sch_htb), so will send a different
patch as v2...
-Toke
^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH net v2] sch_sfb: Don't assume the skb is still around after enqueueing to child
2022-08-31 9:21 [PATCH net] sch_cake: Return __NET_XMIT_STOLEN when consuming enqueued skb Toke Høiland-Jørgensen
2022-08-31 17:08 ` Eric Dumazet
@ 2022-08-31 21:52 ` Toke Høiland-Jørgensen
2022-09-02 11:30 ` patchwork-bot+netdevbpf
2022-09-05 17:55 ` Cong Wang
2022-08-31 22:00 ` [PATCH net] sch_cake: Return __NET_XMIT_STOLEN when consuming enqueued skb patchwork-bot+netdevbpf
2 siblings, 2 replies; 11+ messages in thread
From: Toke Høiland-Jørgensen @ 2022-08-31 21:52 UTC (permalink / raw)
To: Jamal Hadi Salim, Cong Wang, Jiri Pirko, Eric Dumazet, David S. Miller
Cc: Toke Høiland-Jørgensen, zdi-disclosures, Eric Dumazet,
Jakub Kicinski, Paolo Abeni, netdev
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: zdi-disclosures@trendmicro.com # ZDI-CAN-18231
Fixes: e13e02a3c68d ("net_sched: SFB flow scheduler")
Signed-off-by: Toke Høiland-Jørgensen <toke@toke.dk>
---
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(-)
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);
--
2.37.2
^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH net v2] sch_sfb: Don't assume the skb is still around after enqueueing to child
2022-08-31 21:52 ` [PATCH net v2] sch_sfb: Don't assume the skb is still around after enqueueing to child Toke Høiland-Jørgensen
@ 2022-09-02 11:30 ` patchwork-bot+netdevbpf
2022-09-05 17:55 ` Cong Wang
1 sibling, 0 replies; 11+ messages in thread
From: patchwork-bot+netdevbpf @ 2022-09-02 11:30 UTC (permalink / raw)
To: =?utf-8?b?VG9rZSBIw7hpbGFuZC1Kw7hyZ2Vuc2VuIDx0b2tlQHRva2UuZGs+?=
Cc: jhs, xiyou.wangcong, jiri, eric.dumazet, davem, zdi-disclosures,
edumazet, kuba, pabeni, netdev
Hello:
This patch was applied to netdev/net.git (master)
by David S. Miller <davem@davemloft.net>:
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!
--
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH net v2] sch_sfb: Don't assume the skb is still around after enqueueing to child
2022-08-31 21:52 ` [PATCH net v2] sch_sfb: Don't assume the skb is still around after enqueueing to child Toke Høiland-Jørgensen
2022-09-02 11:30 ` patchwork-bot+netdevbpf
@ 2022-09-05 17:55 ` Cong Wang
2022-09-05 19:05 ` Toke Høiland-Jørgensen
1 sibling, 1 reply; 11+ messages in thread
From: Cong Wang @ 2022-09-05 17:55 UTC (permalink / raw)
To: Toke Høiland-Jørgensen
Cc: Jamal Hadi Salim, Jiri Pirko, Eric Dumazet, David S. Miller,
zdi-disclosures, Eric Dumazet, Jakub Kicinski, Paolo Abeni,
netdev
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.
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH net v2] sch_sfb: Don't assume the skb is still around after enqueueing to child
2022-09-05 17:55 ` Cong Wang
@ 2022-09-05 19:05 ` Toke Høiland-Jørgensen
0 siblings, 0 replies; 11+ messages in thread
From: Toke Høiland-Jørgensen @ 2022-09-05 19:05 UTC (permalink / raw)
To: Cong Wang
Cc: Jamal Hadi Salim, Jiri Pirko, Eric Dumazet, David S. Miller,
zdi-disclosures, Eric Dumazet, Jakub Kicinski, Paolo Abeni,
netdev
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
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH net] sch_cake: Return __NET_XMIT_STOLEN when consuming enqueued skb
2022-08-31 9:21 [PATCH net] sch_cake: Return __NET_XMIT_STOLEN when consuming enqueued skb Toke Høiland-Jørgensen
2022-08-31 17:08 ` Eric Dumazet
2022-08-31 21:52 ` [PATCH net v2] sch_sfb: Don't assume the skb is still around after enqueueing to child Toke Høiland-Jørgensen
@ 2022-08-31 22:00 ` patchwork-bot+netdevbpf
2022-08-31 22:13 ` Toke Høiland-Jørgensen
2 siblings, 1 reply; 11+ messages in thread
From: patchwork-bot+netdevbpf @ 2022-08-31 22:00 UTC (permalink / raw)
To: =?utf-8?b?VG9rZSBIw7hpbGFuZC1Kw7hyZ2Vuc2VuIDx0b2tlQHRva2UuZGs+?=
Cc: jhs, xiyou.wangcong, jiri, davem, edumazet, kuba, pabeni, cake, netdev
Hello:
This patch was applied to netdev/net.git (master)
by Jakub Kicinski <kuba@kernel.org>:
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.
>
> [...]
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
You are awesome, thank you!
--
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH net] sch_cake: Return __NET_XMIT_STOLEN when consuming enqueued skb
2022-08-31 22:00 ` [PATCH net] sch_cake: Return __NET_XMIT_STOLEN when consuming enqueued skb patchwork-bot+netdevbpf
@ 2022-08-31 22:13 ` Toke Høiland-Jørgensen
2022-09-01 3:07 ` Jakub Kicinski
0 siblings, 1 reply; 11+ messages in thread
From: Toke Høiland-Jørgensen @ 2022-08-31 22:13 UTC (permalink / raw)
To: Jakub Kicinski
Cc: jhs, xiyou.wangcong, jiri, davem, edumazet, kuba, pabeni, cake, netdev
patchwork-bot+netdevbpf@kernel.org writes:
> Hello:
>
> This patch was applied to netdev/net.git (master)
> by Jakub Kicinski <kuba@kernel.org>:
>
> 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.
>>
>> [...]
>
> 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?
-Toke
[0] https://lore.kernel.org/r/20220831215219.499563-1-toke@toke.dk
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH net] sch_cake: Return __NET_XMIT_STOLEN when consuming enqueued skb
2022-08-31 22:13 ` Toke Høiland-Jørgensen
@ 2022-09-01 3:07 ` Jakub Kicinski
2022-09-01 9:20 ` Toke Høiland-Jørgensen
0 siblings, 1 reply; 11+ messages in thread
From: Jakub Kicinski @ 2022-09-01 3:07 UTC (permalink / raw)
To: Toke Høiland-Jørgensen
Cc: jhs, xiyou.wangcong, jiri, davem, edumazet, pabeni, cake, netdev
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
> 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.
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH net] sch_cake: Return __NET_XMIT_STOLEN when consuming enqueued skb
2022-09-01 3:07 ` Jakub Kicinski
@ 2022-09-01 9:20 ` Toke Høiland-Jørgensen
0 siblings, 0 replies; 11+ messages in thread
From: Toke Høiland-Jørgensen @ 2022-09-01 9:20 UTC (permalink / raw)
To: Jakub Kicinski
Cc: jhs, xiyou.wangcong, jiri, davem, edumazet, pabeni, cake, netdev
Jakub Kicinski <kuba@kernel.org> writes:
> 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
Haha, OK, no worries :)
>> 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.
SGTM!
-Toke
^ permalink raw reply [flat|nested] 11+ messages in thread