Re: [PATCH v2] can: peak_usb: Add bus error reporting for the PCAN-USB FD family

["Stéphane Grosjean" <stephane.grosjean@free.fr>]

Hi,

The NULL check was added to protect against a malformed PUCAN_MSG_ERROR
record reporting channel 1 on a single-channel adapter.
 
Such a value cannot be generated by compliant PCAN-USB FD firmware and
therefore indicates corrupted or malicious input. The driver already
returns -EINVAL and aborts URB decoding for other protocol violations,
for example when the reported channel index exceeds the bounds of the
channel array. This change follows the same policy.
 
Since the input stream is already invalid at this point, I believe
rejecting the entire URB is the correct behaviour rather than attempting
to continue parsing subsequent records from untrusted data.

Best regards,

-- Stéphane

----- Mail original -----
> Thank you for your contribution! Sashiko AI review found 1 potential
> issue(s) to consider:
> - [High] can: peak_usb: returning -EINVAL for bogus error reports
> drops valid URB packets
> --
> 
> commit 6ff05518907e7981e5fff27d40e8a2458d563751
> Author: Stéphane Grosjean <s.grosjean@peak-system.fr>
> 
> can: peak_usb: Add bus error reporting for the PCAN-USB FD family
> 
> CAN bus error reporting is currently missing for all PEAK-System
> USB-to-CAN FD devices. Add support for reporting bus errors by
> enabling
> bus error notifications in the firmware for each CAN channel.
> 
> Link to v1:
> https://patch.msgid.link/20260929-peak_usb-canfd-berr-v1-1-7abd585f877f@peak-system.fr
> 
> > diff --git a/drivers/net/can/usb/peak_usb/pcan_usb_fd.c
> > b/drivers/net/can/usb/peak_usb/pcan_usb_fd.c
> > index 0d46f4ce5dca7..98f6943469b33 100644
> > --- a/drivers/net/can/usb/peak_usb/pcan_usb_fd.c
> > +++ b/drivers/net/can/usb/peak_usb/pcan_usb_fd.c
> > @@ -661,17 +661,71 @@ static int pcan_usb_fd_decode_error(struct
> > pcan_usb_fd_if *usb_if,
> >  	struct pucan_error_msg *er = (struct pucan_error_msg *)rx_msg;
> >  	struct pcan_usb_fd_device *pdev;
> >  	struct peak_usb_device *dev;
> > +	struct can_frame *cf;
> > +	struct sk_buff *skb;
> > +	u8 can_err_tx = 0;
> >  
> >  	if (pucan_ermsg_get_channel(er) >= ARRAY_SIZE(usb_if->dev))
> >  		return -EINVAL;
> >  
> > +	/* Guard against bogus channel 1 reports from single-channel
> > adapters */
> >  	dev = usb_if->dev[pucan_ermsg_get_channel(er)];
> > +	if (!dev)
> > +		return -EINVAL;
> 
> [Severity: High]
> Does returning -EINVAL here cause valid URB packets to be dropped?
> 
> The caller pcan_usb_fd_decode_buf() handles a negative return value
> by
> aborting the URB decoding entirely and jumping to the fail label:
> 
> 	case PUCAN_MSG_ERROR:
> 		err = pcan_usb_fd_decode_error(usb_if, rx_msg);
> 		if (err < 0)
> 			goto fail;
> 		break;
> 
> Because USB bulk transfers pack multiple CAN records together,
> aborting
> the loop discards any subsequent valid CAN frames in the same URB.
> This appears to lead to silent data loss under normal operation.
> 
> Should this return 0 instead of -EINVAL to safely ignore the hardware
> quirk
> without dropping the rest of the buffer?
> 
> --
> Sashiko AI review ·
> https://sashiko.dev/#/patchset/20260929-peak_usb-canfd-berr-v2-1-1a7005797c38@peak-system.fr?part=1
>