[PATCH 0/5] can: bcm: collected fixes

[PATCH 0/5] can: bcm: collected fixes

[Oliver Hartkopp]

Hi Marc,

as there were different patches flying arround to fix CAN_BCM issues and
AI assisted stuff pop's up again and again, I've created this collection
to be applied.

I'm not sure whether sashiko-bot takes the sequence of this series into
account when checking the last patches. So let's see.

Best regards,
Oliver

Signed-off-by: Oliver Hartkopp <socketcan@hartkopp.net>
---
Lee Jones (1):
      can: bcm: defer rx_op deallocation to workqueue to fix thrtimer UAF

Oliver Hartkopp (4):
      can: bcm: mark intentional lockless read of bo->bound for KCSAN
      can: bcm: add locking when updating filter and timer values
      can: bcm: fix CAN frame rx/tx statistics
      can: bcm: use list_add_rcu() in bcm_[rx|tx]_setup()

 net/can/bcm.c | 150 ++++++++++++++++++++++++++++++++++++++++++++--------------
 1 file changed, 115 insertions(+), 35 deletions(-)
---
base-commit: 2b414a95b8f7307d42173ba9e580d6d3e2bcbfce
change-id: 20260612-bcm_fixes-84fb4aa93ac2

Best regards,
--  
Oliver Hartkopp <socketcan@hartkopp.net>

[PATCH 1/5] can: bcm: defer rx_op deallocation to workqueue to fix thrtimer UAF

[Oliver Hartkopp]

From: Lee Jones <lee@kernel.org>

Commit f1b4e32aca08 ("can: bcm: use call_rcu() instead of costly
synchronize_rcu()") replaced synchronize_rcu() in bcm_delete_rx_op()
with call_rcu() and introduced the RX_NO_AUTOTIMER flag.

However, this flag check was omitted for thrtimer in the packet rx
fast-path. During BCM RX operation teardown, a concurrent RCU reader
(bcm_rx_handler) can race and re-arm thrtimer via
bcm_rx_update_and_send() after call_rcu() has been scheduled.  Once
the RCU grace period elapses, bcm_op is freed.  The subsequently
firing thrtimer then dereferences the deallocated op, causing a UAF.

Adding flag checks to the rx fast-path (bcm_rx_update_and_send) does not
fully close the TOCTOU race and introduces latency for every CAN frame.
Conversely, calling hrtimer_cancel() directly inside the RCU callback
(softirq context) is fatal as hrtimer_cancel() can sleep, triggering
a "scheduling while atomic" panic.

Resolve this by deferring the timer cancellation and memory free to a
dedicated unbound workqueue (bcm_wq).  The RCU callback now queues a
work item to bcm_wq, which safely cancels both timers and deallocates
memory in sleepable process context.  A dedicated workqueue is used to
prevent system-wide WQ saturation and is cleanly flushed/destroyed
on module unload to avoid rmmod page faults.

Fixes: f1b4e32aca08 ("can: bcm: use call_rcu() instead of costly synchronize_rcu()")
Signed-off-by: Lee Jones <lee@kernel.org>
---
 net/can/bcm.c | 29 ++++++++++++++++++++++++++---
 1 file changed, 26 insertions(+), 3 deletions(-)

diff --git a/net/can/bcm.c b/net/can/bcm.c
index a4bef2c48a55..c49b09f3229f 100644
--- a/net/can/bcm.c
+++ b/net/can/bcm.c
@@ -56,10 +56,11 @@
 #include <linux/can.h>
 #include <linux/can/core.h>
 #include <linux/can/skb.h>
 #include <linux/can/bcm.h>
 #include <linux/slab.h>
+#include <linux/workqueue.h>
 #include <linux/spinlock.h>
 #include <net/can.h>
 #include <net/sock.h>
 #include <net/net_namespace.h>
 
@@ -90,10 +91,12 @@ MODULE_LICENSE("Dual BSD/GPL");
 MODULE_AUTHOR("Oliver Hartkopp <oliver.hartkopp@volkswagen.de>");
 MODULE_ALIAS("can-proto-2");
 
 #define BCM_MIN_NAMELEN CAN_REQUIRED_SIZE(struct sockaddr_can, can_ifindex)
 
+static struct workqueue_struct *bcm_wq;
+
 /*
  * easy access to the first 64 bit of can(fd)_frame payload. cp->data is
  * 64 bit aligned so the offset has to be multiples of 8 which is ensured
  * by the only callers in bcm_rx_cmp_to_index() bcm_rx_handler().
  */
@@ -103,10 +106,11 @@ static inline u64 get_u64(const struct canfd_frame *cp, int offset)
 }
 
 struct bcm_op {
 	struct list_head list;
 	struct rcu_head rcu;
+	struct work_struct work;
 	int ifindex;
 	canid_t can_id;
 	u32 flags;
 	unsigned long frames_abs, frames_filtered;
 	struct bcm_timeval ival1, ival2;
@@ -791,23 +795,34 @@ static struct bcm_op *bcm_find_op(struct list_head *ops,
 	}
 
 	return NULL;
 }
 
