Re: [PATCH 3/5] can: bcm: add locking when updating filter and timer values

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] The patch addresses race conditions in the RX path but misses identical missing lock vulnerabilities and partial state corruption bugs in the TX path (`bcm_tx_setup` and `bcm_can_tx`).
- [High] Data race in `bcm_rx_setup` when clearing `CAN_RTR_FLAG` for `RX_RTR_FRAME` operations, which can lead to torn reads and CAN packet storms.
- [High] `bcm_rx_handler` reads `op->flags` without lock protection, triggering a data race with `bcm_rx_setup` and potential data corruption.
--

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

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

This commit fixes a KCSAN-detected race condition where timer values
could be overwritten simultaneously in bcm_rx_setup(). It protects the
timer and filter data updates with a new 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 9475758a6749f..fc5158132b554 100644
> --- a/net/can/bcm.c
> +++ b/net/can/bcm.c
> @@ -129,6 +129,7 @@ struct bcm_op {
>  	struct sock *sk;
>  	struct net_device *rx_reg_dev;
>  	spinlock_t bcm_tx_lock; /* protect currframe/count in runtime updates */
> +	spinlock_t bcm_rx_update_lock; /* protect filter/timer data updates */
>  };

[Severity: High]
This is a pre-existing issue, but while we are adding bcm_rx_update_lock
to protect RX filter and timer updates, does the TX path need similar
protection?

In bcm_tx_setup(), when updating an existing operation, memcpy_from_msg()
copies user data directly into op->frames without locking, and 64-bit timer
values like op->kt_ival1 are updated locklessly.

Concurrently, if bcm_tx_timeout_handler() fires, it can read these torn
timer values. Also, bcm_can_tx() reads the frame payload from cf locklessly
via skb_put_data(), which races with memcpy_from_msg().

Furthermore, if bcm_tx_setup() encounters a validation error
(e.g., cf->len > 64) after memcpy_from_msg(), it returns -EINVAL without
rolling back. The active timer will continue firing, reading the malicious
length and allocating a normal-sized skb, potentially leading to an
out-of-bounds read in the CAN driver.

[ ... ]

> @@ -743,6 +755,8 @@ static void bcm_rx_handler(struct sk_buff *skb, void *data)
>  			traffic_flags |= RX_OWN;
>  	}
>  
> +	spin_lock_bh(&op->bcm_rx_update_lock);
> +
>  	if (op->flags & RX_FILTER_ID) {

[Severity: High]
This is a pre-existing issue, but does bcm_rx_handler() read op->flags
without lock protection just before this new lock is acquired?

If a user updates an existing operation to toggle RX_RTR_FRAME, bcm_rx_setup()
updates op->flags and op->frames under the bcm_rx_update_lock.

However, earlier in bcm_rx_handler(), it checks op->flags locklessly:

	if (op->flags & RX_RTR_FRAME) {
		/* send reply for RTR-request (placed in op->frames[0]) */
		bcm_can_tx(op);
		return;
	}

Could bcm_rx_handler() observe the newly written op->flags and immediately
call bcm_can_tx(), which will then read a stale or partially written
op->frames[0] to transmit? This seems to defeat the atomicity provided by
the newly introduced lock.

[ ... ]

> @@ -1277,10 +1315,13 @@ static int bcm_rx_setup(struct bcm_msg_head *msg_head, struct msghdr *msg,
>  		if (op->flags & SETTIMER) {
>  
>  			/* set timer value */
> +			spin_lock_bh(&op->bcm_rx_update_lock);

[Severity: High]
This is a pre-existing issue, but is there a data race when clearing
CAN_RTR_FLAG for RX_RTR_FRAME operations?

In bcm_rx_setup(), just above this block, it handles RX_RTR_FRAME:

	if (op->flags & RX_RTR_FRAME) {
		struct canfd_frame *frame0 = op->frames;
		...
		if ((op->flags & TX_CP_CAN_ID) ||
		    (frame0->can_id == op->can_id))
			frame0->can_id = op->can_id & ~CAN_RTR_FLAG;
	} else {
		if (op->flags & SETTIMER) {

The frame0->can_id modification happens after bcm_rx_update_lock is already
unlocked.

Concurrently, bcm_rx_handler() checks op->flags & RX_RTR_FRAME and calls
bcm_can_tx(), which reads op->frames to send a reply. Could bcm_can_tx()
transmit the frame with CAN_RTR_FLAG still set before it is cleared here,
potentially triggering an infinite loopback storm?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260612-bcm_fixes-v1-0-ca2fa07ee70f@hartkopp.net?part=3