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

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] bcm_can_tx() drops bcm_tx_lock between reading the frame and incrementing currframe, leading to frame sequence corruption if reset concurrently.
- [High] Torn reads and race conditions when reading timer variables, count, and flags outside of the protective spinlock in bcm_tx_timeout_handler() and bcm_proc_show().
--

--- Patch [6]: [PATCH v7 06/10] can: bcm: extend bcm_tx_lock usage for data and timer updates ---
commit 71db2892f1d82b77f3491796e67189db751f4bb1
Author: Oliver Hartkopp <socketcan@hartkopp.net>

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

Stage new CAN frame content for an existing tx op into a kmalloc()'d
buffer and validate it there, mirroring the approach already used in
bcm_rx_setup(). Only copy the validated data into op->frames while
holding op->bcm_tx_lock, so bcm_can_tx() and bcm_tx_timeout_handler()
can no longer observe a partially updated or unvalidated frame.

> diff --git a/net/can/bcm.c b/net/can/bcm.c
> --- a/net/can/bcm.c
> +++ b/net/can/bcm.c
> @@ -477,8 +483,17 @@ static enum hrtimer_restart bcm_tx_timeout_handler(struct hrtimer *hrtimer)
>  {
>  	struct bcm_op *op = container_of(hrtimer, struct bcm_op, timer);
>  	struct bcm_msg_head msg_head;
> +	bool tx_ival1, tx_ival2;
>  
> -	if (op->kt_ival1 && (op->count > 0)) {
> +	/* snapshot kt_ival1/kt_ival2/count under lock to avoid torn
> +	 * ktime_t reads racing with concurrent bcm_tx_setup() updates
> +	 */
> +	spin_lock_bh(&op->bcm_tx_lock);
> +	tx_ival1 = op->kt_ival1 && (op->count > 0);
> +	tx_ival2 = !!op->kt_ival2;
> +	spin_unlock_bh(&op->bcm_tx_lock);
> +
> +	if (tx_ival1) {
>  		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() have torn
reads for timer variables?

Just after the lock is dropped in the hunk above, bcm_tx_timeout_handler()
creates a notification to the user:

bcm_tx_timeout_handler() {
    ...
    spin_unlock_bh(&op->bcm_tx_lock);

    if (tx_ival1) {
        bcm_can_tx(op, NULL);
        if (!op->count && (op->flags & TX_COUNTEVT)) {
            ...
            msg_head.count   = op->count;
            msg_head.ival1   = op->ival1;
            msg_head.ival2   = op->ival2;
            ...
}

Because ival1 and ival2 are multi-word structures read outside the lock,
could a concurrent bcm_tx_setup() holding bcm_tx_lock overwrite these fields
and tear the values sent to user space?

Also, does bcm_proc_show() have a similar issue since it reads 64-bit
kt_ival1 and kt_ival2 under only rcu_read_lock() on 32-bit platforms?

[ ... ]

> @@ -1028,11 +1045,23 @@ static int bcm_tx_setup(struct bcm_msg_head *msg_head, struct msghdr *msg,
[ ... ]
> +		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);
> +		    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 isn't a bug introduced by this patch, but does resetting currframe
here race with bcm_can_tx()?

Looking at bcm_can_tx(), it drops the lock between reading the frame and
incrementing currframe:

bcm_can_tx() {
    if (!cf) {
        spin_lock_bh(&op->bcm_tx_lock);
        memcpy(&cframe, op->frames + op->cfsiz * op->currframe, ...);
        spin_unlock_bh(&op->bcm_tx_lock);
    }
    ...
    can_send(skb, 1);
    ...
    spin_lock_bh(&op->bcm_tx_lock);
    ...
    op->currframe++;
    if (op->currframe >= op->nframes)
        op->currframe = 0;
    spin_unlock_bh(&op->bcm_tx_lock);
}

If a concurrent bcm_tx_setup() acquires bcm_tx_lock, writes new frames, and
sets op->currframe = 0 while bcm_can_tx() is busy sending the old frame,
bcm_can_tx() will re-acquire the lock and erroneously increment currframe
to 1.

Will this skip the newly programmed first frame and cause sequence
corruption?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260708-bcm_fixes-v7-0-c78896b342e4@hartkopp.net?part=6