Re: [PATCH 6.6.y] netfilter: ctnetlink: ensure safe access to master conntrack

[Pablo Neira Ayuso]

Hi,

On Sun, Jun 14, 2026 at 10:38:16PM +0800, XIAO WU wrote:
> Hi,
> 
> Pablo Neira Ayuso <pablo@xxxxxxxxxxxxx> wrote:
> > Holding reference on the expectation is not sufficient, the master
> > conntrack object can just go away, making exp->master invalid.
> >
> > [...]
> >
> > This patch goes for extending the nf_conntrack_expect_lock section
> > to address this issue for simplicity, in the cases that are described
> > below this is just slightly extending the lock section.
> 
> I tested this patch on 6.6.142-g005cf9204c4e with KASAN + lockdep
> enabled and found that the lock extension in ctnetlink_get_expect()
> and ctnetlink_del_expect() does address the expectation-lookup paths,
> but one other path still races:
> 
>   clean_from_lists()
>     nf_ct_remove_expectations()          // holds nf_conntrack_expect_lock
>       list_for_each_entry_safe(exp, ...)
>         nf_ct_remove_expect(exp)
>           del_timer(&exp->timeout)       // returns false if timer is
>                                          // already executing on another CPU
>           // expectation NOT unlinked
>   ...
>   nf_conntrack_free(ct)                  // master is freed
> 
>   // meanwhile, on another CPU:
>   nf_ct_expectation_timed_out()          // timer callback fires
>     nf_ct_unlink_expect_report(exp, ...) // acquires
> nf_conntrack_expect_lock
>       nf_ct_expect_event_report(...)
>         e = nf_ct_ecache_find(exp->master);  // UAF -- master already freed
> 
> nf_ct_remove_expect() already has the lockdep annotation added by this
> patch:
> 
> > +    lockdep_nfct_expect_lock_held();
> > +
> >      if (del_timer(&exp->timeout)) {
> >          nf_ct_unlink_expect(exp);
> >          nf_ct_expect_put(exp);
> 
> But holding the lock is not enough: del_timer() cannot cancel a timer
> that is already running on another CPU.  When it returns false, the
> expectation stays in the hash table, the master is freed immediately
> afterward, and the in-flight timer callback hits a dangling exp->master.
> 
> The same del_timer() pattern also exists in ctnetlink_del_expect(),
> which the patch already extends:
> 
> > +        spin_lock_bh(&nf_conntrack_expect_lock);
> > +
> >          /* bump usage count to 2 */
> >          exp = nf_ct_expect_find_get(info->net, &zone, &tuple);
> > [...]
> >          /* after list removal, usage count == 1 */
> > -        spin_lock_bh(&nf_conntrack_expect_lock);
> >          if (del_timer(&exp->timeout)) {
> >              nf_ct_unlink_expect_report(exp, NETLINK_CB(skb).portid,
> >                             nlmsg_report(info->nlh));
> 
> This del_timer() call has the same vulnerability -- just holding the
> lock doesn't prevent the timer from executing concurrently before the
> lock was acquired.  The timer callback will spin-wait for the lock,
> acquire it after ctnetlink_del_expect() drops it, and then access the
> now-freed (or about-to-be-freed) master.

Thanks for your report.

I started a patch to replace the existing expectation timers by GC
workqueue to address this issue.