Re: [PATCH v13 03/11] can: bcm: add locking when updating filter and timer values

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] Dropping bcm_rx_update_lock in bcm_rx_handler creates a TOCTOU race that can erroneously process a received frame as a filter and restart timers for an RX_RTR_FRAME operation.
--

--- Patch [3]: [PATCH v13 03/11] can: bcm: add locking when updating filter and timer values ---
commit 72b0ae7b8598826552a4ceb6840f50d02ba50085
Author: Oliver Hartkopp <[email protected]>

can: bcm: add locking when updating filter and timer values
    
This commit adds a new bcm_rx_update_lock to protect timer and filter
updates from concurrent access in bcm_rx_setup() and various RX handlers.
It also fixes a race by normalizing the RTR flag on the staged buffer
before registering the frames.

Link: https://lore.kernel.org/linux-can/[email protected]/

> 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

[ ... ]

> @@ -729,9 +751,18 @@ static void bcm_rx_handler(struct sk_buff *skb, void *data)
>  	/* update statistics */
>  	op->frames_abs++;
>  
> -	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);
> +
> +	if (rtr_frame) {
>  		/* send reply for RTR-request (placed in op->frames[0]) */
> -		bcm_can_tx(op);
> +		bcm_can_tx(op, &rtrframe);
>  		return;
>  	}
>  
> @@ -743,6 +774,8 @@ static void bcm_rx_handler(struct sk_buff *skb, void *data)
>  			traffic_flags |= RX_OWN;
>  	}
>  
> +	spin_lock_bh(&op->bcm_rx_update_lock);
> +

[Severity: Medium]
Could this lock split cause a race condition if a concurrent bcm_rx_setup()
updates the operation to an RX_RTR_FRAME?

If bcm_rx_handler() initially sees rtr_frame == false and drops the lock:

CPU1 (bcm_rx_handler)
    spin_lock_bh(&op->bcm_rx_update_lock);
    rtr_frame = op->flags & RX_RTR_FRAME; // false
    spin_unlock_bh(&op->bcm_rx_update_lock);

CPU2 (bcm_rx_setup)
    spin_lock_bh(&op->bcm_rx_update_lock);
    op->flags |= RX_RTR_FRAME;
    // updates op->frames but does not clear op->kt_ival1
    spin_unlock_bh(&op->bcm_rx_update_lock);

CPU1 (bcm_rx_handler)
    spin_lock_bh(&op->bcm_rx_update_lock);
    // Proceeds down the non-RTR path with op->flags now having RX_RTR_FRAME set
    ...
    bcm_rx_starttimer(op);

When bcm_rx_handler() reacquires the lock and proceeds along the non-RTR path,
it will incorrectly compare the received frame against the new RTR reply frame,
potentially sending a spurious RX_CHANGED.

Furthermore, since op->kt_ival1 is not cleared when switching to RTR mode, could
this erroneously start a timer for an RTR operation and result in a spurious
RX_TIMEOUT?

>  	if (op->flags & RX_FILTER_ID) {
>  		/* the easiest case */
>  		bcm_rx_update_and_send(op, op->last_frames, rxframe,
> @@ -778,6 +811,8 @@ static void bcm_rx_handler(struct sk_buff *skb, void *data)
>  
>  rx_starttimer:
>  	bcm_rx_starttimer(op);
> +
> +	spin_unlock_bh(&op->bcm_rx_update_lock);
>  }

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