Re: [PATCH v4 5/7] 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] Updating an existing TX operation in `bcm_tx_setup()` directly modifies `op->frames` and 64-bit timers without holding `op->bcm_tx_lock`, creating a data race and potential state corruption.
--

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

can: bcm: add missing rcu list annotations and operations

This patch adds missing list_add_rcu() and list_del_rcu() operations
when setting up or releasing BCM operations. It ensures proper
RCU list initialization to safely support bcm_proc_show() traverses.

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

> diff --git a/net/can/bcm.c b/net/can/bcm.c
> index 0b53dbb64dd19..5ca684cd08194 100644
> --- a/net/can/bcm.c
> +++ b/net/can/bcm.c

[ ... ]

> @@ -1143,8 +1144,7 @@ static int bcm_tx_setup(struct bcm_msg_head *msg_head, struct msghdr *msg,
>  		hrtimer_setup(&op->thrtimer, hrtimer_dummy_timeout, CLOCK_MONOTONIC,
>  			      HRTIMER_MODE_REL_SOFT);
>  
> -		/* add this bcm_op to the list of the tx_ops */
> -		list_add(&op->list, &bo->tx_ops);
> +		add_op_to_list = true;
>  
>  	} /* if ((op = bcm_find_op(&bo->tx_ops, msg_head->can_id, ifindex))) */
>  

[Severity: High]
This is a pre-existing issue, but does bcm_tx_setup() safely update
existing TX operations?

It appears that bcm_tx_setup() updates existing operations by directly
overwriting live CAN frames (op->frames) via memcpy_from_msg() without
holding op->bcm_tx_lock:

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

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

		if (err < 0)
			return err;
    ...
}

If the updated data violates CAN constraints (e.g., cf->len > 64 or a memcpy
failure), it returns early. However, the existing active op continues to
run and transmit via bcm_tx_timeout_handler(). Can this lead to the blind
cyclic broadcast of corrupted or oversized CAN frames on the bus?

Additionally, 64-bit timers are updated without locking right below this
block, which could cause data races and torn reads on 32-bit platforms
with bcm_tx_timeout_handler():

net/can/bcm.c:bcm_tx_setup() {
    ...
		op->ival1 = msg_head->ival1;
		op->ival2 = msg_head->ival2;
		op->kt_ival1 = bcm_timeval_to_ktime(msg_head->ival1);
		op->kt_ival2 = bcm_timeval_to_ktime(msg_head->ival2);
    ...
}

Could these updates be guarded by op->bcm_tx_lock to prevent state
corruption?

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