Re: [PATCH] can: peak_usb: Add PCAN-USB bus errors reporting

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] can: peak_usb: out-of-bounds read in pcan_usb_handle_bus_evt() due to delayed bounds check
--

commit a04e1603d8018a183d7ea3984b27c9b6064aa261
Author: Stéphane Grosjean <s.grosjean@peak-system.fr>

can: peak_usb: Add PCAN-USB bus errors reporting

This patch adds CAN bus error reporting to the PCAN-USB driver by decoding
the bus error interrupt payload and submitting it as a CAN error frame.

> diff --git a/drivers/net/can/usb/peak_usb/pcan_usb.c b/drivers/net/can/usb/peak_usb/pcan_usb.c
> index 8fd058c328564..e5be07f3dd54c 100644
> --- a/drivers/net/can/usb/peak_usb/pcan_usb.c
> +++ b/drivers/net/can/usb/peak_usb/pcan_usb.c
[ ... ]
> @@ -569,6 +584,83 @@ static int pcan_usb_handle_bus_evt(struct pcan_usb_msg_context *mc, u8 ir)
>  		break;
>  	}
>  

[Severity: Medium]
This is a pre-existing issue, but looking at the switch statement just above
this new block, if a USB packet arrives with ir = 0x80, both
PCAN_USB_ERR_CNT_INC and PCAN_SJA1000_IRQ_BEI equal 0x80.

The switch statement executes first and unconditionally reads mc->ptr[1]
and mc->ptr[2] without checking the payload size:

switch (ir) {
case PCAN_USB_ERR_CNT_DEC:
case PCAN_USB_ERR_CNT_INC:
        pdev->bec.rxerr = mc->ptr[1];
        pdev->bec.txerr = mc->ptr[2];
        break;

Does this result in an out-of-bounds read in the switch statement for
short packets before the new if block executes?

> +	/* process bus error interrupt */
> +	if (ir & PCAN_SJA1000_IRQ_BEI) {
> +		u8 rec_len = status_len & PCAN_USB_STATUSLEN_DLC;
> +		u8 *pd = mc->ptr, ecc = 0;
> +
> +		if (rec_len > 2)
> +			ecc = *pd++;
> +
> +		if (rec_len > 1) {
> +			/* save rx/tx error counters from data bytes */
> +			pdev->bec.rxerr = *pd++;
> +			pdev->bec.txerr = *pd++;
> +		}

[Severity: High]
Here rec_len is used to read up to 3 bytes from the buffer using pd.
However, looking at pcan_usb_decode_status(), the bounds check against
mc->end is deferred until after pcan_usb_handle_bus_evt() returns.

If a malicious USB device provides a status_len indicating rec_len > 1
but a payload shorter than rec_len, does this read past mc->end?

> +
> +		/* create an "bus-error frame" skb if any bit is set in ECC */
> +		if (ecc) {
> +			struct net_device_stats *stats = &mc->netdev->stats;
> +			struct sk_buff *skb;
> +			struct can_frame *cf;
> +			u8 can_err_tx = 0;
> +
> +			pdev->dev.can.can_stats.bus_error++;

[Severity: Medium]
This isn't a bug introduced by this patch, but this increments network
statistics directly. If multiple RX URBs complete concurrently across
different CPUs, can updates to bus_error or rx_errors race and drop
increments on architectures without atomic increment instructions?

> +
> +			/* Error occurred during reception? */
> +			if (ecc & PCAN_SJA1000_ECC_DIR) {
> +				stats->rx_errors++;
> +			} else {
> +				stats->tx_errors++;
> +				can_err_tx = CAN_ERR_PROT_TX;
> +			}
[ ... ]
> +			/* set error location */
> +			cf->data[3] = ecc & PCAN_SJA1000_ECC_SEG;
> +
> +			cf->data[6] = pdev->bec.txerr;
> +			cf->data[7] = pdev->bec.rxerr;
> +
> +			if (status_len & PCAN_USB_STATUSLEN_TIMESTAMP) {
> +				struct skb_shared_hwtstamps *hwts =
> +					skb_hwtstamps(skb);
> +
> +				peak_usb_get_ts_time(&mc->pdev->time_ref,
> +						     mc->ts16, &hwts->hwtstamp);
> +			}
> +
> +			netif_rx(skb);

[Severity: High]
If the bounds check hasn't occurred yet, ecc, rxerr, and txerr could be
populated with uninitialized bytes read past mc->end.

Does placing these out-of-bounds bytes into the SKB and passing it to
netif_rx() leak heap memory to userspace?

>  	}
>  
>  	return 0;
>  }
[ ... ]
> @@ -627,7 +719,7 @@ static int pcan_usb_decode_status(struct pcan_usb_msg_context *mc,
>  
>  	case PCAN_USB_REC_BUSEVT:
>  		/* bus event notifications (get rxerr/txerr) */
> -		err = pcan_usb_handle_bus_evt(mc, n);
> +		err = pcan_usb_handle_bus_evt(mc, n, status_len);
>  		if (err)
>  			return err;
>  		break;

[Severity: High]
Is the bounds check for this packet deferred too late?

pcan_usb_handle_bus_evt() is called here and uses the unvalidated rec_len
to process the packet, but the bounds check ((mc->ptr + rec_len) > mc->end)
does not happen until the end of pcan_usb_decode_status().

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260924-peak_usb-v1-1-d48875169a59@peak-system.fr?part=1