netdev - Re: [PATCH net 1/1 v3] 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: <CAM0EoMk0rKe=AqoD_vNZNj2dz9eKSQpgS0Cc7Bi+FQwqpyHXaw@mail.gmail.com>
Date: Wed, 15 Jan 2025 09:53:27 -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 v3] net: sched: Disallow replacing of child qdisc
 from one parent to another

On Wed, Jan 15, 2025 at 9:36 AM Jakub Kicinski <kuba@...nel.org> wrote:
>
> On Wed, 15 Jan 2025 09:15:31 -0500 Jamal Hadi Salim wrote:
> > > On Sat, 11 Jan 2025 10:14:55 -0500 Jamal Hadi Salim wrote:
> > > > 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".
> > >
> > > Your explanation reads like you want to prevent a qdisc changing
> > > from one parent to another.
> >
> > Yes.
>
> In the selftest with mq Victor updated I'd say we're not changing
> the parent. We replace one child of mq with another.
> TC noobs would say mq is the parent.

Yeah, Victor's test was to reduce the number of lines changed. IIRC,
there was an _child_assert which would have to change to a different
number given mq doesnt destroy the manually created qdiscs.

You can replace a queue attributes (eg queue size) or its algorithm
(example change from pfifo to bfifo) in place - that doesnt change.
What the patch avoids is taking that queue and moving it elsewhere or
having it "shared"; that puts it in the funny state that was used in
the exploit.
That was the ambiguity i was talking about in the earlier email.

> > > > +                             if (leaf_q && leaf_q->parent != q->parent) {
> > > > +                                     NL_SET_ERR_MSG(extack, "Invalid Parent for operation");
> > > > +                                     return -EINVAL;
> > > > +                             }
> > >
> > > But this test looks at the full parent path, not the major.
> > > So the only case you allow is replacing the node.. with itself?
> > >
> >
> > Yes.
> >
> > > Did you mean to wrap these in TC_H_MAJ() || the parent comparison
> > > is redundant || I misunderstand?
> >
> > I may be missing something - what does TC_H_MAJ() provide?
> > The 3:1 and 1:3 in that example are both descendants of the same
> > parent. It could have been 1:3 vs 1:2 and the same rules would apply.
>
> Let me flip the question. What qdisc movement / grafts are you intending
> to still support?
>

Grafting essentially boils down to a del/add of a qdisc. The
ambiguities: Does it mean deleting it from one hierachy point and
adding it to another point? Or does it mean not deleting it from the
first location but making it available in the other one?

> From the report it sounds like we don't want to support _any_ movement
> of existing qdiscs within the hierarchy. Only purpose of graft would
> be to install a new / fresh qdisc as a child.

That sounded like the safest approach. If there is a practical use for
moving queues around (I am not aware of any, doesnt mean there is no
practical use) then we can do the much bigger surgery.

cheers,
jamal