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

On 14.07.26 12:27, [email protected] wrote:
> 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.
> --

Fixed in patch 4

> 
> --- 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);
>>   }
>