[PATCH] net/sched: sch_cake: fix NAT destination port not being updated in cake_update_flowkeys

[PATCH] net/sched: sch_cake: fix NAT destination port not being updated in cake_update_flowkeys

[Dudu Lu]

cake_update_flowkeys() is supposed to update the flow dissector keys
with the NAT-translated addresses and ports from conntrack, so that
CAKE's per-flow fairness correctly identifies post-NAT flows as
belonging to the same connection.

For the source port, this works correctly:
    keys->ports.src = port;  /* writes conntrack port into keys */

But for the destination port, the assignment is reversed:
    port = keys->ports.dst;  /* reads FROM keys into local var — no-op */

This means the NAT destination port is never updated in the flow keys.
As a result, when multiple connections are NATed to the same destination
(same IP + same port), CAKE treats them as separate flows because the
original (pre-NAT) destination ports differ. This completely defeats
CAKE's NAT-aware flow isolation when using the "nat" mode.

The vulnerability was introduced in commit b0c19ed6088a ("sch_cake: Take advantage
of skb->hash where appropriate") which refactored the original direct
assignment into a compare-and-conditionally-update pattern, but wrote
the destination port update backwards.

Fix by reversing the assignment direction to match the source port
pattern.

Fixes: b0c19ed6088a ("sch_cake: Take advantage of skb->hash where appropriate")
Signed-off-by: Dudu Lu <phx0fer@gmail.com>
---
 net/sched/sch_cake.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/sched/sch_cake.c b/net/sched/sch_cake.c
index 9efe23f8371b..4ac6c36ca6e4 100644
--- a/net/sched/sch_cake.c
+++ b/net/sched/sch_cake.c
@@ -619,7 +619,7 @@ static bool cake_update_flowkeys(struct flow_keys *keys,
 		}
 		port = rev ? tuple.src.u.all : tuple.dst.u.all;
 		if (port != keys->ports.dst) {
-			port = keys->ports.dst;
+			keys->ports.dst = port;
 			upd = true;
 		}
 	}
-- 
2.39.3 (Apple Git-145)

Re: [PATCH] net/sched: sch_cake: fix NAT destination port not being updated in cake_update_flowkeys

[Toke Høiland-Jørgensen]

Dudu Lu <phx0fer@gmail.com> writes:

> cake_update_flowkeys() is supposed to update the flow dissector keys
> with the NAT-translated addresses and ports from conntrack, so that
> CAKE's per-flow fairness correctly identifies post-NAT flows as
> belonging to the same connection.
>
> For the source port, this works correctly:
>     keys->ports.src = port;  /* writes conntrack port into keys */
>
> But for the destination port, the assignment is reversed:
>     port = keys->ports.dst;  /* reads FROM keys into local var — no-op */

Huh, what a silly mistake - nice find!

> This means the NAT destination port is never updated in the flow keys.
> As a result, when multiple connections are NATed to the same destination
> (same IP + same port), CAKE treats them as separate flows because the
> original (pre-NAT) destination ports differ. This completely defeats
> CAKE's NAT-aware flow isolation when using the "nat" mode.
>
> The vulnerability was introduced in commit b0c19ed6088a ("sch_cake: Take advantage
> of skb->hash where appropriate")

Calling it a "vulnerability" seems perhaps a tad hyperbolic. Care to
elaborate on what you mean here?

-Toke

Re: [PATCH] net/sched: sch_cake: fix NAT destination port not being updated in cake_update_flowkeys

[Toke Høiland-Jørgensen]

phx <phx0fer@gmail.com> writes:

> You're right, "vulnerability" is too strong - it's a correctness
> bug, not a security issue. Thanks for picking it up.

Cool. Could you please re-send with an updated commit message? Thanks!

-Toke

pw-bot: cr