Re: [PATCH v13 02/11] can: bcm: fix lockless bound/ifindex race and silent RX_SETUP failure

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

Pre-existing issues:
- [High] Unsafe lockless iteration of `bo->tx_ops` and `bo->rx_ops` in procfs, using `list_add()` instead of `list_add_rcu()` for list updates and `list_for_each_entry()` instead of `list_for_each_entry_rcu()`.
- [High] `bcm_rx_setup()` modifies an active `rx_op`'s data and state from process context without any synchronization with `bcm_rx_handler()` running in softirq context.
- [High] `bcm_tx_setup()` modifies an active `tx_op`'s frames and flags without holding `bcm_tx_lock`, racing with the softirq timer and transmission paths.
--

--- Patch [2]: [PATCH v13 02/11] can: bcm: fix lockless bound/ifindex race and silent RX_SETUP failure ---
commit 79233a73945b83eac56e8c17536d2f019c104f50
Author: Oliver Hartkopp <[email protected]>

    can: bcm: fix lockless bound/ifindex race and silent RX_SETUP failure
    
    bcm_sendmsg() reads bo->ifindex and checks bo->bound before taking
    lock_sock(), while bcm_notify(), bcm_connect() and bcm_release() all
    mutate both fields under that same lock. Because the lockless reads
    and the locked writes are unordered with respect to each other, a
    racing bcm_notify() (device unregister) or bcm_connect() (concurrent
    bind on another thread sharing the socket) can make bcm_sendmsg()
    observe an inconsistent combination, e.g. a stale bound=1 together
    with the now-cleared ifindex=0, silently turning a socket bound to a
    specific CAN interface into one that also matches "any" interface.

    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
> @@ -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;
>  			}
>  
>  		} else

[Severity: High]
This isn't a bug introduced by this patch, but does bcm_rx_setup() safely
modify an active rx_op's data and state from process context without
synchronization with bcm_rx_handler() running in softirq context?

bcm_rx_setup() directly overwrites op->frames via memcpy_from_msg() and
clears op->last_frames while only holding lock_sock(sk):

net/can/bcm.c:bcm_rx_setup() {
    ...
    err = memcpy_from_msg(op->frames, msg, msg_head->nframes * op->cfsiz);
    ...
}

Concurrently, bcm_rx_handler() executes locklessly in the softirq RX path
and reads these frames:

net/can/bcm.c:bcm_rx_handler() {
    ...
    if ((get_u64(op->frames, 0) & get_u64(rxframe, 0)) == ...
    ...
}

Since memcpy_from_msg() copies from userspace and can block or be preempted,
could the softirq observe partially updated frames, mismatched lengths, or
torn flags, leading to incorrect CAN message filtering and potential data
corruption?

[ ... ]
> @@ -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 bcm_tx_setup() modify an active
tx_op's frames and flags without holding bcm_tx_lock, racing with the softirq
timer and transmission paths?

When updating an existing tx op, bcm_tx_setup() calls memcpy_from_msg() to
update op->frames before acquiring bcm_tx_lock:

net/can/bcm.c:bcm_tx_setup() {
    ...
    err = memcpy_from_msg((u8 *)cf, msg, op->cfsiz);
    ...
}

Meanwhile, bcm_can_tx() drops the lock and calls skb_put_data() to copy the
frame into an skb:

net/can/bcm.c:bcm_can_tx() {
    ...
    skb_put_data(skb, cf, op->cfsiz);
    ...
}

Could this data race allow the softirq to transmit partially updated CAN
frames onto the bus? Because memcpy_from_msg() accesses userspace and might
block, the race window appears wide enough to result in deterministic
corruption of transmitted frames under concurrent updates.

[Severity: High]
This is also a pre-existing issue, but is it safe for bcm_proc_show() to
iterate over bo->tx_ops and bo->rx_ops locklessly in procfs, while
bcm_tx_setup() and bcm_rx_setup() use list_add() instead of list_add_rcu()?

bcm_tx_setup() and bcm_rx_setup() insert new operations using the plain
list_add() macro, which lacks the smp_store_release() memory barrier:

net/can/bcm.c:bcm_tx_setup() {
    ...
    list_add(&op->list, &bo->tx_ops);
    ...
}

bcm_proc_show() traverses tx_ops locklessly under rcu_read_lock(), but uses
the unsafe list_for_each_entry() macro entirely bypassing RCU read primitives:

net/can/bcm.c:bcm_proc_show() {
    ...
    list_for_each_entry(op, &bo->tx_ops, list) {
    ...
}

Could a concurrent procfs reader observe the list pointer before the bcm_op
structure is fully visible in memory, causing a dereference of uninitialized
memory or a crash?

>  		ret = bcm_tx_setup(&msg_head, msg, ifindex, sk);
>  		break;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=2