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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] can: peak_usb: Stale error counters due to missing decrement updates
- [Medium] can: peak_usb: OOB read of 1 byte in pcan_usb_handle_bus_evt()
--

commit ea2fc1382b5e301ffcd1133cccb20af80b430884
Author: Stéphane Grosjean <s.grosjean@peak-system.fr>
Subject: can: peak_usb: Add PCAN-USB bus errors reporting

This commit adds PCAN-USB bus error reporting capabilities that were
previously missing. It modifies the driver to decode bus error interrupts
and forward the resulting CAN error frames to userspace.

Link: https://patch.msgid.link/20260924-peak_usb-v1-1-d48875169a59@peak-system.fr

> diff --git a/drivers/net/can/usb/peak_usb/pcan_usb.c b/drivers/net/can/usb/peak_usb/pcan_usb.c
> index 8fd058c328564..786f6600a5bfa 100644
> --- a/drivers/net/can/usb/peak_usb/pcan_usb.c
> +++ b/drivers/net/can/usb/peak_usb/pcan_usb.c

[ ... ]

> @@ -550,23 +564,92 @@ static int pcan_usb_decode_error(struct pcan_usb_msg_context *mc, u8 n,
>  /* decode bus event usb packet: first byte contains rxerr while 2nd one contains
>   * txerr.
>   */
> -static int pcan_usb_handle_bus_evt(struct pcan_usb_msg_context *mc, u8 ir)
> +static int pcan_usb_handle_bus_evt(struct pcan_usb_msg_context *mc, u8 ir,
> +				   u8 status_len)
>  {
>  	struct pcan_usb *pdev = mc->pdev;
>  
> -	/* according to the content of the packet */
> -	switch (ir) {
> -	case PCAN_USB_ERR_CNT_DEC:
> -	case PCAN_USB_ERR_CNT_INC:
> +	/* process bus error interrupt */
> +	if (ir & PCAN_SJA1000_IRQ_BEI) {

[Severity: High]
Will this discard valid counter decrements when the Bus Error Interrupt flag
is not set?

The parsing of rxerr and txerr was moved entirely inside this
if (ir & PCAN_SJA1000_IRQ_BEI) block. Since the BEI (Bus Error Interrupt)
flag is only set by the controller when an error occurs (increasing the
counters), status updates during successful transmissions that decrease the
counters will be ignored because they lack the BEI bit.

This leaves pdev->bec artificially high, which can cause
pcan_usb_decode_error() to incorrectly evaluate CAN_STATE_ERROR_PASSIVE
instead of CAN_STATE_ERROR_WARNING, breaking the CAN state machine.

> +		u8 rec_len = status_len & PCAN_USB_STATUSLEN_DLC;
> +		u8 *pd = mc->ptr, ecc = 0;
>  
> -		/* save rx/tx error counters from in the device context */
> -		pdev->bec.rxerr = mc->ptr[1];
> -		pdev->bec.txerr = mc->ptr[2];
> -		break;
> +		/* Check for potential out-of-bound accesses */
> +		if ((pd + rec_len - 1) > mc->end)
> +			return -EINVAL;

[Severity: Medium]
Does this bounds check in pcan_usb_handle_bus_evt() allow an out-of-bounds
read?

If a device crafts a PCAN_USB_REC_BUSEVT record where rec_len is 1, and
pd is exactly at mc->end, (pd + 1 - 1) > mc->end evaluates to false.
This permits the code to proceed and execute:

        if (rec_len >= 1) {
            ecc = *pd++;
            
            /* save rx/tx error counters from record data bytes */
            if (rec_len >= 2) {
                pdev->bec.rxerr = *pd++;
                if (rec_len >= 3)
                    pdev->bec.txerr = *pd++;
            }
        }
        
        ...
        cf->data[6] = pdev->bec.txerr;
        cf->data[7] = pdev->bec.rxerr;
        ...
        netif_rx(skb);

This leaks the out-of-bounds byte from the kernel heap into the cf->data
payload via the netif_rx() call.

> +
> +		if (rec_len >= 1) {
> +			ecc = *pd++;
> +
> +			/* save rx/tx error counters from record data bytes */
> +			if (rec_len >= 2) {
> +				pdev->bec.rxerr = *pd++;
> +				if (rec_len >= 3)
> +					pdev->bec.txerr = *pd++;
> +			}
> +		}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260925-peak_usb-v2-1-f3257bdeb7e6@peak-system.fr?part=1