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

[sashiko-bot@kernel.org]

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