Re: [PATCH 2/5] 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] 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