-static void bcm_free_op_rcu(struct rcu_head *rcu_head)
+static void bcm_free_op_work(struct work_struct *work)
 {
-	struct bcm_op *op = container_of(rcu_head, struct bcm_op, rcu);
+	struct bcm_op *op = container_of(work, struct bcm_op, work);
+
+	hrtimer_cancel(&op->timer);
+	hrtimer_cancel(&op->thrtimer);
 
 	if ((op->frames) && (op->frames != &op->sframe))
 		kfree(op->frames);
 
 	if ((op->last_frames) && (op->last_frames != &op->last_sframe))
 		kfree(op->last_frames);
 
 	kfree(op);
 }
 
+static void bcm_free_op_rcu(struct rcu_head *rcu_head)
+{
+	struct bcm_op *op = container_of(rcu_head, struct bcm_op, rcu);
+
+	INIT_WORK(&op->work, bcm_free_op_work);
+	queue_work(bcm_wq, &op->work);
+}
+
 static void bcm_remove_op(struct bcm_op *op)
 {
 	hrtimer_cancel(&op->timer);
 	hrtimer_cancel(&op->thrtimer);
 
@@ -1837,15 +1852,19 @@ static struct notifier_block canbcm_notifier = {
 
 static int __init bcm_module_init(void)
 {
 	int err;
 
+	bcm_wq = alloc_workqueue("can-bcm-wq", WQ_UNBOUND, 0);
+	if (!bcm_wq)
+		return -ENOMEM;
+
 	pr_info("can: broadcast manager protocol\n");
 
 	err = register_pernet_subsys(&canbcm_pernet_ops);
 	if (err)
-		return err;
+		goto register_pernet_failed;
 
 	err = register_netdevice_notifier(&canbcm_notifier);
 	if (err)
 		goto register_notifier_failed;
 
@@ -1859,17 +1878,21 @@ static int __init bcm_module_init(void)
 
 register_proto_failed:
 	unregister_netdevice_notifier(&canbcm_notifier);
 register_notifier_failed:
 	unregister_pernet_subsys(&canbcm_pernet_ops);
+register_pernet_failed:
+	destroy_workqueue(bcm_wq);
 	return err;
 }
 
 static void __exit bcm_module_exit(void)
 {
 	can_proto_unregister(&bcm_can_proto);
 	unregister_netdevice_notifier(&canbcm_notifier);
 	unregister_pernet_subsys(&canbcm_pernet_ops);
+	rcu_barrier();
+	destroy_workqueue(bcm_wq);
 }
 
 module_init(bcm_module_init);
 module_exit(bcm_module_exit);

-- 
2.53.0

Re: [PATCH 1/5] can: bcm: defer rx_op deallocation to workqueue to fix thrtimer UAF

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Deferring timer cancellation to a workqueue fixes a UAF on `bcm_op`, but exposes a Use-After-Free (UAF) on `op->sk` because `bcm_release` does not wait for the workqueue to flush before freeing the socket.
--

commit 9591bb7d40d0a4fbd45c8700cf885c74ad1dcfec
Author: Lee Jones <lee@kernel.org>

can: bcm: defer rx_op deallocation to workqueue to fix thrtimer UAF

This commit defers the cancellation of timers and deallocation of bcm_op to
a dedicated workqueue. This prevents a use-after-free when a concurrent
RCU reader re-arms the timer after the RCU callback is scheduled.

> diff --git a/net/can/bcm.c b/net/can/bcm.c
> index a4bef2c48a559..c49b09f3229fa 100644
> --- a/net/can/bcm.c
> +++ b/net/can/bcm.c
[ ... ]
> @@ -806,6 +813,14 @@ static void bcm_free_op_rcu(struct rcu_head *rcu_head)
>  	kfree(op);
>  }
>  
> +static void bcm_free_op_rcu(struct rcu_head *rcu_head)
> +{
> +	struct bcm_op *op = container_of(rcu_head, struct bcm_op, rcu);
> +
> +	INIT_WORK(&op->work, bcm_free_op_work);
> +	queue_work(bcm_wq, &op->work);
> +}

[Severity: High]
Could this deferral to a workqueue expose a use-after-free on op->sk?

If a socket is closed immediately after an operation is deleted:

1. bcm_delete_rx_op() removes op from the socket's rx_ops list and schedules
   call_rcu().
2. A concurrent bcm_rx_handler() running under RCU re-arms op->thrtimer.
3. bcm_release() closes the socket. Since op was removed from rx_ops, it is
   skipped, and bcm_release() drops the final reference via sock_put(sk),
   freeing the socket.

Because the workqueue execution is asynchronous, bcm_release() does not wait
for it. If the re-armed thrtimer fires before bcm_free_op_work() can cancel
it, would this trigger a use-after-free?

bcm_rx_thr_handler()
  bcm_send_to_user()
    reason = sock_queue_rcv_skb_reason(op->sk, skb); 

Since op->sk is assigned in bcm_rx_setup() without a corresponding
sock_hold(), does the deferred cleanup need to hold a socket reference to
ensure the socket outlives this work item and any pending timers?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260612-bcm_fixes-v1-0-ca2fa07ee70f@hartkopp.net?part=1

Re: [PATCH 1/5] can: bcm: defer rx_op deallocation to workqueue to fix thrtimer UAF

[Oliver Hartkopp]

Document successful testing for the patch.

On 12.06.26 13:05, Oliver Hartkopp wrote:
> From: Lee Jones <lee@kernel.org>
> 
> Commit f1b4e32aca08 ("can: bcm: use call_rcu() instead of costly
> synchronize_rcu()") replaced synchronize_rcu() in bcm_delete_rx_op()
> with call_rcu() and introduced the RX_NO_AUTOTIMER flag.
> 
> However, this flag check was omitted for thrtimer in the packet rx
> fast-path. During BCM RX operation teardown, a concurrent RCU reader
> (bcm_rx_handler) can race and re-arm thrtimer via
> bcm_rx_update_and_send() after call_rcu() has been scheduled.  Once
> the RCU grace period elapses, bcm_op is freed.  The subsequently
> firing thrtimer then dereferences the deallocated op, causing a UAF.
> 
> Adding flag checks to the rx fast-path (bcm_rx_update_and_send) does not
> fully close the TOCTOU race and introduces latency for every CAN frame.
> Conversely, calling hrtimer_cancel() directly inside the RCU callback
> (softirq context) is fatal as hrtimer_cancel() can sleep, triggering
> a "scheduling while atomic" panic.
> 
> Resolve this by deferring the timer cancellation and memory free to a
> dedicated unbound workqueue (bcm_wq).  The RCU callback now queues a
> work item to bcm_wq, which safely cancels both timers and deallocates
> memory in sleepable process context.  A dedicated workqueue is used to
> prevent system-wide WQ saturation and is cleanly flushed/destroyed
> on module unload to avoid rmmod page faults.
> 
> Fixes: f1b4e32aca08 ("can: bcm: use call_rcu() instead of costly synchronize_rcu()")
> Signed-off-by: Lee Jones <lee@kernel.org>

Tested-by: Oliver Hartkopp <socketcan@hartkopp.net>
Tested-by: Ji'an <eilaimemedsnaimel@gmail.com>

> ---
>   net/can/bcm.c | 29 ++++++++++++++++++++++++++---
>   1 file changed, 26 insertions(+), 3 deletions(-)
> 
> diff --git a/net/can/bcm.c b/net/can/bcm.c
> index a4bef2c48a55..c49b09f3229f 100644
> --- a/net/can/bcm.c
> +++ b/net/can/bcm.c
> @@ -56,10 +56,11 @@
>   #include <linux/can.h>
>   #include <linux/can/core.h>
>   #include <linux/can/skb.h>
>   #include <linux/can/bcm.h>
>   #include <linux/slab.h>
> +#include <linux/workqueue.h>
>   #include <linux/spinlock.h>
>   #include <net/can.h>
>   #include <net/sock.h>
>   #include <net/net_namespace.h>
>   
> @@ -90,10 +91,12 @@ MODULE_LICENSE("Dual BSD/GPL");
>   MODULE_AUTHOR("Oliver Hartkopp <oliver.hartkopp@volkswagen.de>");
>   MODULE_ALIAS("can-proto-2");
>   
>   #define BCM_MIN_NAMELEN CAN_REQUIRED_SIZE(struct sockaddr_can, can_ifindex)
>   
> +static struct workqueue_struct *bcm_wq;
> +
>   /*
>    * easy access to the first 64 bit of can(fd)_frame payload. cp->data is
>    * 64 bit aligned so the offset has to be multiples of 8 which is ensured
>    * by the only callers in bcm_rx_cmp_to_index() bcm_rx_handler().
>    */
> @@ -103,10 +106,11 @@ static inline u64 get_u64(const struct canfd_frame *cp, int offset)
>   }
>   
>   struct bcm_op {
>   	struct list_head list;
>   	struct rcu_head rcu;
> +	struct work_struct work;
>   	int ifindex;
>   	canid_t can_id;
>   	u32 flags;
>   	unsigned long frames_abs, frames_filtered;
>   	struct bcm_timeval ival1, ival2;
> @@ -791,23 +795,34 @@ static struct bcm_op *bcm_find_op(struct list_head *ops,
>   	}
>   
>   	return NULL;
>   }
>   
> -static void bcm_free_op_rcu(struct rcu_head *rcu_head)
> +static void bcm_free_op_work(struct work_struct *work)
>   {
> -	struct bcm_op *op = container_of(rcu_head, struct bcm_op, rcu);
> +	struct bcm_op *op = container_of(work, struct bcm_op, work);
> +
> +	hrtimer_cancel(&op->timer);
> +	hrtimer_cancel(&op->thrtimer);
>   
>   	if ((op->frames) && (op->frames != &op->sframe))
>   		kfree(op->frames);
>   
>   	if ((op->last_frames) && (op->last_frames != &op->last_sframe))
>   		kfree(op->last_frames);
>   
>   	kfree(op);
>   }
>   
> +static void bcm_free_op_rcu(struct rcu_head *rcu_head)
> +{
> +	struct bcm_op *op = container_of(rcu_head, struct bcm_op, rcu);
> +
> +	INIT_WORK(&op->work, bcm_free_op_work);
> +	queue_work(bcm_wq, &op->work);
> +}
> +
>   static void bcm_remove_op(struct bcm_op *op)
>   {
>   	hrtimer_cancel(&op->timer);
>   	hrtimer_cancel(&op->thrtimer);
>   
> @@ -1837,15 +1852,19 @@ static struct notifier_block canbcm_notifier = {
>   
>   static int __init bcm_module_init(void)
>   {
>   	int err;
>   
> +	bcm_wq = alloc_workqueue("can-bcm-wq", WQ_UNBOUND, 0);
> +	if (!bcm_wq)
> +		return -ENOMEM;
> +
>   	pr_info("can: broadcast manager protocol\n");
>   
>   	err = register_pernet_subsys(&canbcm_pernet_ops);
>   	if (err)
> -		return err;
> +		goto register_pernet_failed;
>   
>   	err = register_netdevice_notifier(&canbcm_notifier);
>   	if (err)
>   		goto register_notifier_failed;
>   
> @@ -1859,17 +1878,21 @@ static int __init bcm_module_init(void)
>   
>   register_proto_failed:
>   	unregister_netdevice_notifier(&canbcm_notifier);
>   register_notifier_failed:
>   	unregister_pernet_subsys(&canbcm_pernet_ops);
> +register_pernet_failed:
> +	destroy_workqueue(bcm_wq);
>   	return err;
>   }
>   
>   static void __exit bcm_module_exit(void)
>   {
>   	can_proto_unregister(&bcm_can_proto);
>   	unregister_netdevice_notifier(&canbcm_notifier);
>   	unregister_pernet_subsys(&canbcm_pernet_ops);
> +	rcu_barrier();
> +	destroy_workqueue(bcm_wq);
>   }
>   
>   module_init(bcm_module_init);
>   module_exit(bcm_module_exit);
> 

[PATCH 2/5] can: bcm: mark intentional lockless read of bo->bound for KCSAN

[Oliver Hartkopp]

A static analyzer and KCSAN can detect a potential data race in
net/can/bcm.c between bcm_sendmsg() and bcm_notify().

bcm_sendmsg() reads bo->bound without holding the socket lock to
perform a fast-path check for unbound sockets before checking the
message length:

        /* Lockless fast-path check for bound socket */
        if (!bo->bound)
                return -ENOTCONN;

Concurrently, bcm_notify() can clear bo->bound under lock_sock(sk)
protection during a netdevice notification event:

        lock_sock(sk);
        ...
        bo->bound   = 0;
        bo->ifindex = 0;
        notify_enodev = 1;

This lockless read is intentional and functionally safe. If
bcm_sendmsg() passes the check while bcm_notify() is running, the
socket will block on the subsequent lock_sock(sk) call.

Once the lock is acquired, any subsequent attempts to configure BCM
jobs (like RX_SETUP) will safely fail because bcm_notify() has
already cleared bo->ifindex to 0. While a user-defined ifindex of 0
is normally a valid feature to listen on "all" CAN interfaces,
bcm_notify() only triggers for specific real CAN devices. In this
unregister context, the combination of bound=0 and ifindex=0 effectively
invalidates the socket binding. This prevents the registration of
stale CAN filters and drops downstream operations safely without any
memory corruption or stale state exploitation.

However, to comply with the Linux kernel memory model and to eliminate
false-positive warnings from concurrency sanitizers (KCSAN), annotate
this intentional data race using READ_ONCE() and WRITE_ONCE().

Reported-by: Ginger <ginger.jzllee@gmail.com>
Closes: https://lore.kernel.org/linux-can/CAGp+u1aBK8QVjsvAxM2Ldzep4rEbsP9x_pV3At4g=h1kVEtyhA@mail.gmail.com/
Signed-off-by: Oliver Hartkopp <socketcan@hartkopp.net>
---
 net/can/bcm.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/net/can/bcm.c b/net/can/bcm.c
index c49b09f3229f..9475758a6749 100644
--- a/net/can/bcm.c
+++ b/net/can/bcm.c
@@ -1391,11 +1391,12 @@ static int bcm_sendmsg(struct socket *sock, struct msghdr *msg, size_t size)
 	int ifindex = bo->ifindex; /* default ifindex for this bcm_op */
 	struct bcm_msg_head msg_head;
 	int cfsiz;
 	int ret; /* read bytes or error codes as return value */
 
-	if (!bo->bound)
+	/* Lockless fast-path check for bound socket */
+	if (!READ_ONCE(bo->bound))
 		return -ENOTCONN;
 
 	/* check for valid message length from userspace */
 	if (size < MHSIZ)
 		return -EINVAL;
@@ -1525,11 +1526,11 @@ static void bcm_notify(struct bcm_sock *bo, unsigned long msg,
 			if (sock_net(sk)->can.bcmproc_dir && bo->bcm_proc_read) {
 				remove_proc_entry(bo->procname, sock_net(sk)->can.bcmproc_dir);
 				bo->bcm_proc_read = NULL;
 			}
 #endif
-			bo->bound   = 0;
+			WRITE_ONCE(bo->bound, 0);
 			bo->ifindex = 0;
 			notify_enodev = 1;
 		}
 
 		release_sock(sk);

-- 
2.53.0

Re: [PATCH 2/5] can: bcm: mark intentional lockless read of bo->bound for KCSAN

[sashiko-bot]

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

Pre-existing issues:
- [High] Incomplete fix for KCSAN data race causing TOCTOU logic bug in CAN BCM socket binding. The patch annotates `bo->bound` with READ_ONCE()/WRITE_ONCE() but leaves an adjacent lockless read of `bo->ifindex` unprotected in `bcm_sendmsg()`. Because `bo->ifindex` is cached into a local variable before checking `bo->bound` and before acquiring `lock_sock()`, a concurrent `bcm_notify()` on device unregistration can clear `bo->bound` and `bo->ifindex` while `bcm_sendmsg()` retains and proceeds with the stale `ifindex`.
--

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

can: bcm: mark intentional lockless read of bo->bound for KCSAN

This commit annotates an intentional lockless read of bo->bound with
READ_ONCE() and WRITE_ONCE() to resolve a KCSAN data race warning. The data
race occurs between bcm_sendmsg() and bcm_notify().

Link: https://lore.kernel.org/linux-can/CAGp+u1aBK8QVjsvAxM2Ldzep4rEbsP9x_pV3At4g=h1kVEtyhA@mail.gmail.com/

> Once the lock is acquired, any subsequent attempts to configure BCM
> jobs (like RX_SETUP) will safely fail because bcm_notify() has
> already cleared bo->ifindex to 0.

[Severity: High]
This isn't a bug introduced by this patch, but does this actually happen? Since
bcm_sendmsg() caches bo->ifindex into a local variable before checking
bo->bound or acquiring lock_sock(), won't it use the stale ifindex instead
of 0?

net/can/bcm.c:bcm_sendmsg() {
	struct bcm_sock *bo = bcm_sk(sk);
	int ifindex = bo->ifindex; /* default ifindex for this bcm_op */
	...
	/* Lockless fast-path check for bound socket */
	if (!READ_ONCE(bo->bound))
		return -ENOTCONN;
	...
	lock_sock(sk);
	...
	case RX_SETUP:
		ret = bcm_rx_setup(&msg_head, msg, ifindex, sk);
	...
}

If bcm_notify() unregisters the device concurrently, dev_get_by_index()
inside bcm_rx_setup() might fail using the stale ifindex, which would
bypass the filter registration entirely without returning an error.

> diff --git a/net/can/bcm.c b/net/can/bcm.c
> index c49b09f3229fa..9475758a6749f 100644
> --- a/net/can/bcm.c
> +++ b/net/can/bcm.c
[ ... ]
> @@ -1527,7 +1528,7 @@ static void bcm_notify(struct bcm_sock *bo, unsigned long msg,
>  				bo->bcm_proc_read = NULL;
>  			}
>  #endif
> -			bo->bound   = 0;
> +			WRITE_ONCE(bo->bound, 0);
>  			bo->ifindex = 0;
>  			notify_enodev = 1;
>  		}

[Severity: High]
This is also a pre-existing issue, but could the lack of memory barriers cause
the CPU or compiler to reorder the lockless accesses in bcm_sendmsg(),
observing bo->ifindex == 0 while bo->bound == 1?

If so, bcm_sendmsg() would execute with ifindex = 0 and call
can_rx_register(..., NULL, ...), inadvertently establishing a global wildcard
filter that receives frames from all CAN interfaces in the namespace.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260612-bcm_fixes-v1-0-ca2fa07ee70f@hartkopp.net?part=2

[PATCH 3/5] 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 9475758a6749..fc5158132b55 100644
--- a/net/can/bcm.c
+++ b/net/can/bcm.c
@@ -127,10 +127,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;
@@ -620,10 +621,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);
 	}
