Re: [PATCH v6 3/9] can: bcm: add locking when updating filter and timer values

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Torn reads of 64-bit ktime_t variables and op->count in bcm_tx_timeout_handler.
- [Medium] Unprotected state modifications in bcm_rx_handler lead to data races when listening on all interfaces.
--

--- Patch [3]: [PATCH v6 3/9] can: bcm: add locking when updating filter and timer values ---
commit 27c469169060f50f95adf2e7ea3b4871de414a2e
Author: Oliver Hartkopp <socketcan@hartkopp.net>

    can: bcm: add locking when updating filter and timer values
    
    KCSAN detected a simultaneous access to timer values that can be
    overwritten in bcm_rx_setup() when updating timer and filter content
    while bcm_rx_handler(), bcm_rx_timeout_handler() or bcm_rx_thr_handler()
    run concurrently on incoming CAN traffic.
    
    Link: https://lore.kernel.org/linux-can/6975d5cf.a00a0220.33ccc7.0022.GAE@google.com/

> diff --git a/net/can/bcm.c b/net/can/bcm.c
> index b612135b017d1..af4bb4182bc29 100644
> --- a/net/can/bcm.c
> +++ b/net/can/bcm.c
> @@ -461,7 +466,7 @@ static enum hrtimer_restart bcm_tx_timeout_handler(struct hrtimer *hrtimer)
>  	struct bcm_msg_head msg_head;
>  
>  	if (op->kt_ival1 && (op->count > 0)) {
> -		bcm_can_tx(op);
> +		bcm_can_tx(op, NULL);
>  		if (!op->count && (op->flags & TX_COUNTEVT)) {

[Severity: High]
This is a pre-existing issue, but does bcm_tx_timeout_handler() read
kt_ival1, kt_ival2, and count without holding bcm_tx_lock?

A user application could concurrently update timer values via
bcm_sendmsg(TX_SETUP | SETTIMER) while the active hrtimer callback
bcm_tx_timeout_handler() executes in softirq context. This could result in a
torn read of the 64-bit ktime_t timeout values on 32-bit systems,
potentially causing incorrect timer expiry times. Data races on op->count
can also cause TOCTOU bugs resulting in missed or extra transmissions.

In bcm_tx_setup(), the timer interval values appear to be written without
acquiring bcm_tx_lock:

	if (op->flags & SETTIMER) {
		/* set timer values */
		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);

[ ... ]

> @@ -729,9 +751,18 @@ static void bcm_rx_handler(struct sk_buff *skb, void *data)
>  	/* update statistics */
>  	op->frames_abs++;

[Severity: Medium]
This isn't a bug introduced by this patch, but can op->rx_stamp,
op->rx_ifindex, and op->frames_abs be updated concurrently on multiple CPUs
before bcm_rx_update_lock is acquired?

If a BCM socket is bound to all interfaces (ifindex = 0) and multiple CAN
interfaces receive frames matching the filter simultaneously,
bcm_rx_handler() can run concurrently on multiple CPUs for the same bcm_op.
Could this lead to lost statistic increments, interleaved metadata, or a
torn read of the 64-bit rx_stamp on 32-bit systems?

> -	if (op->flags & RX_RTR_FRAME) {
> +	/* snapshot the flag under lock: op->flags/op->frames may be updated
> +	 * concurrently by bcm_rx_setup().
> +	 */
> +	spin_lock_bh(&op->bcm_rx_update_lock);
> +	rtr_frame = op->flags & RX_RTR_FRAME;
> +	if (rtr_frame)
> +		memcpy(&rtrframe, op->frames, op->cfsiz);
> +	spin_unlock_bh(&op->bcm_rx_update_lock);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260708-bcm_fixes-v6-0-80710a2bc398@hartkopp.net?part=3