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

[Lee Jones <lee@kernel.org>]

On Wed, 20 May 2026, Lee Jones wrote:

> On Tue, 19 May 2026, 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>
> 
> You did?  Can you add a note saying what you changed please?

FYI, did you also see the second swing I took at this:

https://lore.kernel.org/r/20260520080523.2513957-1-lee@kernel.org

> > Fixes: f1b4e32aca08 ("can: bcm: use call_rcu() instead of costly synchronize_rcu()")
> > Signed-off-by: Oliver Hartkopp <socketcan@hartkopp.net>
> > ---
> >  net/can/bcm.c | 14 ++++++++++++--
> >  1 file changed, 12 insertions(+), 2 deletions(-)
> > 
> > diff --git a/net/can/bcm.c b/net/can/bcm.c
> > index a4bef2c48a55..abf7bd2c2e6f 100644
> > --- a/net/can/bcm.c
> > +++ b/net/can/bcm.c
> > @@ -537,10 +537,16 @@ static void bcm_rx_update_and_send(struct bcm_op *op,
> >  
> >  	/* with active throttling timer we are just done here */
> >  	if (hrtimer_active(&op->thrtimer))
> >  		return;
> >  
> > +	/* bcm_remove_op() may have cancelled thrtimer concurrently with this
> > +	 * RCU-protected handler; do not rearm it. Mirrors bcm_rx_starttimer().
> > +	 */
> > +	if (READ_ONCE(op->flags) & RX_NO_AUTOTIMER)
> > +		return;
> > +
> >  	/* first reception with enabled throttling mode */
> >  	if (!op->kt_lastmsg)
> >  		goto rx_changed_settime;
> >  
> >  	/* got a second frame inside a potential throttle period? */
> > @@ -603,11 +609,11 @@ static void bcm_rx_cmp_to_index(struct bcm_op *op, unsigned int index,
> >  /*
> >   * bcm_rx_starttimer - enable timeout monitoring for CAN frame reception
> >   */
> >  static void bcm_rx_starttimer(struct bcm_op *op)
> >  {
> > -	if (op->flags & RX_NO_AUTOTIMER)
> > +	if (READ_ONCE(op->flags) & RX_NO_AUTOTIMER)
> >  		return;
> >  
> >  	if (op->kt_ival1)
> >  		hrtimer_start(&op->timer, op->kt_ival1, HRTIMER_MODE_REL_SOFT);
> >  }
> > @@ -838,11 +844,11 @@ static int bcm_delete_rx_op(struct list_head *ops, struct bcm_msg_head *mh,
> >  	list_for_each_entry_safe(op, n, ops, list) {
> >  		if ((op->can_id == mh->can_id) && (op->ifindex == ifindex) &&
> >  		    (op->flags & CAN_FD_FRAME) == (mh->flags & CAN_FD_FRAME)) {
> >  
> >  			/* disable automatic timer on frame reception */
> > -			op->flags |= RX_NO_AUTOTIMER;
> > +			WRITE_ONCE(op->flags, op->flags | RX_NO_AUTOTIMER);
> >  
> >  			/*
> >  			 * Don't care if we're bound or not (due to netdev
> >  			 * problems) can_rx_unregister() is always a save
> >  			 * thing to do here.
> > @@ -1618,10 +1624,14 @@ static int bcm_release(struct socket *sock)
> >  
> >  	list_for_each_entry_safe(op, next, &bo->tx_ops, list)
> >  		bcm_remove_op(op);
> >  
> >  	list_for_each_entry_safe(op, next, &bo->rx_ops, list) {
> > +
> > +		/* disable automatic timer on frame reception */
> > +		WRITE_ONCE(op->flags, op->flags | RX_NO_AUTOTIMER);
> > +
> >  		/*
> >  		 * Don't care if we're bound or not (due to netdev problems)
> >  		 * can_rx_unregister() is always a save thing to do here.
> >  		 */
> >  		if (op->ifindex) {
> > -- 
> > 2.53.0
> > 
> 
> -- 
> Lee Jones

-- 
Lee Jones