@@ -636,10 +639,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;
 }
 
@@ -684,19 +689,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
  */
@@ -741,10 +753,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;
@@ -776,10 +790,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
  */
@@ -1155,10 +1171,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
@@ -1166,33 +1184,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;
 
@@ -1275,26 +1313,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

Re: [PATCH 3/5] can: bcm: add locking when updating filter and timer values

[sashiko-bot]

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

Pre-existing issues:
- [High] The patch addresses race conditions in the RX path but misses identical missing lock vulnerabilities and partial state corruption bugs in the TX path (`bcm_tx_setup` and `bcm_can_tx`).
- [High] Data race in `bcm_rx_setup` when clearing `CAN_RTR_FLAG` for `RX_RTR_FRAME` operations, which can lead to torn reads and CAN packet storms.
- [High] `bcm_rx_handler` reads `op->flags` without lock protection, triggering a data race with `bcm_rx_setup` and potential data corruption.
--

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

can: bcm: add locking when updating filter and timer values

This commit fixes a KCSAN-detected race condition where timer values
could be overwritten simultaneously in bcm_rx_setup(). It protects the
timer and filter data updates with a new 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 9475758a6749f..fc5158132b554 100644
> --- a/net/can/bcm.c
> +++ b/net/can/bcm.c
> @@ -129,6 +129,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 filter/timer data updates */
>  };

