[PATCH v2 0/2] Fix two KCSAN issues

[PATCH v2 0/2] Fix two KCSAN issues

[Oliver Hartkopp]

Patch 1 supersedes the "add locking when updating filter and timer values"
https://patchwork.kernel.org/project/linux-can/patch/20260219-bcm_spin_lock_init-v3-2-c61026740d7b@hartkopp.net/
where the readers in softirq/timer context have not been addressed
correctly and some comment was insufficient.

Patch 2 fixed an issue that has been detected by KCSAN when I was testing
patch 1 ;-)

V2: Changed the commit message of patch 1 as suggested by the AI review.
But the review result that "The syzbot report link could not be verified."
is obviously wrong. The URL works flawlessly:
https://lore.kernel.org/linux-can/6975d5cf.a00a0220.33ccc7.0022.GAE@google.com/

Oliver Hartkopp (2):
  can: bcm: add locking when updating filter and timer values
  can: bcm: use atomic access in receive statistics

 net/can/bcm.c | 88 ++++++++++++++++++++++++++++++++++++++++-----------
 1 file changed, 69 insertions(+), 19 deletions(-)

-- 
2.53.0

[PATCH v2 1/2] can: bcm: add locking 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.

Fix the RX_SETUP update case by protecting the timer and filter 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 | 61 +++++++++++++++++++++++++++++++++++++++++++--------
 1 file changed, 52 insertions(+), 9 deletions(-)

diff --git a/net/can/bcm.c b/net/can/bcm.c
index a4bef2c48a55..dbdf5d4f4ed7 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 filter/timer data updates */
 };
 
 struct bcm_sock {
 	struct sock sk;
 	int bound;
@@ -616,10 +617,12 @@ static void bcm_rx_starttimer(struct bcm_op *op)
 static enum hrtimer_restart bcm_rx_timeout_handler(struct hrtimer *hrtimer)
 {
 	struct bcm_op *op = container_of(hrtimer, struct bcm_op, timer);
 	struct bcm_msg_head msg_head;
 
+	spin_lock_bh(&op->bcm_rx_update_lock);
+
 	/* if user wants to be informed, when cyclic CAN-Messages come back */
 	if ((op->flags & RX_ANNOUNCE_RESUME) && op->last_frames) {
 		/* clear received CAN frames to indicate 'nothing received' */
 		memset(op->last_frames, 0, op->nframes * op->cfsiz);
 	}
@@ -632,10 +635,12 @@ static enum hrtimer_restart bcm_rx_timeout_handler(struct hrtimer *hrtimer)
 	msg_head.ival1   = op->ival1;
 	msg_head.ival2   = op->ival2;
 	msg_head.can_id  = op->can_id;
 	msg_head.nframes = 0;
 
+	spin_unlock_bh(&op->bcm_rx_update_lock);
+
 	bcm_send_to_user(op, &msg_head, NULL, 0);
 
 	return HRTIMER_NORESTART;
 }
 
@@ -680,19 +685,26 @@ static int bcm_rx_thr_flush(struct bcm_op *op)
  *                      Check for throttled data and send it to the userspace
  */
 static enum hrtimer_restart bcm_rx_thr_handler(struct hrtimer *hrtimer)
 {
 	struct bcm_op *op = container_of(hrtimer, struct bcm_op, thrtimer);
+	enum hrtimer_restart ret;
+
+	spin_lock_bh(&op->bcm_rx_update_lock);
 
 	if (bcm_rx_thr_flush(op)) {
 		hrtimer_forward_now(hrtimer, op->kt_ival2);
-		return HRTIMER_RESTART;
+		ret = HRTIMER_RESTART;
 	} else {
 		/* rearm throttle handling */
 		op->kt_lastmsg = 0;
-		return HRTIMER_NORESTART;
+		ret = HRTIMER_NORESTART;
 	}
+
+	spin_unlock_bh(&op->bcm_rx_update_lock);
+
+	return ret;
 }
 
 /*
  * bcm_rx_handler - handle a CAN frame reception
  */
@@ -737,10 +749,12 @@ static void bcm_rx_handler(struct sk_buff *skb, void *data)
 		traffic_flags |= RX_LOCAL;
 		if (skb->sk == op->sk)
 			traffic_flags |= RX_OWN;
 	}
 
+	spin_lock_bh(&op->bcm_rx_update_lock);
+
 	if (op->flags & RX_FILTER_ID) {
 		/* the easiest case */
 		bcm_rx_update_and_send(op, op->last_frames, rxframe,
 				       traffic_flags);
 		goto rx_starttimer;
@@ -772,10 +786,12 @@ 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);
 }
 
 /*
  * helpers for bcm_op handling: find & delete bcm [rx|tx] op elements
  */
@@ -1140,10 +1156,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
@@ -1151,33 +1169,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;
 
