[PATCH] can: bcm: disable bh when updating filter and timer values

[PATCH] can: bcm: disable bh when updating filter and timer values

[Oliver Hartkopp]

KCSAN detected a simultaneous access to timer values that can be
overwritten in bcm_rx_setup when updating timer and filter content.
This caused no functional issues in the past as the new values might
show up at any time without losing its intended functionality.

Btw. the KCSAN report can be easily resolved by protecting the
'lockless' data updates with local_bh_[dis|en]able().

Fixes: ffd980f976e7 ("[CAN]: Add broadcast manager (bcm) protocol")
Reported-by: syzbot+75e5e4ae00c3b4bb544e@syzkaller.appspotmail.com
Signed-off-by: Oliver Hartkopp <socketcan@hartkopp.net>
---
 net/can/bcm.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/net/can/bcm.c b/net/can/bcm.c
index 7eba8ae01a5b..5fde4d4db893 100644
--- a/net/can/bcm.c
+++ b/net/can/bcm.c
@@ -1145,23 +1145,27 @@ static int bcm_rx_setup(struct bcm_msg_head *msg_head, struct msghdr *msg,
 		 * therefore (complexity / locking) it is not supported.
 		 */
 		if (msg_head->nframes > op->nframes)
 			return -E2BIG;
 
+		local_bh_disable();
 		if (msg_head->nframes) {
 			/* update CAN frames content */
 			err = memcpy_from_msg(op->frames, msg,
 					      msg_head->nframes * op->cfsiz);
-			if (err < 0)
+			if (err < 0) {
+				local_bh_enable();
 				return err;
+			}
 
 			/* clear last_frames to indicate 'nothing received' */
 			memset(op->last_frames, 0, msg_head->nframes * op->cfsiz);
 		}
 
 		op->nframes = msg_head->nframes;
 		op->flags = msg_head->flags;
+		local_bh_enable();
 
 		/* Only an update -> do not call can_rx_register() */
 		do_rx_register = 0;
 
 	} else {
@@ -1254,24 +1258,26 @@ static int bcm_rx_setup(struct bcm_msg_head *msg_head, struct msghdr *msg,
 
 	} else {
 		if (op->flags & SETTIMER) {
 
 			/* set timer value */
+			local_bh_disable();
 			op->ival1 = msg_head->ival1;
 			op->ival2 = msg_head->ival2;
 			op->kt_ival1 = bcm_timeval_to_ktime(msg_head->ival1);
 			op->kt_ival2 = bcm_timeval_to_ktime(msg_head->ival2);
+			op->kt_lastmsg = 0;
+			local_bh_enable();
 
 			/* disable an active timer due to zero value? */
 			if (!op->kt_ival1)
 				hrtimer_cancel(&op->timer);
 
 			/*
 			 * In any case cancel the throttle timer, flush
 			 * potentially blocked msgs and reset throttle handling
 			 */
-			op->kt_lastmsg = 0;
 			hrtimer_cancel(&op->thrtimer);
 			bcm_rx_thr_flush(op);
 		}
 
 		if ((op->flags & STARTTIMER) && op->kt_ival1)
-- 
2.47.3

Re: [PATCH] can: bcm: disable bh when updating filter and timer values

[Marc Kleine-Budde]

[-- Attachment #1: Type: text/plain, Size: 1153 bytes --]

On 26.01.2026 17:17:11, Oliver Hartkopp wrote:
> KCSAN detected a simultaneous access to timer values that can be
> overwritten in bcm_rx_setup when updating timer and filter content.
> This caused no functional issues in the past as the new values might
> show up at any time without losing its intended functionality.

> Btw. the KCSAN report can be easily resolved by protecting the
> 'lockless' data updates with local_bh_[dis|en]able().

What's the exact problem we have here?

Simultaneous access or modification (which one?) of op->frames,
op->last_frames, op->nframes, op->flags from the timer and
bcm_rx_setup()?

How does disabling the bh on the local CPU only solve the problem? What
happens if the timer is currently running?

Also see patch c2aba69d0c36 ("can: bcm: add locking for bcm_op runtime
updates") and the related discussion.

regards,
Marc

--
Pengutronix e.K.                 | Marc Kleine-Budde          |
Embedded Linux                   | https://www.pengutronix.de |
Vertretung Nürnberg              | Phone: +49-5121-206917-129 |
Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-9   |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH] can: bcm: disable bh when updating filter and timer values

[Oliver Hartkopp]

On 28.01.26 11:25, Marc Kleine-Budde wrote:
> On 26.01.2026 17:17:11, Oliver Hartkopp wrote:
>> KCSAN detected a simultaneous access to timer values that can be
>> overwritten in bcm_rx_setup when updating timer and filter content.
>> This caused no functional issues in the past as the new values might
>> show up at any time without losing its intended functionality.
> 
>> Btw. the KCSAN report can be easily resolved by protecting the
>> 'lockless' data updates with local_bh_[dis|en]able().
> 
> What's the exact problem we have here?
> 
> Simultaneous access or modification (which one?) of op->frames,
> op->last_frames, op->nframes, op->flags from the timer and
> bcm_rx_setup()?

In opposite to the bcm op locking, where two sides are writing data to 
the variables, we can overwrite existing content in bcm_rx_setup().

The softirq side is only reading the filters and timer settings when a 
CAN frame is received. There is no concurrent write.

> How does disabling the bh on the local CPU only solve the problem? What
> happens if the timer is currently running?

There are two potential timers (timeout of cyclic messages and throttle 
timer) that can be rearmed in the softirq. There is no issue IMO and it 
was no problem in the past.

KCSAN only remarked that writing the updated values and reading these 
values happened at the same time.

And I think this can exactly be solved with local_bh_disable().
Other code in the kernel using local_bh_disable() use the same pattern 
and asking the KI about the use case of local_bh_disable() fits to this fix.

Best regards,
Oliver

> Also see patch c2aba69d0c36 ("can: bcm: add locking for bcm_op runtime
> updates") and the related discussion.
> 
> regards,
> Marc
> 
> --
> Pengutronix e.K.                 | Marc Kleine-Budde          |
> Embedded Linux                   | https://www.pengutronix.de |
> Vertretung Nürnberg              | Phone: +49-5121-206917-129 |
> Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-9   |