From: Oliver Hartkopp <socketcan@hartkopp.net>
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 resolved by protecting the 'lockless'
data updates with a spin_lock_bh().
Fixes: ffd980f976e7 ("[CAN]: Add broadcast manager (bcm) protocol")
Reported-by: syzbot+75e5e4ae00c3b4bb544e@syzkaller.appspotmail.com
Closes: https://lore.kernel.org/linux-can/6975d5cf.a00a0220.33ccc7.0022.GAE@google.com/
Signed-off-by: Oliver Hartkopp <socketcan@hartkopp.net>
---
net/can/bcm.c | 39 ++++++++++++++++++++++++++++++++-------
1 file changed, 32 insertions(+), 7 deletions(-)
diff --git a/net/can/bcm.c b/net/can/bcm.c
index fd9fa072881e..0a3dc5500e14 100644
--- a/net/can/bcm.c
+++ b/net/can/bcm.c
@@ -123,10 +123,11 @@ struct bcm_op {
struct canfd_frame sframe;
struct canfd_frame last_sframe;
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 update of filter data */
};
struct bcm_sock {
struct sock sk;
int bound;
@@ -1141,10 +1142,12 @@ static int bcm_rx_setup(struct bcm_msg_head *msg_head, struct msghdr *msg,
return -EINVAL;
/* check the given can_id */
op = bcm_find_op(&bo->rx_ops, msg_head, ifindex);
if (op) {
+ void *new_frames = NULL;
+
/* update existing BCM operation */
/*
* Do we need more space for the CAN frames than currently
* allocated? -> This is a _really_ unusual use-case and
@@ -1152,33 +1155,53 @@ static int bcm_rx_setup(struct bcm_msg_head *msg_head, struct msghdr *msg,
*/
if (msg_head->nframes > op->nframes)
return -E2BIG;
if (msg_head->nframes) {
- /* update CAN frames content */
- err = memcpy_from_msg(op->frames, msg,
+ /* get new CAN frames content before locking */
+ new_frames = kmalloc(msg_head->nframes * op->cfsiz,
+ GFP_KERNEL);
+ if (!new_frames)
+ return -ENOMEM;
+
+ err = memcpy_from_msg(new_frames, msg,
msg_head->nframes * op->cfsiz);
- if (err < 0)
+ if (err < 0) {
+ kfree(new_frames);
return err;
-
- /* clear last_frames to indicate 'nothing received' */
- memset(op->last_frames, 0, msg_head->nframes * op->cfsiz);
+ }
}
+ spin_lock_bh(&op->bcm_rx_update_lock);
op->nframes = msg_head->nframes;
op->flags = msg_head->flags;
+ if (msg_head->nframes) {
+ /* update CAN frames content */
+ memcpy(op->frames, new_frames,
+ msg_head->nframes * op->cfsiz);
+
+ /* clear last_frames to indicate 'nothing received' */
+ memset(op->last_frames, 0,
+ msg_head->nframes * op->cfsiz);
+ }
+ spin_unlock_bh(&op->bcm_rx_update_lock);
+
+ /* free temporary frames / kfree(NULL) is safe */
+ kfree(new_frames);
+
/* Only an update -> do not call can_rx_register() */
do_rx_register = 0;
} else {
/* insert new BCM operation for the given can_id */
op = kzalloc(OPSIZ, GFP_KERNEL);
if (!op)
return -ENOMEM;
spin_lock_init(&op->bcm_tx_lock);
+ spin_lock_init(&op->bcm_rx_update_lock);
op->can_id = msg_head->can_id;
op->nframes = msg_head->nframes;
op->cfsiz = CFSIZ(msg_head->flags);
op->flags = msg_head->flags;
@@ -1261,24 +1284,26 @@ static int bcm_rx_setup(struct bcm_msg_head *msg_head, struct msghdr *msg,
} else {
if (op->flags & SETTIMER) {
/* set timer value */
+ spin_lock_bh(&op->bcm_rx_update_lock);
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;
+ spin_unlock_bh(&op->bcm_rx_update_lock);
/* 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.51.0The nipa/ai-review bot found these problems:
| https://netdev-ai.bots.linux.dev/ai-review.html?id=873e6dfb-7d8e-4f4d-98ef-a89fcedb701a
commit 6ea1c1c7064ff2b5c2a0434ec61d62fd3463fc12
Author: Oliver Hartkopp <socketcan@hartkopp.net>
can: bcm: add locking when updating filter and timer values
This commit addresses a KCSAN-detected data race in the CAN BCM protocol
by adding locking to protect simultaneous access to timer and filter values
during updates in bcm_rx_setup(). The race was benign functionally but is
resolved by introducing spin_lock_bh() protection.
Closes: 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 fd9fa072881e..0a3dc5500e14 100644
> --- a/net/can/bcm.c
> +++ b/net/can/bcm.c
> @@ -125,6 +125,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 update of filter data */
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
The comment says this lock protects "update of filter data" but the lock
is also used to protect timer data updates (ival1, ival2, kt_ival1,
kt_ival2, kt_lastmsg) in the SETTIMER block later in bcm_rx_setup().
Should the comment be more accurate and say "protect update of filter and
timer data"?
Additionally, this lock differs from bcm_tx_lock in that it's only taken on
the writer side (bcm_rx_setup) while the reader-side softirq handlers
(bcm_rx_handler, bcm_rx_timeout_handler, bcm_rx_thr_handler) never acquire
this lock. The protection is asymmetric -- it provides KCSAN suppression
and local-CPU softirq exclusion through spin_lock_bh(), but doesn't
synchronize against readers on other CPUs.
Is this asymmetric locking strategy intentional, or should the comment
clarify the actual protection model?
> };
>
> struct bcm_sock {
[ ... ]
> @@ -1263,10 +1286,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);
> 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;
> + spin_unlock_bh(&op->bcm_rx_update_lock);
This is where the lock also protects timer data, which confirms the comment
at the struct field declaration should mention timer data protection as well.
[ ... ]
--
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 |
On 02.03.2026 11:21:38, Marc Kleine-Budde wrote: > The nipa/ai-review bot found these problems: > > | https://netdev-ai.bots.linux.dev/ai-review.html?id=873e6dfb-7d8e-4f4d-98ef-a89fcedb701a I think this patch is the only one of this series, which is still open, right? 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 |
On 23.03.26 10:42, Marc Kleine-Budde wrote: > On 02.03.2026 11:21:38, Marc Kleine-Budde wrote: >> The nipa/ai-review bot found these problems: >> >> | https://netdev-ai.bots.linux.dev/ai-review.html?id=873e6dfb-7d8e-4f4d-98ef-a89fcedb701a > > I think this patch is the only one of this series, which is still open, > right? > Yes, it is still open. I'll look at it soon, but it should not delay the other fixes that are currently agreed. Best regards, Oliver
© 2016 - 2026 Red Hat, Inc.