Re: [PATCH v2] can: bcm: prevent thrtimer UAF in rx path by checking RX_NO_AUTOTIMER

[Lee Jones <lee@kernel.org>]

On Thu, 15 Oct 2026, Oliver Hartkopp wrote:

> Answering myself:
> 
> This patch from Lee with READ_ONCE() and WRITE_ONCE() had been completely
> reworked to use a workqueue:
> 
> "can: bcm: defer rx_op deallocation to workqueue to fix thrtimer UAF"
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=68973f9db76144825e4f35dfdc80fb8279eb2d57
> 
> which is also from Lee and upstream as said.
> 
> So THIS patch here is obsolete.

Thanks for the clarifications Oliver.

> p.s. And the READ_ONCE() and WRITE_ONCE() in this patch also became
> obsolete. It would have no effect for race conditions only for KCSAN
> annotations - which we do not have. So we are completely done.
> 
> 
> On 15.10.26 10:03, Oliver Hartkopp wrote:
> > 
> > 
> > On 15.10.26 09:34, Lee Jones wrote:
> > > On Thu, 15 Oct 2026, Marc Kleine-Budde wrote:
> > > 
> > > > On 19.05.2026 13:38:05, Oliver Hartkopp wrote:
> > > > > From: Lee Jones <lee@kernel.org>
> > > > > 
> > > > > Commit f1b4e32aca08 ("can: bcm: use call_rcu() instead of costly
> > > > > synchronize_rcu()") removed the synchronize_rcu() call from
> > > > > bcm_delete_rx_op() and introduced the RX_NO_AUTOTIMER flag to prevent
> > > > > timers from being rearmed during deletion.  However, it only applied
> > > > > this check to op->timer via bcm_rx_starttimer().
> > > > > 
> > > > > It missed the fact that op->thrtimer can also be rearmed by an
> > > > > in-flight bcm_rx_handler() (which runs as an RCU reader) via
> > > > > bcm_rx_update_and_send().  This allows op->thrtimer to be queued after
> > > > > bcm_remove_op() has already cancelled it, leading to a use-after-free
> > > > > when the timer fires on the deferred-freed struct bcm_op.
> > > > > 
> > > > > Address the omission by checking the RX_NO_AUTOTIMER flag
> > > > > in bcm_rx_update_and_send() before starting op->thrtimer, effectively
> > > > > preventing it from being rearmed concurrently with teardown.
> > > > > 
> > > > > [Hartkopp] Added the setting of RX_NO_AUTOTIMER also to
> > > > > bcm_release() before
> > > > > removing the CAN filters following the bcm_delete_rx_op() approach.
> > > > > 
> > > > > Additionally WRITE_ONCE()/READ_ONCE() macros have been introduced for
> > > > > the changes of RX_NO_AUTOTIMER at rx op removal time to prevent a
> > > > > potential code reordering of RX_NO_AUTOTIMER setting after
> > > > > CAN filter removal.
> > > > > 
> > > > > Signed-off-by: Lee Jones <lee@kernel.org>
> > > > > Co-developed-by: Oliver Hartkopp <socketcan@hartkopp.net>
> > > > > Fixes: f1b4e32aca08 ("can: bcm: use call_rcu() instead of
> > > > > costly synchronize_rcu()")
> > > > > Signed-off-by: Oliver Hartkopp <socketcan@hartkopp.net>
> > > > 
> > > > This patch is obsolete, right?
> > > 
> > > No, I don't think so.  I thought it was applied to the maintainer tree?
> > > 
> > 
> > I think it became obsolete through all the recent (and stable) changes
> > that addressed multiple races. The UAF problem is solved today.
> > 
> > Btw. the use of READ_ONCE() and WRITE_ONCE() can be seen as some
> > addditional hardening but the entire patch does not fit to the current
> > bcm.c code.
> > 
> > The READ_ONCE() and WRITE_ONCE() may be added for the 7.5 net-next
> > window cycle. No pressure here.
> > 
> > Best regards,
> > Oliver
> > 
> > 
> 

-- 
Lee Jones