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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAM0EoMn7uADZkTQkg48VP7K7KD=ZVHPLfZheAwXSumqFWommNg@mail.gmail.com>
Date: Fri, 10 Jan 2025 09:48:02 -0500
From: Jamal Hadi Salim <jhs@...atatu.com>
To: Jakub Kicinski <kuba@...nel.org>
Cc: netdev@...r.kernel.org, jiri@...nulli.us, xiyou.wangcong@...il.com, 
	davem@...emloft.net, edumazet@...gle.com, security@...nel.org, 
	nnamrec@...il.com
Subject: Re: [PATCH net 1/1 v2] net: sched: Disallow replacing of child qdisc
 from one parent to another

On Thu, Jan 9, 2025 at 1:29 PM Jakub Kicinski <kuba@...nel.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..