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

[Lee Jones <lee@kernel.org>]

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?

> 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