[Severity: High]
This is a pre-existing issue, but while we are adding bcm_rx_update_lock
to protect RX filter and timer updates, does the TX path need similar
protection?

In bcm_tx_setup(), when updating an existing operation, memcpy_from_msg()
copies user data directly into op->frames without locking, and 64-bit timer
values like op->kt_ival1 are updated locklessly.

Concurrently, if bcm_tx_timeout_handler() fires, it can read these torn
timer values. Also, bcm_can_tx() reads the frame payload from cf locklessly
via skb_put_data(), which races with memcpy_from_msg().

Furthermore, if bcm_tx_setup() encounters a validation error
(e.g., cf->len > 64) after memcpy_from_msg(), it returns -EINVAL without
rolling back. The active timer will continue firing, reading the malicious
length and allocating a normal-sized skb, potentially leading to an
out-of-bounds read in the CAN driver.

[ ... ]

> @@ -743,6 +755,8 @@ static void bcm_rx_handler(struct sk_buff *skb, void *data)
>  			traffic_flags |= RX_OWN;
>  	}
>  
> +	spin_lock_bh(&op->bcm_rx_update_lock);
> +
>  	if (op->flags & RX_FILTER_ID) {

[Severity: High]
This is a pre-existing issue, but does bcm_rx_handler() read op->flags
without lock protection just before this new lock is acquired?

If a user updates an existing operation to toggle RX_RTR_FRAME, bcm_rx_setup()
updates op->flags and op->frames under the bcm_rx_update_lock.

However, earlier in bcm_rx_handler(), it checks op->flags locklessly:

	if (op->flags & RX_RTR_FRAME) {
		/* send reply for RTR-request (placed in op->frames[0]) */
		bcm_can_tx(op);
		return;
	}

Could bcm_rx_handler() observe the newly written op->flags and immediately
call bcm_can_tx(), which will then read a stale or partially written
op->frames[0] to transmit? This seems to defeat the atomicity provided by
the newly introduced lock.

[ ... ]

> @@ -1277,10 +1315,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);

