Re: [PATCH can-next 6/7] can: peak_usb: Add bus error reporting for the PCAN-USB FD family

[Stéphane Grosjean]

Hello Marc,

Can you let me know if I need to make changes myself to these patches you sent, and if so, how? (Should the new requested changes—which are unrelated to the original patch—be included in a new version? Or in a different series?...)

Thank you for your help!

Best regards,

-- Stéphane

----- Mail original -----
> Thank you for your contribution! Sashiko AI review found 3 potential
> issue(s) to consider.
> 
> Critical: 0 · High: 1 · Medium: 1 (1) · Low: 1
> 
> If you believe none of the issues are legitimate, please reply to
> each
> issue of Medium severity or higher and explain why the AI is wrong.
> If any of them are legitimate and you plan to respin, please let
> patchwork
> know by including "pw-bot: cr" as a separate line at the end of your
> reply
> (one such reply per series is enough).
> 
> - [High] The invalid-channel guard is incomplete.
> - [Medium] pcan_usb_fd_start() now calls `err =
>   pcan_usb_fd_set_options(dev, 1, PUCAN_OPTION_ERROR, usb_opts);`
>   (line…
> - [Low] The new `if (!dev)` check in pcan_usb_fd_decode_error() fixes
> an
>   existing NULL pointer dereference, but the fix is folded into a…
> 
> Pre-existing issues:
> - [Medium] This problem wasn't introduced by this patch, but when
>   peak_usb_start() fails at dev_set_bus(dev, 1) after…
> 
> --
> 
> can: peak_usb: Add bus error reporting for the PCAN-USB FD family
> 
> This patch adds CAN bus error reporting for the PEAK-System
> USB-to-CAN FD
> devices. pcan_usb_fd_start() now enables PUCAN_OPTION_ERROR for each
> channel, pcan_usb_fd_decode_error() turns firmware error records into
> CAN
> error frames, and the adapters now advertise
> CAN_CTRLMODE_BERR_REPORTING.
> It also adds a NULL check for an invalid channel in
> pcan_usb_fd_decode_error().
> 
> > 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 82502594a4092..9081f30e3d356 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,73 @@ 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.
> > +	 * Treat the entire URB as invalid in that case.
> > +	 */
> >  	dev = usb_if->dev[pucan_ermsg_get_channel(er)];
> > +	if (!dev)
> > +		return -EINVAL;
> > +
> 
> [Severity: High]
> This check covers pcan_usb_fd_decode_error(). Do the other decoders
> that
> read a channel from the same URB need it too?
> 
> On PCAN-USB FD and PCAN-Chip USB, ctrl_count is
> PCAN_USBFD_CHANNEL_COUNT
> (1). pcan_usb_fd_init() therefore sets only usb_if->dev[0], and
> dev[1]
> stays NULL. The ARRAY_SIZE(usb_if->dev) bound is PCAN_USB_MAX_CHANNEL
> (2),
> so channel 1 passes that check.
> 
> The other three decoders still dereference without a NULL check:
> 
> pcan_usb_fd_decode_canmsg():
> 	dev = usb_if->dev[pucan_msg_get_channel(rm)];
> 	netdev = dev->netdev;
> 
> pcan_usb_fd_decode_status():
> 	dev = usb_if->dev[pucan_stmsg_get_channel(sm)];
> 	pdev = container_of(dev, struct pcan_usb_fd_device, dev);
> 	netdev = dev->netdev;
> 
> pcan_usb_fd_decode_overrun():
> 	dev = usb_if->dev[pufd_omsg_get_channel(ov)];
> 	netdev = dev->netdev;
> 
> Suppose the firmware sends the bogus channel 1 described in the new
> comment
> on a CAN_RX, STATUS or OVERRUN record. Wouldn't that dereference NULL
> in
> peak_usb_read_bulk_callback()->pcan_usb_fd_decode_buf()?
> 
> The commit message says "Parsing of the entire URB is aborted if the
> firmware reports an invalid channel". As far as I can tell, that only
> holds
> for error records. The later loopback patch in this series doesn't
> seem to
> change these decoders either.
> 
> [Severity: Low]
> The code itself is fine, but this check also fixes an existing NULL
> pointer
> dereference. Before this patch, pcan_usb_fd_decode_error() did:
> 
> 	dev = usb_if->dev[pucan_ermsg_get_channel(er)];
> 	pdev = container_of(dev, struct pcan_usb_fd_device, dev);
> 	pdev->bec.txerr = er->tx_err_cnt;
> 
> PUCAN_OPTION_ERROR was already enabled on first open, so a channel 1
> error
> record on a single-channel adapter could already fault there.
> 
> Could this fix be split out with a Fixes: tag and Cc: stable, as was
> done
> for the earlier "fix missing CAN_ERR_FLAG ..." patch in this series?
> That
> way stable trees would pick it up.
> 
> [ ... ]
> 
> > @@ -912,12 +969,17 @@ static int pcan_usb_fd_start(struct
> > peak_usb_device *dev)
> >  		peak_usb_init_time_ref(&pdev->usb_if->time_ref,
> >  				       &pcan_usb_pro_fd);
> >  
> > -		/* enable USB calibration messages */
> > -		err = pcan_usb_fd_set_options(dev, 1,
> > -					      PUCAN_OPTION_ERROR,
> > -					      PCAN_UFD_FLTEXT_CALIBRATION);
> > +		/* enable USB calibration messages (needed only once for the
> > +		 * entire interface)
> > +		 */
> > +		usb_opts |= PCAN_UFD_FLTEXT_CALIBRATION;
> >  	}
> >  
> > +	/* set channel device options: always asks for bus error
> > notifications
> > +	 * to get (at least) rxerr/txerr, as well as any USB-specific
> > option.
> > +	 */
> > +	err = pcan_usb_fd_set_options(dev, 1, PUCAN_OPTION_ERROR,
> > usb_opts);
> > +
> >  	pdev->usb_if->dev_opened_count++;
> 
> [Severity: Medium]
> Can dev_opened_count end up one too high here?
> 
> pcan_usb_fd_set_options() now runs on every open. If it fails, the
> counter
> is still incremented before err is returned. peak_usb_start() then
> takes
> the err_adapter path:
> 
> 	if (dev->adapter->dev_start) {
> 		err = dev->adapter->dev_start(dev);
> 		if (err)
> 			goto err_adapter;
> 	}
> 
> err_adapter frees the tx URBs and kills the rx URBs, but it never
> calls
> adapter->dev_stop(). ndo_stop isn't called after a failed open
> either.
> 
> With the count stuck too high:
> 
> - the == 0 check here no longer resets time_ref or re-enables
> calibration
>   on the next first open
> - the == 1 check in pcan_usb_fd_stop() never matches, so calibration
>   messages are never turned off
> - the > 0 check in pcan_usb_fd_exit() gives the wrong result
> 
> Before this patch, a non-first open could only fail in
> pcan_usb_fd_set_filter_std(), which returns before the increment.
> Should
> the increment be skipped when pcan_usb_fd_set_options() fails?
> 
> [Severity: Medium]
> This is a pre-existing issue and was not introduced by this patch,
> but a
> related path exists when pcan_usb_fd_start() succeeds and bus-on then
> fails
> in peak_usb_start():
> 
> 	err = dev->adapter->dev_set_bus(dev, 1);
> 	if (err)
> 		goto err_adapter;
> 
> err_adapter doesn't call dev_stop() here either. dev_opened_count
> stays
> incremented and the firmware notification options stay enabled. With
> this
> patch, the per-channel PUCAN_OPTION_ERROR is also left armed in that
> case.
> 
> Should peak_usb_start() undo dev_start() on this path?
> 
> [ ... ]
> 
> --
> Sashiko AI review ·
> https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261002-peak_usb_enhancements-v1-0-50e965755c06%40pengutronix.de
> 
>