Re: [PATCH v2 5/6] can: bcm: add missing rcu list annotations and operations

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Concurrent updates to CAN frames and timers in bcm_tx_setup() race with bcm_tx_timeout_handler() and bcm_can_tx().
--

--- Patch [5]: [PATCH v2 5/6] can: bcm: add missing rcu list annotations and operations ---
commit 6cbc186daf031268589b88c789ab291e31165918
Author: Oliver Hartkopp <socketcan@hartkopp.net>

can: bcm: add missing rcu list annotations and operations

This commit adds missing list_add_rcu() calls in bcm_tx_setup() and
bcm_rx_setup() to ensure proper initialization of bcm_op structures during
RCU traversal. It also fixes the removal side in bcm_release() by properly
unlinking ops with list_del_rcu() before freeing them via call_rcu().

Closes: https://lore.kernel.org/linux-can/20260610094654.A1FFE1F00893@smtp.kernel.org/

> diff --git a/net/can/bcm.c b/net/can/bcm.c
> index 20b1bdc87414a..3609178a1367d 100644
> --- a/net/can/bcm.c
> +++ b/net/can/bcm.c
[ ... ]
> @@ -1156,6 +1156,10 @@ static int bcm_tx_setup(struct bcm_msg_head *msg_head, struct msghdr *msg,
>  		op->flags |= TX_ANNOUNCE;
>  	}
>  
> +	/* add this bcm_op to the list of the tx_ops? */
> +	if (add_op_to_list)
> +		list_add_rcu(&op->list, &bo->tx_ops);
> +

[Severity: High]
This is a pre-existing issue, but can concurrent updates to CAN frames and
timers in bcm_tx_setup() race with bcm_tx_timeout_handler() and
bcm_can_tx()?

While a previous commit in the series fixed this exact issue for RX ops by
using a staging buffer and lock, the TX path appears to be left unprotected.

When an existing BCM TX op is running its cyclic transmission via the
hrtimer bcm_tx_timeout_handler(), and user-space calls sendmsg() with
TX_SETUP to update the op's frames or timers, bcm_tx_setup() directly
copies new data into op->frames using memcpy_from_msg() without holding
bcm_tx_lock or using a staging buffer. It also updates op->kt_ival1 and
op->kt_ival2 without locking.

Concurrently, bcm_tx_timeout_handler() reads the unprotected timer values
and invokes bcm_can_tx().

bcm_can_tx() acquires bcm_tx_lock only to resolve the cf pointer to the
current frame, but drops the lock before calling skb_put_data().

Could this allow skb_put_data(skb, cf, op->cfsiz) to copy the frame payload
out of op->frames simultaneously with memcpy_from_msg() writing to it,
resulting in a torn read that transmits corrupt or partially updated CAN
frames onto the network?

>  	if (op->flags & TX_ANNOUNCE)
>  		bcm_can_tx(op, NULL);
>

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260707-bcm_fixes-v2-0-96620c534b75@hartkopp.net?part=5