Re: [PATCH v14 06/11] can: bcm: extend bcm_tx_lock usage for data and timer updates

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

Pre-existing issues:
- [High] Preexisting TOCTOU race in `bcm_can_tx` causes CAN frame sequence corruption and skipped frames when an operation is updated concurrently.
--

--- Patch [6]: [PATCH v14 06/11] can: bcm: extend bcm_tx_lock usage for data and timer updates ---
commit 2415d37e8cc6682239aca2edf2ea4608c5e64f24
Author: Oliver Hartkopp <[email protected]>

can: bcm: extend bcm_tx_lock usage for data and timer updates

This commit stages new CAN frame content into a dynamically allocated
buffer to validate before acquiring the lock, and updates timer variables
under lock to fix a torn 64-bit ktime_t read on 32-bit platforms.

> diff --git a/net/can/bcm.c b/net/can/bcm.c
> index 940cd9df1cca3..d5ca5ca472f1f 100644
> --- a/net/can/bcm.c
> +++ b/net/can/bcm.c

[ ... ]

> @@ -1052,36 +1094,38 @@ static int bcm_tx_setup(struct bcm_msg_head *msg_head, struct msghdr *msg,
>  					err = -EINVAL;
>  			}
>  
> -			if (err < 0)
> +			if (err < 0) {
> +				kfree(new_frames);
>  				return err;
> +			}
>  
>  			if (msg_head->flags & TX_CP_CAN_ID) {
>  				/* copy can_id into frame */
>  				cf->can_id = msg_head->can_id;
>  			}
>  		}
> +
> +		spin_lock_bh(&op->bcm_tx_lock);
> +
> +		/* update CAN frames content */
> +		memcpy(op->frames, new_frames, msg_head->nframes * op->cfsiz);
> +
>  		op->flags = msg_head->flags;
>  
> -		/* only lock for unlikely count/nframes/currframe changes */
>  		if (op->nframes != msg_head->nframes ||
> -		    op->flags & TX_RESET_MULTI_IDX ||
> -		    op->flags & SETTIMER) {
> -
> -			spin_lock_bh(&op->bcm_tx_lock);
>  
> -			if (op->nframes != msg_head->nframes ||
> -			    op->flags & TX_RESET_MULTI_IDX) {
> -				/* potentially update changed nframes */
> -				op->nframes = msg_head->nframes;
> -				/* restart multiple frame transmission */
> -				op->currframe = 0;
> -			}
> +		    op->flags & TX_RESET_MULTI_IDX) {
> +			/* potentially update changed nframes */
> +			op->nframes = msg_head->nframes;
> +			/* restart multiple frame transmission */
> +			op->currframe = 0;
> +		}

[Severity: High]
This is a pre-existing issue, but does bcm_can_tx in net/can/bcm.c have a 
race condition with TX_RESET_MULTI_IDX that corrupts the sequence state?

When bcm_can_tx is called by the timer, it reads op->currframe under
op->bcm_tx_lock, drops the lock to send the frame, and reacquires the lock
to update the sequence state:

CPU1 (Timer context in bcm_can_tx)
    spin_lock_bh(&op->bcm_tx_lock);
    // reads currframe
    spin_unlock_bh(&op->bcm_tx_lock);
    
    can_send(skb, 1);

If a concurrent sendmsg triggers bcm_tx_setup while the lock is dropped:

CPU2 (User context in bcm_tx_setup)
    spin_lock_bh(&op->bcm_tx_lock);
    if (op->flags & TX_RESET_MULTI_IDX)
        op->currframe = 0;
    if (op->flags & SETTIMER)
        op->count = msg_head->count;
    spin_unlock_bh(&op->bcm_tx_lock);

Then CPU1 resumes and unconditionally increments the state:

CPU1 (Resuming in bcm_can_tx)
    spin_lock_bh(&op->bcm_tx_lock);
    op->currframe++; 
    if (op->count > 0)
        op->count--;
    spin_unlock_bh(&op->bcm_tx_lock);

Could this unconditional increment change the newly reset currframe from 0
to 1, causing frame 0 of the new sequence to be skipped? Also, wouldn't
this incorrectly decrement the new op->count limit?

>  
> -			if (op->flags & SETTIMER)
> -				op->count = msg_head->count;
> +		if (op->flags & SETTIMER)
> +			op->count = msg_head->count;
>  
> -			spin_unlock_bh(&op->bcm_tx_lock);
> -		}
> +		spin_unlock_bh(&op->bcm_tx_lock);
>  
> -		kfree(new_frames);

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