Re: [PATCH v10 02/10] 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 2 potential issue(s) to consider:

Pre-existing issues:
- [High] Missing RCU protection and memory barriers for `tx_ops` and `rx_ops` list modifications and traversals.
- [High] Lockless modification of CAN frames and filters from process context while they are concurrently read by softirq handlers.
--

--- Patch [2]: [PATCH v10 02/10] can: bcm: fix lockless bound/ifindex race and silent RX_SETUP failure ---
commit 14fe77d5df361ef7f02ad605d5649032b1cb3565
Author: Oliver Hartkopp <socketcan@hartkopp.net>

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

This commit fixes an unordered read/write race in bcm_sendmsg() between
bo->bound and bo->ifindex by moving the re-check into the locked section.
It also ensures bcm_rx_setup() returns an error if the target device
disappears concurrently instead of silently succeeding.

Link: 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
[ ... ]
> @@ -1436,20 +1463,21 @@ static int bcm_sendmsg(struct socket *sock, struct msghdr *msg, size_t size)
>  			struct net_device *dev;
>  
>  			dev = dev_get_by_index(sock_net(sk), ifindex);
> -			if (!dev)
> -				return -ENODEV;
> +			if (!dev) {
> +				ret = -ENODEV;
> +				goto out_release;
> +			}
>  
>  			if (dev->type != ARPHRD_CAN) {
>  				dev_put(dev);
> -				return -ENODEV;
> +				ret = -ENODEV;
> +				goto out_release;
>  			}
>  
>  			dev_put(dev);
>  		}
>  	}
>  
> -	lock_sock(sk);
> -
>  	switch (msg_head.opcode) {
>  
>  	case TX_SETUP:

[Severity: High]
This is a pre-existing issue, but does this lack of RCU protection and memory
barriers allow a lockless reader to observe uninitialized fields?

Looking at bcm_proc_show(), it traverses bo->tx_ops locklessly under
rcu_read_lock():

    list_for_each_entry(op, &bo->tx_ops, list) {

Concurrently, bcm_tx_setup() can add a new operation to this list:

    list_add(&op->list, &bo->tx_ops);

Because list_add() lacks the smp_store_release() memory barrier provided
by list_add_rcu(), and the traversal uses the non-RCU
list_for_each_entry() macro, can concurrent readers see torn pointers or
uninitialized bcm_op fields, potentially causing a kernel crash when
reading uninitialized memory?

[Severity: High]
This is a pre-existing issue, but does this lockless modification of CAN
frames allow softirq readers to see partially updated data?

In bcm_tx_setup() and bcm_rx_setup(), op->frames are modified directly
from process context:

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

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

Concurrently, softirq handlers read these frames locklessly. For example,
bcm_can_tx() reads frames from timer context to transmit:

    skb_put_data(skb, cf, op->cfsiz);

And bcm_rx_cmp_to_index() reads filter mask frames from RX softirq context:

    if ((get_u64(cf, i) & get_u64(rxdata, i)) != ...

Can this data race result in transmitting torn or functionally corrupted
CAN frames to the network bus, or incorrect evaluation of incoming packets
against partially updated filter masks?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260709-bcm_fixes-v10-0-57fff38e34ce@hartkopp.net?part=2