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

[PATCH can-next] 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 a4bef2c48a55..57c2ac2cec7d 100644
--- a/net/can/bcm.c
+++ b/net/can/bcm.c
@@ -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);
-- 
2.53.0

Re: [PATCH can-next] 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] Lockless TOCTOU race in bcm_sendmsg() leads to unintended Global CAN Filter Leak.
--

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

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/

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

> diff --git a/net/can/bcm.c b/net/can/bcm.c
> index fd9fa072881e2..6c93c57212b49 100644
> --- a/net/can/bcm.c
> +++ b/net/can/bcm.c
> @@ -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?

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