Re: [PATCH net v2 1/1] net/sched: sch_teql: Introduce slaves_lock to avoid race condition and UAF

[Simon Horman <horms@kernel.org>]

On Fri, Jun 26, 2026 at 12:11:47PM -0400, Jamal Hadi Salim wrote:
> Hi Simon,
> 
> On Fri, Jun 26, 2026 at 10:15 AM Simon Horman <horms@kernel.org> wrote:
> >
> > On Fri, Jun 26, 2026 at 06:16:43AM -0400, Jamal Hadi Salim wrote:
> > > "
> > >
> > > On Wed, Jun 24, 2026 at 6:40 PM Jamal Hadi Salim <jhs@mojatatu.com> wrote:
> > > >
> > > > The teql master->slaves singly linked list is not protected against
> > > > multiple writes. It can be mod'ed concurently from teql_master_xmit(),
> > > > teql_dequeue(), teql_init() and teql_destroy() without holding any list
> > > > lock or RCU protection.
> > > >
> > > > zdi-disclosures@trendmicro.com has demonstrated that the qdisc is freed
> > > > after an RCU grace period, but teql_master_xmit() running on another
> > > > CPU can still hold a stale pointer into the list, resulting in a
> > > > slab-use-after-free:
> > > >
> > > > BUG: KASAN: slab-use-after-free in teql_destroy+0x3ca/0x440 linux/net/sched/sch_teql.c:142
> > > > Read of size 8 at addr ffff88802923aa80 by task ip/10024
> > > >
> > > > The zdi-disclosures@trendmicro.com repro created concurrent AF_PACKET
> > > > senders on a teql device against a thread that repeatedly adds/deletes the
> > > > slave qdisc, together with a SLUB spray that reclaims the freed slot; the
> > > > resulting UAF is controllable enough to be turned into a read/write
> > > > primitive against the freed qdisc object.
> > > >
> > > > The fix?
> > > > Add a per-master slaves_lock spinlock that serializes all mutations of
> > > > master->slaves and the NEXT_SLAVE() links in teql_destroy() and
> > > > teql_qdisc_init(). teql_master_xmit() also takes the same slaves_lock
> > > > around those updates.
> > > > Annotate master->slaves and the per-slave ->next pointer with __rcu and
> > > > use the appropriate RCU accessors everywhere they are touched:
> > > > rcu_assign_pointer() on the writer side (under slaves_lock),
> > > > rcu_dereference_protected() for the writer-side loads (also under
> > > > slaves_lock), rcu_dereference_bh() for the loads in teql_master_xmit() and
> > > > rtnl_dereference() for the loads in teql_master_open()/teql_master_mtu(),
> > > > which run under RTNL.
> > > > Pair this with rcu_read_lock_bh()/rcu_read_unlock_bh() around the list
> > > > traversal in teql_master_xmit(), so that readers either observe a fully
> > > > linked list or are deferred until the in-flight mutation completes. The two
> > > > early-return paths in teql_master_xmit() are updated to release the RCU-bh
> > > > read-side critical section before returning, since leaving it held would
> > > > disable BH on that CPU for good.
> > > >
> > >
> > > sashiko-gemini's complaints:
> > > https://sashiko.dev/#/patchset/20260624224016.24018-1-jhs%40mojatatu.com
> > > seem bogus to me (someone correct me if i am wrong). I am only going
> > > to address the first claim of "TOCTOU / "resurrection" race in
> > > teql_master_xmit()"
> > > teql_master_xmit() holds rcu_read_lock_bh() across the entire
> > > traversal. teql_destroy() freeing can only proceed once the qdisc's
> > > RCU grace period has elapsed - so where is this TOCTOU? Let's say this
> > > were true: both calls hold the slaves_lock.
> > > The other issues are of similar nature.
> >
> > Hi Jamal,
> >
> > I think the central question here is about the protection offered by RCU
> > in these code paths. And while I agree it protects the use of elements
> > of the list, I think the problem flagged relates to the management of
> > the list itself.
> >
> > The example AI gave me when I asked is like this:
> >
> >     Assume a TEQL master has one slave, `q`.
> >     The list is circular: `q->next == q`.
> >
> >     1. CPU A (Transmitting): Enters `teql_master_xmit()`.
> >        It reads `master->sla ves` and gets a local pointer to `q`.
> >
> >     2.  CPU B (Destroying): Calls `teql_destroy(q)`.
> >         It takes the lock, unlinks `q`, and sets `master->slaves = NULL`.
> >         The list is now logically empty.
> >
> >     3.  CPU A: Finishes its work and prepares to rotate the list head
> >         to the next slave.
> >         It takes the lock.
> >
> >     4.  CPU A (The "Use" / The Resurrection):
> >         It executes: `rcu_assign_pointer(master->slaves, NEXT_SLAVE(q));`
> >         Because `q` was circular, `NEXT_SLAVE(q)` is still `q`.
> >
> >     5.  CPU A: Releases the lock.
> >         **The global `master->slaves` is now `q` again.**
> >
> >     6.  The System: The RCU grace period expires. CPU B finishes
> >         `teql_destroy()` and the memory for `q` is freed.
> >
> >     The global `master->slaves` pointer is now a **dangling pointer**
> >     pointing to freed memory.
> >
> 
> 
> Yeah, thats the same earlier claim of TOCTOU (what sashiko-gemini
> claimed was "resurrecting the freed q")
> My view is rcu read lock blocks the subsequent call_rcu free - and
> destroy() and xmit() already serialize on slaves_lock.

