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

[Paolo Abeni <pabeni@redhat.com>]

On 6/30/26 1:49 PM, Jamal Hadi Salim wrote:
> On Tue, Jun 30, 2026 at 7:15 AM Paolo Abeni <pabeni@redhat.com> wrote:
>> On 6/28/26 1:12 PM, Jamal Hadi Salim 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_master_xmit+0xf0f/0x16b0
>>> Read of size 8 at addr ffff888013fb0440 by task poc/332
>>> Freed 512-byte region [ffff888013fb0400, ffff888013fb0600) (kmalloc-512)
>>>
>>> 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.
>>>
>>> Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
>>> Reported-by: zdi-disclosures@trendmicro.com
>>> Tested-by: Victor Nogueira <victor@mojatatu.com>
>>> Signed-off-by: Jamal Hadi Salim <jhs@mojatatu.com>
>>
>> Looks good, thanks!
>>
>> Please note that sashiko/gemini found a pre-existing issues which may
>> require a follow-up/separate fix:
>>
>> https://sashiko.dev/#/patchset/20260628111229.669751-1-jhs%40mojatatu.com
>>
>> (the 2nd one in the above link, IDK how to generate a direct link to a
>> specific comment)
> 
> I just sent v4 which covered that but i will send a followup instead
> if you already applied.

The PW bot is went on vacation and no 'patch applied' notification is
reaching the ML; v3 is already applied.

> BTW: What is the ruling on when Sashiko finds a pre-existing issue?
> Should we address that as a separate follow-up patch? It is unclear
> what the policy is.

The general guidance is that pre-existing issues should be addressed
separately.

> This teql patch was one of the hardest to deal with in terms of
> reproduciability and the fact sashiko kept coming up with pre-existing
> issues - including the one Simon and I were discussing. Note: None of
> the pre-existing issues affected reproducibility at all although i am
> sure one of the AI-kiddies reading the sashiko reports will find a way
> to create a poc (this is why i entertain fixing them when they look
> simple enough)
Not an ideal situation both ways (which is increasingly the case).

Addressing incrementally pre-existing issues can lead to an huge/endless
number of iterations when touching some unfortunate area (4 is _not_ a
big number ;) delaying the actual fix indefinitely.

/P