Re: [PATCH v11 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:

Pre-existing issues:
- [High] TX path directly overwrites active CAN frames via `memcpy_from_msg()` without a staging buffer or write-side lock, rendering the read-side lock useless and allowing torn frames or arbitrary `can_id` injection.
--

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

can: bcm: add locking when updating filter and timer values

This commit adds locking to protect timer and filter data updates in
bcm_rx_setup() against concurrent execution of RX handlers. It also adds a
lock-protected snapshot of the currframe in bcm_can_tx() to avoid partial
overwrites by content updates in bcm_tx_setup().

Closes: 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
[ ... ]
> @@ -293,22 +294,26 @@ static int bcm_proc_show(struct seq_file *m, void *v)
>   * bcm_can_tx - send the (next) CAN frame to the appropriate CAN interface
>   *              of the given bcm tx op
>   */
> -static void bcm_can_tx(struct bcm_op *op)
> +static void bcm_can_tx(struct bcm_op *op, struct canfd_frame *cf)
>  {
[ ... ]
> -	/* read currframe under lock protection */
> -	spin_lock_bh(&op->bcm_tx_lock);
> -	cf = op->frames + op->cfsiz * op->currframe;
> -	spin_unlock_bh(&op->bcm_tx_lock);
> +	if (!cf) {
> +		/* read currframe under lock protection */
> +		spin_lock_bh(&op->bcm_tx_lock);
> +		memcpy(&cframe, op->frames + op->cfsiz * op->currframe,
> +		       op->cfsiz);
> +		cf = &cframe;
> +		spin_unlock_bh(&op->bcm_tx_lock);
> +	}

[Severity: High]
This is a pre-existing issue, but does this read-side snapshot actually
provide protection against partial overwrites from bcm_tx_setup()?

When updating an existing TX operation, bcm_tx_setup() directly overwrites
op->frames using the sleepable memcpy_from_msg() without holding
bcm_tx_lock or using a staging buffer:

net/can/bcm.c:bcm_tx_setup() {
    ...
    err = memcpy_from_msg((u8 *)cf, msg, op->cfsiz);
    ...
    if (msg_head->flags & TX_CP_CAN_ID) {
        /* copy can_id into frame */
        cf->can_id = msg_head->can_id;
    }
    ...
}

Because the write-side completely ignores the lock, could a concurrent
timer still transmit torn CAN frames?

If TX_CP_CAN_ID is used, could the frame be transmitted after the user
payload is copied but before the enforced can_id is applied, allowing
unprivileged users to bypass ID enforcement and inject arbitrary CAN IDs?

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