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

PATCH In review Oliver Hartkopp 2 participants 3 messages Jun 1 – Jun 10

Subsystems (2)

CAN network drivers CAN network layer

lore

Share & tools

lore

Oliver Hartkopp

author maintainer

2026-06-01 18:19 UTC #0 lore raw

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

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

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

Signed-off-by Oliver Hartkopp <[email protected]>

---
 net/can/bcm.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

net/can/bcm.c +3 -2

index a4bef2c48a55..57c2ac2cec7d 100644
@@ -1376,11 +1376,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;

@@ -1510,11 +1511,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);

Signature · 1 line

2.53.0

[email protected]

AI review

2026-06-09 18:51 UTC #1 lore raw

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

Pre-existing issues:
- [High] Lockless TOCTOU race in bcm_sendmsg() leads to unintended Global CAN Filter Leak. --

commit 7f7ce41fd35425d40c8342127156c8195ff2363e
Author: Oliver Hartkopp <[email protected]>

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

The commit marks the lockless read of bo->bound in bcm_sendmsg() and the corresponding write in bcm_notify() with READ_ONCE() and WRITE_ONCE() to resolve KCSAN warnings. It asserts this lockless read is functionally safe and will safely drop downstream operations.

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

Quoted 9 lines

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.

[Severity: High]
Is this description accurate regarding safely failing and preventing the registration of stale CAN filters?

This is a pre-existing issue, but if bcm_notify() unregisters the device concurrently, couldn't a race occur where bcm_sendmsg() caches ifindex == 0 while still observing bo->bound == 1?

Since bo->ifindex is a plain read at the top of bcm_sendmsg(), the compiler can defer it until after the volatile read of bo->bound and the potentially sleeping memcpy_from_msg().

If this happens, bcm_sendmsg() could observe bo->bound == 1, but cache bo->ifindex == 0.

Quoted 5 lines

net/can/bcm.c +0 -0

index fd9fa072881e2..6c93c57212b49 100644

net/can/bcm.c static int bcm_sendmsg(struct socket *sock, struct msghdr *msg, size_t size) · line 1386

Quoted 9 lines

net/can/bcm.c +2 -1

@@ -1379,7 +1379,8 @@ static int bcm_sendmsg(struct socket *sock, struct msghdr *msg, size_t size)
 	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 */

[Severity: High]
This isn't a bug introduced by this patch, but doesn't bcm_sendmsg() then pass the cached ifindex == 0 to bcm_rx_setup() under the socket lock?

net/can/bcm.c:bcm_sendmsg() {
    ...
    lock_sock(sk);

    switch (msg_head.opcode) {
    ...
    case RX_SETUP:
        ret = bcm_rx_setup(&msg_head, msg, ifindex, sk);
        break;
    ...
}

Does bcm_rx_setup() ever re-validate bo->bound while holding the lock?

If it doesn't, wouldn't bcm_rx_setup() interpret ifindex == 0 as an explicit request to listen on all CAN interfaces?

net/can/bcm.c:bcm_rx_setup() {
    ...
    } else
        err = can_rx_register(sock_net(sk), NULL, op->can_id,
                              REGMASK(op->can_id),
                              bcm_rx_handler, op, "bcm", sk);
    ...
}

Could this actively register a global filter on an unbound socket, leaking traffic from all CAN buses into a process that was only authorized to monitor a specific, now-deleted interface?

Signature · 1 line

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

Oliver Hartkopp

author maintainer

2026-06-10 07:35 UTC #2 lore raw

Quoted 28 lines · On 09.06.26 20:51, [email protected] wrote:

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
Pre-existing issues:
- [High] Lockless TOCTOU race in bcm_sendmsg() leads to unintended Global CAN Filter Leak. --
commit 7f7ce41fd35425d40c8342127156c8195ff2363e
Author: Oliver Hartkopp <[email protected]>
can: bcm: mark intentional lockless read of bo->bound for KCSAN
The commit marks the lockless read of bo->bound in bcm_sendmsg() and the corresponding write in bcm_notify() with READ_ONCE() and WRITE_ONCE() to resolve KCSAN warnings. It asserts this lockless read is functionally safe and will safely drop downstream operations.
Closes https://lore.kernel.org/linux-can/CAGp+u1aBK8QVjsvAxM2Ldzep4rEbsP9x_pV3At4g=h1kVEtyhA@mail.gmail.com/ open here
Quoted 9 lines
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.

Quoted 7 lines

[Severity: High]
Is this description accurate regarding safely failing and preventing the registration of stale CAN filters?
This is a pre-existing issue, but if bcm_notify() unregisters the device concurrently, couldn't a race occur where bcm_sendmsg() caches ifindex == 0 while still observing bo->bound == 1?

No. bcm_notify() can only be invoked for real interfaces which have an ifindex > 0

Quoted 57 lines

Since bo->ifindex is a plain read at the top of bcm_sendmsg(), the compiler can defer it until after the volatile read of bo->bound and the potentially sleeping memcpy_from_msg().
If this happens, bcm_sendmsg() could observe bo->bound == 1, but cache bo->ifindex == 0.
Quoted 5 lines
net/can/bcm.c +0 -0
index fd9fa072881e2..6c93c57212b49 100644
net/can/bcm.c static int bcm_sendmsg(struct socket *sock, struct msghdr *msg, size_t size) · line 1386
Quoted 9 lines
net/can/bcm.c +2 -1
@@ -1379,7 +1379,8 @@ static int bcm_sendmsg(struct socket *sock, struct msghdr *msg, size_t size)
  	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 */
[Severity: High]
This isn't a bug introduced by this patch, but doesn't bcm_sendmsg() then pass the cached ifindex == 0 to bcm_rx_setup() under the socket lock?
net/can/bcm.c:bcm_sendmsg() {
     ...
     lock_sock(sk);

     switch (msg_head.opcode) {
     ...
     case RX_SETUP:
         ret = bcm_rx_setup(&msg_head, msg, ifindex, sk);
         break;
     ...
}
Does bcm_rx_setup() ever re-validate bo->bound while holding the lock?
If it doesn't, wouldn't bcm_rx_setup() interpret ifindex == 0 as an explicit request to listen on all CAN interfaces?
net/can/bcm.c:bcm_rx_setup() {
     ...
     } else
         err = can_rx_register(sock_net(sk), NULL, op->can_id,
                               REGMASK(op->can_id),
                               bcm_rx_handler, op, "bcm", sk);
     ...
}
Could this actively register a global filter on an unbound socket, leaking traffic from all CAN buses into a process that was only authorized to monitor a specific, now-deleted interface?