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

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

[Stéphane Grosjean]

From: Stéphane Grosjean <s.grosjean@peak-system.fr>

The good old PCAN-USB was missing CAN bus error reporting. This patch fixes
that.

Signed-off-by: Stéphane Grosjean <s.grosjean@peak-system.fr>
---
 drivers/net/can/usb/peak_usb/pcan_usb.c | 101 ++++++++++++++++++++++++++++++--
 1 file changed, 97 insertions(+), 4 deletions(-)

diff --git a/drivers/net/can/usb/peak_usb/pcan_usb.c b/drivers/net/can/usb/peak_usb/pcan_usb.c
index 8fd058c32856..e5be07f3dd54 100644
--- a/drivers/net/can/usb/peak_usb/pcan_usb.c
+++ b/drivers/net/can/usb/peak_usb/pcan_usb.c
@@ -115,6 +115,7 @@
 #define PCAN_USB_REC_BUSEVT		5
 
 /* CAN bus events notifications selection mask */
+#define PCAN_USB_ERR_ECC		0x01	/* ask for BERR */
 #define PCAN_USB_ERR_RXERR		0x02	/* ask for rxerr counter */
 #define PCAN_USB_ERR_TXERR		0x04	/* ask for txerr counter */
 
@@ -122,7 +123,20 @@
  * In other words, its interest is to know which side among rx and tx is
  * responsible of the change of the bus state.
  */
-#define PCAN_USB_BERR_MASK	(PCAN_USB_ERR_RXERR | PCAN_USB_ERR_TXERR)
+#define PCAN_USB_BERR_MASK	(PCAN_USB_ERR_ECC | \
+				 PCAN_USB_ERR_RXERR | PCAN_USB_ERR_TXERR)
+
+/* SJA1000 ECC register */
+#define PCAN_SJA1000_ECC_SEG		0x1f
+#define PCAN_SJA1000_ECC_DIR		0x20
+#define PCAN_SJA1000_ECC_ERR		6
+#define PCAN_SJA1000_ECC_BIT		0x00
+#define PCAN_SJA1000_ECC_FORM		0x40
+#define PCAN_SJA1000_ECC_STUFF		0x80
+#define PCAN_SJA1000_ECC_MASK		0xc0
+
+/* SJA1000 Bus Error Interrupt */
+#define PCAN_SJA1000_IRQ_BEI		0x80
 
 /* identify bus event packets with rx/tx error counters */
 #define PCAN_USB_ERR_CNT_DEC		0x00	/* counters are decreasing */
@@ -550,7 +564,8 @@ 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;
 
@@ -569,6 +584,83 @@ static int pcan_usb_handle_bus_evt(struct pcan_usb_msg_context *mc, u8 ir)
 		break;
 	}
 
+	/* 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++;
+		}
+
+		/* 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++;
+
+			/* Error occurred during reception? */
+			if (ecc & PCAN_SJA1000_ECC_DIR) {
+				stats->rx_errors++;
+			} else {
+				stats->tx_errors++;
+				can_err_tx = CAN_ERR_PROT_TX;
+			}
+
+			/* if berr-reporting is off, stop here */
+			if (!(pdev->dev.can.ctrlmode &
+						CAN_CTRLMODE_BERR_REPORTING))
+				return 0;
+
+			/* allocate an skb to store the error frame */
+			skb = alloc_can_err_skb(mc->netdev, &cf);
+			if (!skb)
+				return -ENOMEM;
+
+			cf->can_id |= CAN_ERR_PROT | CAN_ERR_BUSERROR;
+			cf->data[2] |= can_err_tx;
+
+			/* set error type according to 1st data byte (ECC) */
+			switch (ecc & PCAN_SJA1000_ECC_MASK) {
+			case PCAN_SJA1000_ECC_BIT:
+				cf->data[2] |= CAN_ERR_PROT_BIT;
+				break;
+			case PCAN_SJA1000_ECC_FORM:
+				cf->data[2] |= CAN_ERR_PROT_FORM;
+				break;
+			case PCAN_SJA1000_ECC_STUFF:
+				cf->data[2] |= CAN_ERR_PROT_STUFF;
+				break;
+			default:
+				break;
+			}
+
+			/* 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);
+		}
+	}
+
 	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;
@@ -1017,7 +1109,8 @@ const struct peak_usb_adapter pcan_usb = {
 	.device_id = PCAN_USB_PRODUCT_ID,
 	.ctrl_count = 1,
 	.ctrlmode_supported = CAN_CTRLMODE_3_SAMPLES | CAN_CTRLMODE_LISTENONLY |
-			      CAN_CTRLMODE_CC_LEN8_DLC,
+			      CAN_CTRLMODE_CC_LEN8_DLC |
+			      CAN_CTRLMODE_BERR_REPORTING,
 	.clock = {
 		.freq = PCAN_USB_CRYSTAL_HZ / 2,
 	},

---
base-commit: 3bb1277609aae9b56b8e9462fe18fc000944fa96
change-id: 20260924-peak_usb-5d8607af1f99

Best regards,
--  
Stéphane Grosjean <s.grosjean@peak-system.fr>

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

[sashiko-bot]

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