Re: [PATCH v11 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] Lockless data race in BCM operation updates corrupts active TX/RX CAN frames.
--

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

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

This commit fixes a race condition in bcm_sendmsg() by moving the bo->ifindex
read and bo->bound check under the socket lock. It also ensures bcm_rx_setup()
returns -ENODEV when the target device concurrently disappears.

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 is a pre-existing issue, but does bcm_rx_setup() modify the active
op->frames array locklessly on the RX side?

Looking at the earlier part of bcm_rx_setup():

    /* update CAN frames content */
    err = memcpy_from_msg(op->frames, msg,
                          msg_head->nframes * op->cfsiz);

Could this data race with the softirq reader bcm_rx_handler() while it is
actively processing frames?

[ ... ]

> @@ -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 also a pre-existing issue, but does bcm_tx_setup() write to the
active op->frames array locklessly before validating the frame length?

When TX_SETUP is called, bcm_tx_setup() copies user data:

    cf = op->frames + op->cfsiz * i;
    err = memcpy_from_msg((u8 *)cf, msg, op->cfsiz);

    if (op->flags & CAN_FD_FRAME) {
        if (cf->len > 64)
            err = -EINVAL;

If an invalid length is provided from userspace, it is written to op->frames
first. The function catches the error and returns -EINVAL, but the
corrupted frame remains in the array.

Concurrently, can the high-resolution timer callback bcm_can_tx() transmit
this corrupted frame?

bcm_can_tx() reads the frame contents outside of bcm_tx_lock:

    spin_lock_bh(&op->bcm_tx_lock);
    cf = op->frames + op->cfsiz * op->currframe;
    spin_unlock_bh(&op->bcm_tx_lock);

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

Could this pass an unvalidated cf->len down to the hardware driver and
potentially overflow device buffers? An attacker creating a cyclic
transmission via TX_SETUP, then issuing a concurrent TX_SETUP update with
an invalid length, might be able to trigger this condition.

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