[PATCH v2] can: bcm: fix CAN frame rx/tx statistics

[PATCH v2] can: bcm: fix CAN frame rx/tx 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 with (signed) long data types to access the
statistics in the hot path to fix the KCSAN complaint.

Additionally simplify the update and check of statistics overflow by
using the atomic operations in a separate bcm_update_stats() function.

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

diff --git a/net/can/bcm.c b/net/can/bcm.c
index 658c4a7a55df..e5db4dca8d5a 100644
--- a/net/can/bcm.c
+++ b/net/can/bcm.c
@@ -106,11 +106,11 @@ 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, 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;
@@ -223,14 +223,17 @@ static int bcm_proc_show(struct seq_file *m, void *v)
 
 	rcu_read_lock();
 
 	list_for_each_entry_rcu(op, &bo->rx_ops, list) {
 
-		unsigned long reduction;
+		long reduction, frames_filtered, frames_abs;
+
+		frames_filtered = atomic_long_read(&op->frames_filtered);
+		frames_abs = atomic_long_read(&op->frames_abs);
 
 		/* print only active entries & prevent division by zero */
-		if (!op->frames_abs)
+		if (!frames_abs)
 			continue;
 
 		seq_printf(m, "rx_op: %03X %-5s ", op->can_id,
 			   bcm_proc_getifname(net, ifname, op->ifindex));
 
@@ -248,13 +251,13 @@ 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);
+			   frames_filtered, frames_abs);
 
-		reduction = 100 - (op->frames_filtered * 100) / op->frames_abs;
+		reduction = 100 - (frames_filtered * 100) / frames_abs;
 
 		seq_printf(m, "%s%ld%%\n",
 			   (reduction == 100) ? "near " : "", reduction);
 	}
 
@@ -274,20 +277,30 @@ 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();
 
 	return 0;
 }
 #endif /* CONFIG_PROC_FS */
 
+static void bcm_update_stats(struct bcm_op *op)
+{
+	/* prevent statistics overflow */
+	if (atomic_long_inc_return(&op->frames_abs) > LONG_MAX / 100) {
+		atomic_long_set(&op->frames_filtered, 0);
+		atomic_long_set(&op->frames_abs, 0);
+	}
+}
+
 /*
  * bcm_can_tx - send the (next) CAN frame to the appropriate CAN interface
  *              of the given bcm tx op
  */
 static void bcm_can_tx(struct bcm_op *op)
@@ -334,11 +347,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++;
+		bcm_update_stats(op);
 
 	op->currframe++;
 
 	/* reached last frame? */
 	if (op->currframe >= op->nframes)
@@ -487,16 +500,13 @@ 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++;
-
-	/* prevent statistics overflow */
-	if (op->frames_filtered > ULONG_MAX/100)
-		op->frames_filtered = op->frames_abs = 0;
+	/* update statistics (frames_filtered <= frames_abs) */
+	if (atomic_long_read(&op->frames_abs))
+		atomic_long_inc(&op->frames_filtered);
 
 	/* this element is not throttled anymore */
 	data->flags &= ~RX_THR;
 
 	memset(&head, 0, sizeof(head));
