Skip to contentSkip to conversation

can: bcm: fix CAN frame rx/tx statistics

PATCH New Oliver Hartkopp 2 participants 4 messages Jun 10

Subsystems (2)
lore
Share & tools
lore

Oliver Hartkopp

author maintainer
#0 lore raw

PATCH v2 1/1 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.

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")
---
 net/can/bcm.c | 38 ++++++++++++++++++++++++--------------
 1 file changed, 24 insertions(+), 14 deletions(-)
net/can/bcm.c +24 -14
index 658c4a7a55df..e5db4dca8d5a 100644
@@ -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;
Signature · 1 line
2.53.0
author maintainer
#1 lore raw

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

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 [email protected]
Fixes dac5e6249159 ("can: bcm: add missing rcu read protection for procfs content")
---
 net/can/bcm.c | 15 +++++++++------
 1 file changed, 9 insertions(+), 6 deletions(-)
net/can/bcm.c +9 -6
index e5db4dca8d5a..76d1e067e156 100644
@@ -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;
 }
 
Signature · 1 line
2.53.0
#2 lore raw

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

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

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 <[email protected]>

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().

net/can/bcm.c static int bcm_tx_setup(struct bcm_msg_head *msg_head, struct msghdr *msg, · line 935

Quoted 11 lines
net/can/bcm.c +1 -0
index a4bef2c48a559..7215fa32832c8 100644
@@ -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.

[ ... ]

net/can/bcm.c static int bcm_tx_setup(struct bcm_msg_head *msg_head, struct msghdr *msg, · line 1099

Quoted 8 lines
net/can/bcm.c +4 -0
@@ -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?

Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1
#3 lore raw
In reply to Oliver Hartkopp
Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:

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 <[email protected]>

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.

Quoted 4 lines
net/can/bcm.c +0 -0
index a4bef2c48a559..e7b1eb61918f5 100644

[ ... ]

net/can/bcm.c static int bcm_proc_show(struct seq_file *m, void *v) · line 260

Quoted 12 lines
net/can/bcm.c +2 -2
@@ -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?

[ ... ]

net/can/bcm.c static int bcm_proc_show(struct seq_file *m, void *v) · line 299

Quoted 12 lines
net/can/bcm.c +8 -0
@@ -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?

[ ... ]

net/can/bcm.c static void bcm_rx_handler(struct sk_buff *skb, void *data) · line 736

Quoted 6 lines
net/can/bcm.c +1 -1
@@ -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?

Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1