netdev - Re: [RFC PATCH net-next v3] net/sched: Introduce qdisc quirk

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] [day] [month] [year] [list]

Message-ID: <aSih15MrrRud2qho@pop-os.localdomain>
Date: Thu, 27 Nov 2025 11:09:11 -0800
From: Cong Wang <xiyou.wangcong@...il.com>
To: Victor Nogueira <victor@...atatu.com>
Cc: davem@...emloft.net, kuba@...nel.org, edumazet@...gle.com,
	pabeni@...hat.com, jhs@...atatu.com, jiri@...nulli.us,
	horms@...nel.org, stephen@...workplumber.org,
	netdev@...r.kernel.org
Subject: Re: [RFC PATCH net-next v3] net/sched: Introduce qdisc quirk_chk op

On Thu, Nov 27, 2025 at 01:49:35PM -0300, Victor Nogueira wrote:
> There is a pattern of bugs that end up creating UAFs or null ptr derefs.
> The majority of these bugs follow the formula below:
> a) create a nonsense hierarchy of qdiscs which has no practical value,
> b) start sending packets
> Optional c) netlink cmds to change hierarchy some more; It's more fun if
> you can get packets stuck - the formula in this case includes non
> work-conserving qdiscs somewhere in the hierarchy
> Optional d dependent on c) send more packets
> e) profit
> 
> Current init/change qdisc APIs are localised to validate only within the
> constraint of a single qdisc. So catching #a or #c is a challenge. Our
> policy, when said bugs are presented, is to "make it work" by modifying
> generally used data structures and code, but these come at the expense of
> adding special checks for corner cases which are nonsensical to begin with.
> 
> The goal of this patchset is to create an equivalent to PCI quirks, which
> will catch nonsensical hierarchies in #a and #c and reject such a config.
> 
> With that in mind, we are proposing the addition of a new qdisc op
> (quirk_chk). We introduce, as a first example, the quirk_chk op to netem.
> Its purpose here is to validate whether the user is attempting to add
> nested netem duplicates in the same qdisc tree branch which will be
> forbidden.

As I already explained in your v2, this does not justify the ugliness of
the code. It was wrong to introduce the ugly checks from the very
beginning, therefore, it makes no sense to keep adding more ugly code to
fix it. Reverting it is the best option, as far as I can see.

> 
> Here is an example that should now work:

Oh, I am glad to see you and your boss finally acknowledge breaking
users. :)

> 
> DEV="eth0"
> NUM_QUEUES=4
> DUPLICATE_PERCENT="5%"
> 
> tc qdisc del dev $DEV root > /dev/null 2>&1
> tc qdisc add dev $DEV root handle 1: mq
> 
> for i in $(seq 1 $NUM_QUEUES); do
>     HANDLE_ID=$((i * 10))
>     PARENT_ID="1:$i"
>     tc qdisc add dev $DEV parent $PARENT_ID handle \
>         ${HANDLE_ID}: netem duplicate $DUPLICATE_PERCENT
> done

Looks identical to Ji'Soo's report:
https://bugzilla.kernel.org/show_bug.cgi?id=220774#c0

without crediting Ji-Soo... What's wrong with crediting reporters fairly?

> 
> Suggested-by: Jamal Hadi Salim <jhs@...atatu.com>
> Signed-off-by: Victor Nogueira <victor@...atatu.com>

Nacked-by: Cong Wang <xiyou.wangcong@...il.com>

Note: My response is just for record purpose, I have to reject it for record
because I don't want to get blamed for being disrespectful to our users.
This does not aim to stop you and your boss pushing your agenda in the open
source community. And you don't need to respond against your boss' will, this is
perfectly understood. I will bring this up for other maintainers at LPC, since
I care about the open source community. Let record speak.

Hope this makes sense to you. If there is anything I could help you,
please don't hesitate to reach out to me directly at any time.

Regards,
Cong Wang