@@ -733,11 +743,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++;
+	bcm_update_stats(op);
 
 	if (op->flags & RX_RTR_FRAME) {
 		/* send reply for RTR-request (placed in op->frames[0]) */
 		bcm_can_tx(op);
 		return;
-- 
2.53.0

[PATCH] can: bcm: use list_add_rcu() in bcm_[rx|tx]_setup()

[Oliver Hartkopp]

sashiko-bot remarked the missing use of list_add_rcu() in
bcm_[rx|tx]_setup() to have a proper initialized bcm_op structure
when bcm_proc_show() traverses the bcm_op's under rcu_read_lock().

To cover all initial settings of the bcm_op's the list_add_rcu() calls
are moved to the end of the setup code.

Reported-by: sashiko-reviews@lists.linux.dev
Closes: https://lore.kernel.org/linux-can/20260610094654.A1FFE1F00893@smtp.kernel.org/
Fixes: dac5e6249159 ("can: bcm: add missing rcu read protection for procfs content")
Signed-off-by: Oliver Hartkopp <socketcan@hartkopp.net>
---
 net/can/bcm.c | 15 +++++++++------
 1 file changed, 9 insertions(+), 6 deletions(-)

diff --git a/net/can/bcm.c b/net/can/bcm.c
index e5db4dca8d5a..76d1e067e156 100644
--- a/net/can/bcm.c
+++ b/net/can/bcm.c
@@ -954,10 +954,11 @@ static int bcm_tx_setup(struct bcm_msg_head *msg_head, struct msghdr *msg,
 			int ifindex, struct sock *sk)
 {
 	struct bcm_sock *bo = bcm_sk(sk);
 	struct bcm_op *op;
 	struct canfd_frame *cf;
+	bool add_op_to_list = false;
 	unsigned int i;
 	int err;
 
 	/* we need a real device to send frames */
 	if (!ifindex)
@@ -1094,12 +1095,11 @@ static int bcm_tx_setup(struct bcm_msg_head *msg_head, struct msghdr *msg,
 
 		/* currently unused in tx_ops */
 		hrtimer_setup(&op->thrtimer, hrtimer_dummy_timeout, CLOCK_MONOTONIC,
 			      HRTIMER_MODE_REL_SOFT);
 
-		/* add this bcm_op to the list of the tx_ops */
-		list_add(&op->list, &bo->tx_ops);
+		add_op_to_list = true;
 
 	} /* if ((op = bcm_find_op(&bo->tx_ops, msg_head->can_id, ifindex))) */
 
 	if (op->flags & SETTIMER) {
 		/* set timer values */
@@ -1117,10 +1117,14 @@ static int bcm_tx_setup(struct bcm_msg_head *msg_head, struct msghdr *msg,
 		hrtimer_cancel(&op->timer);
 		/* spec: send CAN frame when starting timer */
 		op->flags |= TX_ANNOUNCE;
 	}
 
+	/* add this bcm_op to the list of the tx_ops? */
+	if (add_op_to_list)
+		list_add_rcu(&op->list, &bo->tx_ops);
+
 	if (op->flags & TX_ANNOUNCE)
 		bcm_can_tx(op);
 
 	if (op->flags & STARTTIMER)
 		bcm_tx_start_timer(op);
@@ -1278,13 +1282,10 @@ static int bcm_rx_setup(struct bcm_msg_head *msg_head, struct msghdr *msg,
 		hrtimer_setup(&op->timer, bcm_rx_timeout_handler, CLOCK_MONOTONIC,
 			      HRTIMER_MODE_REL_SOFT);
 		hrtimer_setup(&op->thrtimer, bcm_rx_thr_handler, CLOCK_MONOTONIC,
 			      HRTIMER_MODE_REL_SOFT);
 
-		/* add this bcm_op to the list of the rx_ops */
-		list_add(&op->list, &bo->rx_ops);
-
 		/* call can_rx_register() */
 		do_rx_register = 1;
 
 	} /* if ((op = bcm_find_op(&bo->rx_ops, msg_head->can_id, ifindex))) */
 
@@ -1359,14 +1360,16 @@ static int bcm_rx_setup(struct bcm_msg_head *msg_head, struct msghdr *msg,
 			err = can_rx_register(sock_net(sk), NULL, op->can_id,
 					      REGMASK(op->can_id),
 					      bcm_rx_handler, op, "bcm", sk);
 		if (err) {
 			/* this bcm rx op is broken -> remove it */
-			list_del_rcu(&op->list);
 			bcm_remove_op(op);
 			return err;
 		}
+
+		/* add this bcm_op to the list of the rx_ops */
+		list_add_rcu(&op->list, &bo->rx_ops);
 	}
 
 	return msg_head->nframes * op->cfsiz + MHSIZ;
 }
 
-- 
2.53.0

Re: [PATCH] can: bcm: use list_add_rcu() in bcm_[rx|tx]_setup()

[sashiko-bot]

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

Pre-existing issues:
- [High] Data race allowing transmission of unvalidated CAN frames with out-of-bounds lengths.
- [Medium] Missing RCU list traversal primitive for tx_ops in bcm_proc_show().
- [Medium] Unrecoverable state corruption upon validation failure during BCM operation update.
--

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

can: bcm: use list_add_rcu() in bcm_[rx|tx]_setup()

This patch moves the list_add_rcu() calls in bcm_tx_setup() and
bcm_rx_setup() to the end of the initialization code. This ensures the
bcm_op structure is fully initialized before it becomes visible to RCU
readers like bcm_proc_show().

Link: https://lore.kernel.org/linux-can/20260610094654.A1FFE1F00893@smtp.kernel.org/

> diff --git a/net/can/bcm.c b/net/can/bcm.c
> index a4bef2c48a559..7215fa32832c8 100644
> --- a/net/can/bcm.c
> +++ b/net/can/bcm.c
> @@ -930,6 +930,7 @@ static int bcm_tx_setup(struct bcm_msg_head *msg_head, struct msghdr *msg,
>  	struct bcm_sock *bo = bcm_sk(sk);
>  	struct bcm_op *op;
>  	struct canfd_frame *cf;
> +	bool add_op_to_list = false;
>  	unsigned int i;
>  	int err;

[Severity: High]
This is a pre-existing issue, but is it possible for an unvalidated
CAN frame to be transmitted here?

When updating an existing operation in bcm_tx_setup(), the user data is
copied directly into the active transmission buffer before validation:

    err = memcpy_from_msg((u8 *)cf, msg, op->cfsiz);
    if (op->flags & CAN_FD_FRAME) {
        if (cf->len > 64)
            err = -EINVAL;
    }

Concurrently, the softirq timer bcm_tx_timeout_handler() could fire,
calling bcm_can_tx(), which fetches this frame and sends it without locks:

    skb_put_data(skb, cf, op->cfsiz);
    err = can_send(skb, 1);

Could a malicious user craft a frame with an out-of-bounds length
(e.g., cf->len = 255) that gets fetched and transmitted before
bcm_tx_setup() validates it and aborts? This could potentially cause
crashes or out-of-bounds memory reads in downstream CAN drivers.


[Severity: Medium]
This is a pre-existing issue, but does a failed validation leave the socket
in an unrecoverable corrupted state?

In both bcm_tx_setup() and bcm_rx_setup(), if an existing operation is being
updated and the validation fails after the memcpy_from_msg() block:

    err = memcpy_from_msg(op->frames, msg, msg_head->nframes * op->cfsiz);
    if (err < 0)
        return err;

The function returns an error without reverting the changes to op->frames.
Because the timers might still be running for the active bcm_op, the socket
seems permanently stuck transmitting or filtering based on this partially
overwritten data.

[ ... ]
> @@ -1093,6 +1093,10 @@ static int bcm_tx_setup(struct bcm_msg_head *msg_head, struct msghdr *msg,
>  		op->flags |= TX_ANNOUNCE;
>  	}
>  
> +	/* add this bcm_op to the list of the tx_ops? */
> +	if (add_op_to_list)
> +		list_add_rcu(&op->list, &bo->tx_ops);
> +

[Severity: Medium]
This is a pre-existing issue, but since we are now using list_add_rcu()
here, should the iteration in bcm_proc_show() also use the RCU primitive?

bcm_proc_show() traverses the bo->tx_ops list under rcu_read_lock(), but
currently uses the standard list macro:

    list_for_each_entry(op, &bo->tx_ops, list) {

Without list_for_each_entry_rcu(), it omits the necessary rcu_dereference()
calls and memory barriers. On weakly ordered architectures, couldn't this
lead to reading torn pointers or uninitialized memory of a newly inserted
bcm_op, potentially causing a crash?

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

Re: [PATCH v2] can: bcm: fix CAN frame rx/tx statistics

[sashiko-bot]

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

New issues:
- [Medium] Lockless sequential resetting of related statistics creates a race condition leading to invalid procfs output.

Pre-existing issues:
- [High] Missing synchronize_rcu() in bcm_delete_rx_op() causes Use-After-Free of bcm_op due to concurrent timer re-arming.
- [High] Incomplete fix for concurrent rx data races leaves CAN frame payload and timestamps vulnerable to corruption.
--

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

can: bcm: fix CAN frame rx/tx statistics

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. This patch uses atomic operations with (signed) long data
types to access the statistics in the hot path.

> diff --git a/net/can/bcm.c b/net/can/bcm.c
> index a4bef2c48a559..e7b1eb61918f5 100644
> --- a/net/can/bcm.c
> +++ b/net/can/bcm.c
[ ... ]
> @@ -249,9 +252,9 @@ 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);
> +			   frames_filtered, frames_abs);
>  
> -		reduction = 100 - (op->frames_filtered * 100) / op->frames_abs;
> +		reduction = 100 - (frames_filtered * 100) / frames_abs;
>  
>  		seq_printf(m, "%s%ld%%\n",
>  			   (reduction == 100) ? "near " : "", reduction);

[Severity: Medium]
Could this reduction calculation produce a wildly incorrect negative
percentage if the counters become desynchronized?

[ ... ]
> @@ -285,6 +289,15 @@ static int bcm_proc_show(struct seq_file *m, void *v)
>  }
>  #endif /* CONFIG_PROC_FS */
>  
> +static void bcm_update_stats(struct bcm_op *op)
> +{
> +	/* prevent statistics overflow */
> +	if (atomic_long_inc_return(&op->frames_abs) > LONG_MAX / 100) {
> +		atomic_long_set(&op->frames_filtered, 0);
> +		atomic_long_set(&op->frames_abs, 0);
> +	}
> +}

[Severity: Medium]
Does sequentially resetting these atomic counters introduce a race window?

If a thread is preempted between resetting frames_filtered and frames_abs,
could another thread process numerous CAN frames and increment both counters,
leaving frames_filtered at a large value when the delayed thread resumes and
resets only frames_abs?

[ ... ]
> @@ -723,7 +733,7 @@ static void bcm_rx_handler(struct sk_buff *skb, void *data)
>  	/* save originator for recvfrom() */
>  	op->rx_ifindex = skb->dev->ifindex;
>  	/* update statistics */
> -	op->frames_abs++;
> +	bcm_update_stats(op);

[Severity: High]
This isn't a bug introduced by this patch, but since bcm_rx_handler() can
execute concurrently on multiple CPUs, does this leave the rest of the
shared bcm_op state vulnerable?

Concurrent lockless writes to op->rx_stamp and op->rx_ifindex could cause
torn reads.

Additionally, bcm_rx_update_and_send() performs a non-atomic memcpy()
to shared memory:

net/can/bcm.c:bcm_rx_update_and_send() {
	...
	memcpy(lastdata, rxdata, op->cfsiz);
	lastdata->flags |= (RX_RECV|RX_THR);
	lastdata->flags |= traffic_flags;
	...
}

Can this corrupt the CAN frame payload delivered to userspace?

[Severity: High]
This is a pre-existing issue, but is there a use-after-free risk during
RX operation removal?

When bcm_delete_rx_op() removes an RX operation, it defers subscriber
removal to an RCU callback via can_rx_unregister(). It then calls
bcm_remove_op() which cancels timers and frees the bcm_op via call_rcu()
without waiting for existing readers:

net/can/bcm.c:bcm_delete_rx_op() {
	...
	can_rx_unregister(sock_net(op->sk), NULL, op->can_id,
			  REGMASK(op->can_id), bcm_rx_handler, op);
	list_del_rcu(&op->list);
	bcm_remove_op(op);
	...
}

Could a concurrently executing bcm_rx_handler() subsequently call
bcm_rx_update_and_send(), which unconditionally executes
hrtimer_start(&op->thrtimer, ...)?

Does this re-arm the timer after it was cancelled, causing a use-after-free
when the timer fires after the bcm_op memory is freed at the end of the RCU
grace period?

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