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

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

Pre-existing issues:
- [High] Missing payload length validation for `RX_RTR_FRAME` replies in `bcm_rx_setup` allows injecting frames with arbitrary length, causing out-of-bounds reads in drivers.
- [High] Incomplete fix: `bcm_tx_setup()` updates `op->frames` locklessly, rendering the reader lock in `bcm_can_tx()` ineffective against data races.
- [High] Torn reads of 64-bit timer values (`kt_ival1`, `kt_ival2`) in the TX path on 32-bit systems lead to timer corruption.
- [High] Concurrent reset of `op->frames_abs` causes a divide-by-zero kernel panic in `bcm_proc_show()`.
--

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

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

KCSAN detected a simultaneous access to timer values that can be
overwritten in bcm_rx_setup() when updating timer and filter content
while bcm_rx_handler(), bcm_rx_timeout_handler() or bcm_rx_thr_handler()
run concurrently on incoming CAN traffic.

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
> @@ -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)
>  {
>  	struct sk_buff *skb;
>  	struct can_skb_ext *csx;
>  	struct net_device *dev;
> -	struct canfd_frame *cf;
> +	struct canfd_frame cframe;
>  	int err;
>  
>  	/* no target device? => exit */
>  	if (!op->ifindex)
>  		return;
>  
> -	/* 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 bcm_tx_setup() still overwrite
op->frames locklessly from userspace?

If bcm_tx_setup() directly uses memcpy_from_msg() without holding
bcm_tx_lock:

net/can/bcm.c:bcm_tx_setup() {
    ...
    cf = op->frames + op->cfsiz * i;
    err = memcpy_from_msg((u8 *)cf, msg, op->cfsiz);
    ...
}

Would this reader lock in bcm_can_tx() be ineffective, allowing it to
copy and transmit a partially overwritten torn frame?

[ ... ]

> @@ -461,7 +466,7 @@ static enum hrtimer_restart bcm_tx_timeout_handler(struct hrtimer *hrtimer)
>  	struct bcm_msg_head msg_head;
>  
>  	if (op->kt_ival1 && (op->count > 0)) {
> -		bcm_can_tx(op);
> +		bcm_can_tx(op, NULL);

[Severity: High]
This is a pre-existing issue, but could this lockless read of op->kt_ival1 in
softirq context lead to a torn read?

If bcm_tx_setup() assigns the 64-bit kt_ival1 and kt_ival2 locklessly:

net/can/bcm.c:bcm_tx_setup() {
    ...
    op->kt_ival1 = bcm_timeval_to_ktime(msg_head->ival1);
    op->kt_ival2 = bcm_timeval_to_ktime(msg_head->ival2);
    ...
}

Could a concurrent bcm_tx_timeout_handler() on a 32-bit system observe torn
halves of the timer interval and set a corrupted expiry time? This appears
similar to the KCSAN-reported torn read on the RX path that this patch fixes.

[ ... ]

> @@ -729,9 +751,18 @@ static void bcm_rx_handler(struct sk_buff *skb, void *data)
>  	/* update statistics */
>  	op->frames_abs++;

[Severity: High]
This is a pre-existing issue, but does the concurrent reset of op->frames_abs
cause a divide-by-zero kernel panic in bcm_proc_show()?

If bcm_proc_show() executes locklessly:

net/can/bcm.c:bcm_proc_show() {
    if (!op->frames_abs)
        continue;
    
    // ... 
    reduction = 100 - (op->frames_filtered * 100) / op->frames_abs;
    ...
}

And a concurrent CAN frame reception triggers bcm_rx_changed() which resets
these counters without synchronization:

net/can/bcm.c:bcm_rx_changed() {
    if (op->frames_filtered > ULONG_MAX/100)
        op->frames_filtered = op->frames_abs = 0;
    ...
}

Could op->frames_abs be zeroed out between the check and the division,
resulting in a crash?

[ ... ]

> @@ -1130,6 +1165,24 @@ static int bcm_tx_setup(struct bcm_msg_head *msg_head, struct msghdr *msg,
>  	return err;
>  }
>  
> +static void bcm_rx_setup_rtr_check(struct bcm_msg_head *msg_head,
> +				   struct bcm_op *op, void *new_frames)
> +{
> +	/* funny feature in RX(!)_SETUP only for RTR-mode:
> +	 * copy can_id into frame BUT without RTR-flag to
> +	 * prevent a full-load-loopback-test ... ;-]
> +	 * normalize this on the staged buffer, before it is
> +	 * ever installed into op->frames.
> +	 */
> +	if (msg_head->flags & RX_RTR_FRAME) {
> +		struct canfd_frame *frame0 = new_frames;
> +
> +		if ((msg_head->flags & TX_CP_CAN_ID) ||
> +		    frame0->can_id == op->can_id)
> +			frame0->can_id = op->can_id & ~CAN_RTR_FLAG;
> +	}
> +}

[Severity: High]
This is a pre-existing issue, but is there missing payload length validation
for RX_RTR_FRAME replies?

While bcm_rx_setup_rtr_check() normalizes the CAN ID, it does not appear to
check the len field of the user-supplied canfd_frame. 

If a user creates an RX_SETUP operation with RX_RTR_FRAME and provides a
frame with a maliciously large len field (e.g., 255), could bcm_can_tx()
later allocate an SKB and hand this invalid frame to the networking stack?

Will this cause out-of-bounds reads in drivers that trust skb->len?

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