Re: [PATCH v7 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 3 potential issue(s) to consider:

Pre-existing issues:
- [High] Missing RCU memory barriers for tx_ops and rx_ops list insertions and iterations, risking unhandled page faults in procfs.
- [High] Unsynchronized updates to active BCM RX operations can corrupt CAN frame filtering.
- [High] Unsynchronized updates to active BCM TX operations can transmit torn CAN frames.
--

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

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. This commit fixes a race
where bcm_sendmsg() could observe an inconsistent combination of these
fields. It also ensures bcm_rx_setup() reports -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 can unsynchronized updates to active
BCM RX operations corrupt CAN frame filtering?

When updating an existing BCM RX operation, bcm_rx_setup() in net/can/bcm.c
copies data from user space directly into op->frames via:

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

This happens without disabling softirqs or taking a lock that the softirq
handler respects. Concurrently, bcm_rx_handler() runs in NET_RX_SOFTIRQ
context and accesses these fields locklessly to evaluate incoming CAN
frames or generate RTR replies.

Does this data race allow the softirq to observe torn data, resulting in
corrupted CAN frame filtering or malformed RTR replies?

[Severity: High]
This is a pre-existing issue, but do bcm_rx_setup() and bcm_tx_setup()
need RCU release barriers when inserting into bo->rx_ops and bo->tx_ops?

In net/can/bcm.c, new operations are inserted using list_add() instead
of list_add_rcu():

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

Concurrently, bcm_proc_show() iterates through these lists. For rx_ops,
it uses list_for_each_entry_rcu() (but the insertion lacked a release
barrier). For tx_ops, it iterates inside rcu_read_lock() using:

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

which lacks an acquire barrier. Since list_add() doesn't execute a write
memory barrier, can concurrent procfs readers observe the newly linked
node before its internal pointers are fully initialized, potentially
dereferencing garbage pointers and causing an unhandled page fault?

[ ... ]

> @@ -1396,12 +1401,13 @@ static int bcm_sendmsg(struct socket *sock, struct msghdr *msg, size_t size)
>  {
>  	struct sock *sk = sock->sk;
>  	struct bcm_sock *bo = bcm_sk(sk);
> -	int ifindex = bo->ifindex; /* default ifindex for this bcm_op */
> +	int ifindex;
>  	struct bcm_msg_head msg_head;
>  	int cfsiz;
>  	int ret; /* read bytes or error codes as return value */
>  
> -	if (!bo->bound)
> +	/* Lockless fast-path check for bound socket */
> +	if (!READ_ONCE(bo->bound))
>  		return -ENOTCONN;
>  
>  	/* check for valid message length from userspace */

[Severity: High]
This is a pre-existing issue, but can unsynchronized updates to active
BCM TX operations cause the transmission of torn CAN frames?

When bcm_sendmsg() delegates to bcm_tx_setup() in net/can/bcm.c to update
an existing BCM TX operation, bcm_tx_setup() copies data from user space
into op->frames without acquiring op->bcm_tx_lock:

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

Concurrently, the transmission timer may fire and execute bcm_can_tx().
It calculates the frame pointer under the lock but drops the lock before
copying the frame data locklessly into the SKB:

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

Does this data race allow a partially written (torn) CAN frame to be
constructed and transmitted onto the CAN bus, causing data corruption
on the network?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260708-bcm_fixes-v7-0-c78896b342e4@hartkopp.net?part=2