Re: [PATCH v2 6/6] 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:

New issues:
- [Medium] Reading `cf->len` after a failed `memcpy_from_msg()` accesses uninitialized heap memory and can incorrectly overwrite the returned error code.

Pre-existing issues:
- [High] The patch fails to fix the data race on `op->frames` because `bcm_can_tx()` still copies frame data locklessly, allowing the transmission of torn CAN frames.
--

--- Patch [6]: [PATCH v2 6/6] can: bcm: extend bcm_tx_lock usage for data and timer updates ---
commit 9fb191cdc2eb3086cb7c75f76d0a1d693042a43b
Author: Oliver Hartkopp <socketcan@hartkopp.net>

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

This commit stages new CAN frame content into an allocated buffer to validate
it before updating the existing bcm_op under a lock. It also moves time
value updates under the same lock to avoid torn reads.

> diff --git a/net/can/bcm.c b/net/can/bcm.c
> index 3609178a1367d..fdde25b5aa1d7 100644
> --- a/net/can/bcm.c
> +++ b/net/can/bcm.c

[ ... ]

> @@ -1021,10 +1038,18 @@ static int bcm_tx_setup(struct bcm_msg_head *msg_head, struct msghdr *msg,
>  		if (msg_head->nframes > op->nframes)
>  			return -E2BIG;
>  
> -		/* update CAN frames content */
> +		/* get new CAN frames content into a staging buffer before
> +		 * locking: validate and normalize the frames there so that
> +		 * bcm_can_tx() / bcm_tx_timeout_handler() never observe a
> +		 * partially updated or unvalidated frame in op->frames
> +		 */
> +		new_frames = kmalloc(msg_head->nframes * op->cfsiz, GFP_KERNEL);
> +		if (!new_frames)
> +			return -ENOMEM;
> +
>  		for (i = 0; i < msg_head->nframes; i++) {
>  
> -			cf = op->frames + op->cfsiz * i;
> +			cf = new_frames + op->cfsiz * i;
>  			err = memcpy_from_msg((u8 *)cf, msg, op->cfsiz);
>  
>  			if (op->flags & CAN_FD_FRAME) {
> @@ -1035,36 +1060,38 @@ static int bcm_tx_setup(struct bcm_msg_head *msg_head, struct msghdr *msg,
>  					err = -EINVAL;
>  			}

[Severity: Medium]
If memcpy_from_msg() fails (e.g. returning -EFAULT), will this read cf->len
from uninitialized heap memory before the error check?

Since new_frames is allocated via kmalloc(), reading cf->len might read
garbage. If the garbage value happens to be > 64, it could incorrectly
overwrite the -EFAULT in err with -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);
> +

[Severity: High]
This is a pre-existing issue, but does this still leave a data race where
bcm_can_tx() can read partially updated frames?

The commit message states that bcm_can_tx() can no longer observe a
partially updated frame. However, looking at bcm_can_tx(), it calculates
the cf pointer under the lock but then reads the frame data locklessly:

net/can/bcm.c:bcm_can_tx() {
    if (!cf) {
        spin_lock_bh(&op->bcm_tx_lock);
        cf = op->frames + op->cfsiz * op->currframe;
        spin_unlock_bh(&op->bcm_tx_lock);
    }
    ...
    skb_put_data(skb, cf, op->cfsiz);
}

Doesn't skb_put_data() read from cf locklessly, potentially racing with
the memcpy() here in bcm_tx_setup() and copying a torn frame?

>  		op->flags = msg_head->flags;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260707-bcm_fixes-v2-0-96620c534b75@hartkopp.net?part=6