Re: [PATCH v2 1/2] can: bcm: add locking when updating filter and timer values

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [High] Concurrent RX_DELETE and CAN frame reception can cause a timer to be armed after it was canceled, leading to a Use-After-Free.
--

commit a1560c18e32869b3dc94d859252cf67b537b151d
Author: Oliver Hartkopp <socketcan@hartkopp.net>

can: bcm: add locking when updating filter and timer values

KCSAN detected a simultaneous access to timer values that can be
overwritten in bcm_rx_setup when updating timer and filter content.

Fix the RX_SETUP update case by protecting the timer and filter updates
with a spin_lock_bh().

Link: https://lore.kernel.org/linux-can/6975d5cf.a00a0220.33ccc7.0022.GAE@google.com/

> diff --git a/net/can/bcm.c b/net/can/bcm.c
> index a4bef2c48a559..dbdf5d4f4ed7b 100644
> --- a/net/can/bcm.c
> +++ b/net/can/bcm.c

[ ... ]

> @@ -774,6 +788,8 @@ static void bcm_rx_handler(struct sk_buff *skb, void *data)
>  
>  rx_starttimer:
>  	bcm_rx_starttimer(op);
> +
> +	spin_unlock_bh(&op->bcm_rx_update_lock);
>  }

[Severity: High]
This is a pre-existing issue, but does this still leave a race condition
with bcm_delete_rx_op()?

Even with bcm_rx_update_lock held here, bcm_delete_rx_op() modifies
op->flags without taking bcm_rx_update_lock:

net/can/bcm.c:bcm_delete_rx_op() {
	...
	/* disable automatic timer on frame reception */
	op->flags |= RX_NO_AUTOTIMER;
	...
	bcm_remove_op(op);
}

And bcm_remove_op() cancels the timers and queues the object for RCU freeing:

net/can/bcm.c:bcm_remove_op() {
	hrtimer_cancel(&op->timer);
	hrtimer_cancel(&op->thrtimer);
	call_rcu(&op->rcu, bcm_free_op_rcu);
}

If bcm_rx_starttimer() reads the old op->flags value, can it be executed
after bcm_remove_op() has already canceled the timers? This would result in
the timer being re-armed on a soon-to-be-freed object, potentially leading
to a use-after-free when the timer fires.

Should bcm_delete_rx_op() also take bcm_rx_update_lock when updating
op->flags?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260522180758.51128-1-socketcan@hartkopp.net?part=1