@@ -1260,26 +1298,31 @@ 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);
+
+			spin_lock_bh(&op->bcm_rx_update_lock);
 			bcm_rx_thr_flush(op);
+			spin_unlock_bh(&op->bcm_rx_update_lock);
 		}
 
 		if ((op->flags & STARTTIMER) && op->kt_ival1)
 			hrtimer_start(&op->timer, op->kt_ival1,
 				      HRTIMER_MODE_REL_SOFT);
-- 
2.53.0

[PATCH v2 2/2] can: bcm: use atomic access in receive statistics

[Oliver Hartkopp]

KCSAN detected a data race within the bcm_rx_handler() when two CAN frames
have been simultaneously received and processed in a single rx op by two
different CPUs.

Use atomic operations to access the statistics in the hot path to fix the
KCSAN complaint.

Fixes: ffd980f976e7 ("[CAN]: Add broadcast manager (bcm) protocol")
Signed-off-by: Oliver Hartkopp <socketcan@hartkopp.net>
---
 net/can/bcm.c | 27 +++++++++++++++++----------
 1 file changed, 17 insertions(+), 10 deletions(-)

diff --git a/net/can/bcm.c b/net/can/bcm.c
index dbdf5d4f4ed7..8a2cb766603d 100644
--- a/net/can/bcm.c
+++ b/net/can/bcm.c
@@ -106,11 +106,12 @@ struct bcm_op {
 	struct list_head list;
 	struct rcu_head rcu;
 	int ifindex;
 	canid_t can_id;
 	u32 flags;
-	unsigned long frames_abs, frames_filtered;
+	atomic_long_t frames_abs;
+	atomic_long_t frames_filtered;
 	struct bcm_timeval ival1, ival2;
 	struct hrtimer timer, thrtimer;
 	ktime_t rx_stamp, kt_ival1, kt_ival2, kt_lastmsg;
 	int rx_ifindex;
 	int cfsiz;
@@ -226,11 +227,11 @@ static int bcm_proc_show(struct seq_file *m, void *v)
 	list_for_each_entry_rcu(op, &bo->rx_ops, list) {
 
 		unsigned long reduction;
 
 		/* print only active entries & prevent division by zero */
-		if (!op->frames_abs)
+		if (!atomic_long_read(&op->frames_abs))
 			continue;
 
 		seq_printf(m, "rx_op: %03X %-5s ", op->can_id,
 			   bcm_proc_getifname(net, ifname, op->ifindex));
 
@@ -248,13 +249,16 @@ static int bcm_proc_show(struct seq_file *m, void *v)
 		if (op->kt_ival2)
 			seq_printf(m, "thr=%lld ",
 				   (long long)ktime_to_us(op->kt_ival2));
 
 		seq_printf(m, "# recv %ld (%ld) => reduction: ",
-			   op->frames_filtered, op->frames_abs);
+			   atomic_long_read(&op->frames_filtered),
+			   atomic_long_read(&op->frames_abs));
 
-		reduction = 100 - (op->frames_filtered * 100) / op->frames_abs;
+		reduction = 100 -
+			(atomic_long_read(&op->frames_filtered) * 100) /
+			atomic_long_read(&op->frames_abs);
 
 		seq_printf(m, "%s%ld%%\n",
 			   (reduction == 100) ? "near " : "", reduction);
 	}
 
@@ -274,11 +278,12 @@ static int bcm_proc_show(struct seq_file *m, void *v)
 
 		if (op->kt_ival2)
 			seq_printf(m, "t2=%lld ",
 				   (long long)ktime_to_us(op->kt_ival2));
 
-		seq_printf(m, "# sent %ld\n", op->frames_abs);
+		seq_printf(m, "# sent %ld\n",
+			   atomic_long_read(&op->frames_abs));
 	}
 	seq_putc(m, '\n');
 
 	rcu_read_unlock();
 
@@ -334,11 +339,11 @@ static void bcm_can_tx(struct bcm_op *op)
 
 	/* update currframe and count under lock protection */
 	spin_lock_bh(&op->bcm_tx_lock);
 
 	if (!err)
-		op->frames_abs++;
+		atomic_long_inc(&op->frames_abs);
 
 	op->currframe++;
 
 	/* reached last frame? */
 	if (op->currframe >= op->nframes)