The read of master->slaves is outside of the slaves_lock critical
section(s) in teql_master_xmit(). This is possibly the nub of this issue.

> I could be totaly wrong, but it's almost like sashiko-gemini thinks
> that the list-mutation lock _alone_ governs the object lifetime.
> The rcu read-side critical section prevents the UAF, not just the
> slaves_lock alone
> Only reason i added slaves_lock was to prevent corrupting the list
> state (whereas the RCU read lock prevents premature free).
> 
> In step #4 above this thing somehow leaves out any mention of the rcu
> read lock entirely and places the free in step 6 as if it was
> independent of CPU A's critical section.

I see what you are saying regarding the free not occurring at step 4
because CPU A is in an RCU read-side critical section.

But once CPU A has assigned master->slaves as q (again) it exits
the RCU read-side critical section. Then the free of q can occur.
And master->slaves will point to memory that has been been freed.

So the access to q is safe when teql_master_xmit is invoked, due to RCU
protecting object lifetime.  But it is unsafe when teql_master_xmit is
invoked again because by then master->slaves is a dangling pointer.

> 
> I am not sure how to improve it.
> 
> > > OTOH, sashiko-claude
> > > (https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260624224016.24018-1-jhs%40mojatatu.com)
> > > does make some valid claims which are low value, so not sure a resend
> > > is worth it.
> > > For example in claim 1 it says "Should the changelog mention this
> > > teql_dequeue() site too?" Sure I can - but just because I provided
> > > extra information in the commit log, which I could have omitted, now I
> > > have to add more info? ;->
> >
> > FWIIW, I think there is a value in tightening up the commit message.
> > E.g. so it's accurate when we look at again in two years time.
> > But I also lean towards it not being necessary to post an update
> > only to address this.
> >
> >
> > > The second claim is "rcu_dereference_bh()
> > > should be rcu_dereference_protected() on writer side". Sparse didnt
> > > complain and i dont see this as breakage rather a consistency measure.
> >
> > I think it would be good to address in the long run.  But as per my comment
> > immediately above, I also lean towards it not being necessary to post an
> > update only to address this.
> 
> I can resend with these two taken care of - but i am skeptical of what
> sashiko-gemini is claiming (and i admit as a human the AI may see
> something i am totally missing).
> 
> cheers,
> jamal
> >
> > > Unless I am missing something ..
> > >
> > > cheers,
> > > jamal