[Severity: High]
This is a pre-existing issue, but is there a data race when clearing
CAN_RTR_FLAG for RX_RTR_FRAME operations?

In bcm_rx_setup(), just above this block, it handles RX_RTR_FRAME:

	if (op->flags & RX_RTR_FRAME) {
		struct canfd_frame *frame0 = op->frames;
		...
		if ((op->flags & TX_CP_CAN_ID) ||
		    (frame0->can_id == op->can_id))
			frame0->can_id = op->can_id & ~CAN_RTR_FLAG;
	} else {
		if (op->flags & SETTIMER) {

The frame0->can_id modification happens after bcm_rx_update_lock is already
unlocked.

Concurrently, bcm_rx_handler() checks op->flags & RX_RTR_FRAME and calls
bcm_can_tx(), which reads op->frames to send a reply. Could bcm_can_tx()
transmit the frame with CAN_RTR_FLAG still set before it is cleared here,
potentially triggering an infinite loopback storm?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260612-bcm_fixes-v1-0-ca2fa07ee70f@hartkopp.net?part=3

[PATCH 4/5] 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 fc5158132b55..f1f1f11d1f92 100644
--- a/net/can/bcm.c
+++ b/net/can/bcm.c
@@ -110,11 +110,11 @@ struct bcm_op {
 	struct rcu_head rcu;
 	struct work_struct work;
 	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;
@@ -227,14 +227,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));
 
@@ -252,13 +255,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);
 	}
 
