Re: [PATCH v3 2/7] can: bcm: fix lockless bound/ifindex race and silent RX_SETUP failure

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Unprotected and non-RCU-safe list manipulations on `bo->tx_ops` and `bo->rx_ops` cause data races with lockless procfs readers.
- [High] Concurrent update of `op->frames` causes a data race with softirq CAN TX processing, sending corrupted frames.
- [Medium] Concurrent update of `op->frames` causes a data race with softirq CAN RX processing.
--

--- Patch [2]: [PATCH v3 2/7] can: bcm: fix lockless bound/ifindex race and silent RX_SETUP failure ---
commit 8b8711c2150aeadcb80c48a64d2a2f7206bc9502
Author: Oliver Hartkopp <socketcan@hartkopp.net>

can: bcm: fix lockless bound/ifindex race and silent RX_SETUP failure

This commit fixes a race condition where bcm_sendmsg() could read an
inconsistent bo->bound and bo->ifindex state during concurrent socket
operations. It addresses this by moving the ifindex read and bound validation
inside lock_sock(). The patch also fixes bcm_rx_setup() to correctly return
-ENODEV instead of silently succeeding when a device disappears concurrently.

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

> diff --git a/net/can/bcm.c b/net/can/bcm.c
> index bdf53241bd7b3..b612135b017d1 100644
> --- a/net/can/bcm.c
> +++ b/net/can/bcm.c
[ ... ]
> @@ -1323,6 +1323,11 @@ static int bcm_rx_setup(struct bcm_msg_head *msg_head, struct msghdr *msg,
>  
>  				op->rx_reg_dev = dev;
>  				dev_put(dev);
> +			} else {
> +				/* the requested device is gone - do not
> +				 * silently succeed without registering
> +				 */
> +				err = -ENODEV;
>  			}

[Severity: Medium]
This isn't a bug introduced by this patch, but does the concurrent update of
op->frames cause a data race with softirq CAN RX processing?

In bcm_rx_setup(), when updating an existing RX filter, the code copies from
userspace without excluding the softirq path:

bcm_rx_setup() {
    ...
    err = memcpy_from_msg(op->frames, msg, msg_head->nframes * op->cfsiz);
    ...
}

Since memcpy_from_msg() can sleep and the CAN RX softirq runs concurrently,
bcm_rx_cmp_to_index() might execute mid-update. It locklessly reads op->frames
and op->last_frames to match multiplexed CAN frames:

bcm_rx_cmp_to_index() {
    ...
    if ((get_u64(cf, i) & get_u64(rxdata, i)) != (get_u64(cf, i) & get_u64(lcf, i))) {
    ...
}

Can this lead to torn reads where the BCM spuriously triggers or fails to
trigger filter updates based on corrupted matching data?

[ ... ]
> @@ -1436,20 +1463,21 @@ static int bcm_sendmsg(struct socket *sock, struct msghdr *msg, size_t size)
[ ... ]
>  	switch (msg_head.opcode) {
>  
>  	case TX_SETUP:

[Severity: High]
This is a pre-existing issue, but do these list insertions lack RCU write
barriers?

bcm_proc_show() iterates bo->tx_ops and bo->rx_ops locklessly using
rcu_read_lock(). However, bcm_tx_setup() and bcm_rx_setup() insert new
elements using list_add() instead of list_add_rcu(), which lacks the
smp_store_release() write barrier required to safely publish initialized
bcm_op structs to concurrent RCU readers.

Additionally, bcm_proc_show() appears to use the non-RCU list_for_each_entry()
macro for bo->tx_ops, omitting the required read barrier. Could an unprivileged
local user reading /proc/net/can-bcm/* concurrently trigger a panic?

[Severity: High]
This is another pre-existing issue, but does a similar concurrent update of
op->frames cause a data race with softirq CAN TX processing?

In bcm_tx_setup(), when updating an existing TX operation, memcpy_from_msg()
overwrites the payload in op->frames without acquiring op->bcm_tx_lock or
excluding the softirq:

bcm_tx_setup() {
    ...
    err = memcpy_from_msg((u8 *)cf, msg, op->cfsiz);
    ...
}

Concurrently, the hrtimer callback bcm_tx_timeout_handler() can fire and invoke
bcm_can_tx(). bcm_can_tx() retrieves the frame pointer under the lock, but
accesses the underlying frame data locklessly outside the lock:

bcm_can_tx() {
    ...
    skb_put_data(skb, cf, op->cfsiz);
    ...
}

Could this allow the softirq to transmit a partially updated or torn CAN frame
onto the physical CAN bus?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260707-bcm_fixes-v3-0-84600ca6d889@hartkopp.net?part=2