@@ -488,15 +493,17 @@ static enum hrtimer_restart bcm_tx_timeout_handler(struct hrtimer *hrtimer)
 static void bcm_rx_changed(struct bcm_op *op, struct canfd_frame *data)
 {
 	struct bcm_msg_head head;
 
 	/* update statistics */
-	op->frames_filtered++;
+	atomic_long_inc(&op->frames_filtered);
 
 	/* prevent statistics overflow */
-	if (op->frames_filtered > ULONG_MAX/100)
-		op->frames_filtered = op->frames_abs = 0;
+	if (atomic_long_read(&op->frames_filtered) > ULONG_MAX / 100) {
+		atomic_long_set(&op->frames_filtered, 0);
+		atomic_long_set(&op->frames_abs, 0);
+	}
 
 	/* this element is not throttled anymore */
 	data->flags &= ~RX_THR;
 
 	memset(&head, 0, sizeof(head));
@@ -733,11 +740,11 @@ static void bcm_rx_handler(struct sk_buff *skb, void *data)
 	/* save rx timestamp */
 	op->rx_stamp = skb->tstamp;
 	/* save originator for recvfrom() */
 	op->rx_ifindex = skb->dev->ifindex;
 	/* update statistics */
-	op->frames_abs++;
+	atomic_long_inc(&op->frames_abs);
 
 	if (op->flags & RX_RTR_FRAME) {
 		/* send reply for RTR-request (placed in op->frames[0]) */
 		bcm_can_tx(op);
 		return;
-- 
2.53.0

Re: [PATCH v2 2/2] can: bcm: use atomic access in receive statistics

[sashiko-bot]

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

New issues:
- [Medium] Converting `frames_filtered` to `atomic_long_t` introduces a signed integer overflow on 32-bit systems during procfs statistics calculation, producing bogus corrupted percentages.

Pre-existing issues:
- [High] A Time-of-Check to Time-of-Use (TOCTOU) race condition in `bcm_proc_show()` can cause a division by zero and kernel panic due to multiple lockless reads of `op->frames_abs`.
--

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

can: bcm: use atomic access in receive statistics

This commit modifies the CAN broadcast manager (bcm) protocol to use atomic
operations for rx statistics. This prevents a data race detected by KCSAN when
multiple CAN frames are processed simultaneously by different CPUs.

> diff --git a/net/can/bcm.c b/net/can/bcm.c
> index dbdf5d4f4ed7b..8a2cb766603dc 100644
> --- a/net/can/bcm.c
> +++ b/net/can/bcm.c
> @@ -228,7 +229,7 @@ static int bcm_proc_show(struct seq_file *m, void *v)
>  		unsigned long reduction;
>  
>  		/* print only active entries & prevent division by zero */
> -		if (!op->frames_abs)
> +		if (!atomic_long_read(&op->frames_abs))
>  			continue;
>  
>  		seq_printf(m, "rx_op: %03X %-5s ", op->can_id,

[ ... ]

> @@ -250,9 +251,12 @@ static int bcm_proc_show(struct seq_file *m, void *v)
>  				   (long long)ktime_to_us(op->kt_ival2));
>  
>  		seq_printf(m, "# recv %ld (%ld) => reduction: ",
> -			   op->frames_filtered, op->frames_abs);
> +			   atomic_long_read(&op->frames_filtered),
> +			   atomic_long_read(&op->frames_abs));
>  
> -		reduction = 100 - (op->frames_filtered * 100) / op->frames_abs;
> +		reduction = 100 -
> +			(atomic_long_read(&op->frames_filtered) * 100) /
> +			atomic_long_read(&op->frames_abs);

[Severity: High]
This isn't a bug introduced by this patch, but does reading op->frames_abs
multiple times introduce a Time-of-Check to Time-of-Use race here?

bcm_proc_show() in net/can/bcm.c first ensures op->frames_abs is not zero.
However, concurrently, bcm_rx_changed() can hit the overflow threshold and
reset it:

net/can/bcm.c:bcm_rx_changed() {
    ...
	if (atomic_long_read(&op->frames_filtered) > ULONG_MAX / 100) {
		atomic_long_set(&op->frames_filtered, 0);
		atomic_long_set(&op->frames_abs, 0);
	}
    ...
}

If this reset occurs precisely between the initial non-zero check and the
division above, could the denominator evaluate to 0 and cause a divide-by-zero
kernel panic?

Would it be safer to read the atomic values exactly once into local variables
at the beginning of the iteration?

[Severity: Medium]
Does changing the statistics variables to atomic_long_t introduce a signed
integer overflow on 32-bit systems?

atomic_long_read() returns a signed long. The expression
atomic_long_read(&op->frames_filtered) * 100 in bcm_proc_show() now evaluates
using signed 32-bit integer arithmetic on 32-bit systems.

While the overflow prevention in bcm_rx_changed() caps frames_filtered at
ULONG_MAX / 100, multiplying by 100 when frames_filtered exceeds LONG_MAX / 100
will overflow the maximum positive value for a signed 32-bit integer.

Dividing this negative intermediate result by frames_abs yields a negative
number, which corrupts the final reduction percentage.

Should the reads be cast to unsigned long before the calculation?

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

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

[sashiko-bot]

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