@@ -278,20 +281,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)
@@ -338,11 +351,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)
@@ -491,16 +504,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));
@@ -737,11 +747,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 5/5] 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 | 17 ++++++++++-------
 1 file changed, 10 insertions(+), 7 deletions(-)

diff --git a/net/can/bcm.c b/net/can/bcm.c
index f1f1f11d1f92..6a1d52189c0a 100644
--- a/net/can/bcm.c
+++ b/net/can/bcm.c
@@ -263,11 +263,11 @@ static int bcm_proc_show(struct seq_file *m, void *v)
 
 		seq_printf(m, "%s%ld%%\n",
 			   (reduction == 100) ? "near " : "", reduction);
 	}
 
-	list_for_each_entry(op, &bo->tx_ops, list) {
+	list_for_each_entry_rcu(op, &bo->tx_ops, list) {
 
 		seq_printf(m, "tx_op: %03X %s ", op->can_id,
 			   bcm_proc_getifname(net, ifname, op->ifindex));
 
 		if (op->flags & CAN_FD_FRAME)
@@ -969,10 +969,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)
@@ -1109,12 +1110,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 */
@@ -1132,10 +1132,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);
@@ -1293,13 +1297,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))) */
 
@@ -1374,14 +1375,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 5/5] can: bcm: use list_add_rcu() in bcm_[rx|tx]_setup()

