netdev - Re: [PATCH net] net/sched: sch_qfq: Fix NULL deref when deactivating

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: <mhp4ctfcrwebw57lnq66pqtqlcktzkubgt2f3ikhomk7ouyt5k@hs5qjwj4vxex>
Date: Sun, 14 Dec 2025 17:23:43 -0700
From: Xiang Mei <xmei5@....edu>
To: Jakub Kicinski <kuba@...nel.org>
Cc: Cong Wang <xiyou.wangcong@...il.com>, security@...nel.org, 
	netdev@...r.kernel.org, jhs@...atatu.com, jiri@...nulli.us
Subject: Re: [PATCH net] net/sched: sch_qfq: Fix NULL deref when deactivating

On Thu, Dec 11, 2025 at 06:23:03PM +0900, Jakub Kicinski wrote:
> On Mon, 8 Dec 2025 15:17:57 -0700 Xiang Mei wrote:
> > Sorry for not explaining that. This PoC could be complex to use TC to
> > trigger. I was thinking about the same thing: transforming this C
> > program to `tc` commands so we can have a tc-tests case, but this bug
> > is related to a race condition.
> > 
> > We have to use racing to create the state mentioned in the commit
> > message: "Two qfq_class objects may point to the same
> > leaf_qdisc"(Racing between tc_new_tfilter and qdisc_delete). I failed
> > to find a clean way to use `tc` to trigger this race after several
> > hours of trial, so I gave up on that. For non-race condition bugs,
> > I'll try to provide self-tests.
> 
> Speaking under correction here, but you can submit the test as C code
> to the tools/testing/selftests/net directory. It doesn't have to use
> existing bash tooling. It needs to follow kernel coding style as Cong
> alluded to, however. Ideally if you could rewrite the reproducer to use
> YNL C that'd be save a lot of netlink boilerplate.. I think QFQ is
> supported by YNL tho not sure if that support is sufficient.

Thanks for the tip. I agree that a C self-test case could be a better
choice. For this particular issue, writing a reliable YNL testcase is
unfortunately time-consuming. The hardest part is hitting the race
condition between qdisc_delete and tc_new_tfilter:

1. Build qfq_qdisc, qfq_class, and attach a tfilter to qfq_class
2. Thread 1: call tc_new_tfilter to have refcnt++
3. Thread 2: call qdisc_get to delete the qfq_qdisc
4. Thread 2: create a new qfq_qdisc and a new qfdq_class
5. Thread 2: enqueue a packet so we have the leaf qdisc->q.qlen++
6. Thread 1: refcnt-- in tc_new_tfilter to trigger qfq_reset

As the procedure listed above, we have 3 things (4 syscalls) to do between
step 2 and 6. Considering the time window between step 2 and 6 is not big,
we have to make these 4 syscalls faster. In the C reproducer, we can
repeatedly call these syscalls to warm up (faster syscalls). However,
using the commands makes this harder. Also, the uncertainty makes the test
case less valuable.

For the C self-test case, I am also concerned about the time cost,
considering we have to transform the C reproducer to a format that is
readable and maintainable. As a security researcher, I understand the
importance of having test cases, but for complex/hard-to-trigger bugs, the
time cost pressure could be concerning for the vulnerability
fixers/reporters.

> 
> Two more asks/questions
>  - could you please add the crash info to the commit message

Thanks for the tip. I'll add it to the next version.

>  - there is a similar code construct in qfq_deact_rm_from_agg()
>    does it also need to be fixed?

That's a good question.
The ideal fix would indeed be to consistently rely on the active_list
state rather than cl->qdisc->q.qlen for all qdiscs under net. However,
in this case it appears unnecessary.

> 
> And a reminder to not top post on the kernel mailing lists, please.
> -- 
> pw-bot: cr

Thanks for the tip.

Best,
Xiang