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

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

Pre-existing issues:
- [High] Data race when updating CAN frames locklessly allows transmission of torn or partially updated frames and incorrect RX filtering.
--

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

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

This commit fixes a race condition where bcm_sendmsg() reads bo->ifindex
and checks bo->bound locklessly while concurrent updates modify them,
preventing inconsistent socket bindings. It also ensures bcm_rx_setup()
returns an error when a requested device is concurrently removed instead of
silently failing.

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 isn't a bug introduced by this patch, but is there a data race when
updating CAN frames locklessly?

Looking at bcm_tx_setup(), it writes directly to op->frames using data from
userspace without holding any lock to synchronize with the timer:

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

Could the timer function bcm_can_tx() locklessly read op->frames and
transmit torn data if it races with bcm_tx_setup()?

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

Similarly, bcm_rx_setup() updates RX filters locklessly:

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

Does this allow the RX softirq handler to evaluate incoming CAN frames against
partially written filters in bcm_rx_handler()?

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

Since memcpy_from_msg() can sleep on page faults, the data might be in a
partially written state for an extended period. Is it possible to copy the
data to a temporary buffer first, and then update op->frames under the
appropriate spinlocks?

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