[sashiko-bot]

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

Pre-existing issues:
- [Critical] Lockless update of `op->frames` in `bcm_tx_setup()` creates a TOCTOU race allowing transmission of unvalidated CAN frames, leading to Out-of-Bounds reads in CAN drivers.
- [Critical] Complete lack of frame length validation in `bcm_rx_setup()` allows Out-of-Bounds reads when responding to RTR frames.
- [High] Lockless updates to 64-bit ktime_t timer variables in `bcm_tx_setup()` lead to torn reads and soft lockups on 32-bit platforms.
- [High] Objects are passed to call_rcu() while still linked in the RCU-protected tx_ops and rx_ops lists during socket release.
--

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

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

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 f1f1f11d1f92a..6a1d52189c0ab 100644
> --- a/net/can/bcm.c
> +++ b/net/can/bcm.c

[ ... ]

> @@ -1111,8 +1112,7 @@ static int bcm_tx_setup(struct bcm_msg_head *msg_head, struct msghdr *msg,
>  		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))) */

[Severity: Critical]
This is a pre-existing issue, but does this lockless update of op->frames
create a race condition?

In bcm_tx_setup(), user data is copied directly into the active array:

err = memcpy_from_msg((u8 *)cf, msg, op->cfsiz);

The validation (cf->len > 64 or > 8) happens after this copy without
holding op->bcm_tx_lock or disabling the active timer.

Could bcm_tx_timeout_handler() fire during this window, causing
bcm_can_tx() to read and transmit a partially updated or unvalidated
frame? If an attacker supplies an iovec that faults immediately after
the length byte is copied, leaving a malicious length (like 255), does
this cause an out-of-bounds read in the CAN driver when the timer
transmits the frame?

[Severity: High]
This is a pre-existing issue, but do the lockless updates to 64-bit timer
variables lead to torn reads on 32-bit platforms?

In bcm_tx_setup(), op->kt_ival1 and op->kt_ival2 are updated based on
user input without holding op->bcm_tx_lock or disabling softirqs.

Since bcm_tx_timeout_handler() reads these variables in softirq context:

if (op->kt_ival1 && (op->count > 0)) {
    ...
} else if (op->kt_ival2) {

Could a softirq interrupting the update read a torn ktime_t value (a mix
of old and new 32-bit halves) on 32-bit systems? Would passing this
garbage value to hrtimer_set_expires() result in the timer firing
continuously or hanging indefinitely?

> @@ -1134,6 +1134,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: High]
This is a pre-existing issue, but does the teardown loop in bcm_release()
pass objects to call_rcu() while they are still linked in the RCU-protected
lists?

While this patch correctly updates list_add_rcu() for bo->tx_ops and
bo->rx_ops, bcm_release() iterates over these lists with
list_for_each_entry_safe() and calls bcm_remove_op().

Since bcm_remove_op() immediately calls call_rcu(&op->rcu, bcm_free_op_rcu),
the elements are never unlinked using list_del_rcu() before the call_rcu()
invocation.

Does leaving these objects physically linked in the RCU-protected list
violate the remove-before-reclaim ordering rule, potentially allowing
new readers to find the object after the grace period begins?

[ ... ]

> @@ -1376,10 +1377,12 @@ static int bcm_rx_setup(struct bcm_msg_head *msg_head, struct msghdr *msg,
>  					      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);
>  	}

[Severity: Critical]
This is a pre-existing issue, but does the lack of frame length validation
in bcm_rx_setup() allow out-of-bounds reads during RTR replies?

When CAN frames are copied from user-space into op->frames via
memcpy_from_msg() in bcm_rx_setup(), the length field is never validated.

If the user configures the RX operation with the RX_RTR_FRAME flag,
bcm_rx_handler() automatically replies to incoming RTR packets:

if (op->flags & RX_RTR_FRAME) {
    if (rx_ext->skb->sk != op->sk)
        bcm_can_tx(op);
    return;
}

Could an attacker set an arbitrary length (e.g., 255) for
op->frames[0].len, causing bcm_can_tx() to send an unvalidated frame
and trigger an out-of-bounds heap read in the driver?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260612-bcm_fixes-v1-0-ca2fa07ee70f@hartkopp.net?part=5