Re: [PATCH can-next 7/7] can: peak_usb: Add support of loopback mode to the PCAN-USB FD family

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found more severe
issue(s) in other patches of this series. This mail is only an FYI, in case
you want to address the nit picks below when you respin.

Low priority / nit picks for your consideration:

- [Low] The patch adds CAN_CTRLMODE_LOOPBACK to the static
  .ctrlmode_supported of all four FD-family adapters: pcan_usb_fd,…

Pre-existing issues:
- [High] This problem wasn't introduced by this patch, but a malicious or
  faulty USB device that presents itself as a single-channel PCAN-USB FD…

--

can: peak_usb: Add support of loopback mode to the PCAN-USB FD family

This commit adds CAN_CTRLMODE_LOOPBACK to the supported modes of the
PCAN-USB FD, PCAN-Chip USB, PCAN-USB Pro FD and PCAN-USB X6 adapters. When
loopback is on, pcan_usb_fd_encode_msg() sets PUCAN_MSG_LOOPED_BACK and
PUCAN_MSG_SELF_RECEIVE in the uCAN TX message flags. This follows the
peak_pciefd driver, but local echo still comes from USB write completions.

> 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 9081f30e3d356..59debe009f922 100644
> --- a/drivers/net/can/usb/peak_usb/pcan_usb_fd.c
> +++ b/drivers/net/can/usb/peak_usb/pcan_usb_fd.c
> @@ -934,6 +934,12 @@ static int pcan_usb_fd_encode_msg(struct peak_usb_device *dev,
>  	if (dev->can.ctrlmode & CAN_CTRLMODE_ONE_SHOT)
>  		tx_msg_flags |= PUCAN_MSG_SINGLE_SHOT;
>  
> +	/* Request controller loopback and mark the frame as self-received,
> +	 * matching the behavior of the peak_pciefd driver.
> +	 */
> +	if (dev->can.ctrlmode & CAN_CTRLMODE_LOOPBACK)
> +		tx_msg_flags |= PUCAN_MSG_LOOPED_BACK | PUCAN_MSG_SELF_RECEIVE;
> +
>  	tx_msg->flags = cpu_to_le16(tx_msg_flags);
>  	tx_msg->channel_dlc = PUCAN_MSG_CHANNEL_DLC(dev->ctrl_idx, dlc);
>  	memcpy(tx_msg->d, cfd->data, cfd->len);
> @@ -1263,7 +1269,8 @@ const struct peak_usb_adapter pcan_usb_fd = {
>  	.name = "PCAN-USB FD",
>  	.device_id = PCAN_USBFD_PRODUCT_ID,
>  	.ctrl_count = PCAN_USBFD_CHANNEL_COUNT,
> -	.ctrlmode_supported = CAN_CTRLMODE_LISTENONLY |
> +	.ctrlmode_supported = CAN_CTRLMODE_LOOPBACK |
> +			      CAN_CTRLMODE_LISTENONLY |
>  			      CAN_CTRLMODE_3_SAMPLES |
>  			      CAN_CTRLMODE_ONE_SHOT |
>  			      CAN_CTRLMODE_BERR_REPORTING |

[Severity: Low]
Does every firmware revision of these adapters support loopback?

LOOPBACK is added statically here, and also in pcan_usb_chip,
pcan_usb_pro_fd and pcan_usb_x6. All the new mode does is set
PUCAN_MSG_LOOPED_BACK | PUCAN_MSG_SELF_RECEIVE in pcan_usb_fd_encode_msg().
The host does not program any controller mode, and
pcan_usb_fd_decode_canmsg() never reads these flags. Whether the mode works
is therefore up to the firmware.

pcan_usb_fd_init() already gates modes that depend on the firmware:

	if (fw_info->fw_version[0] >= 2) {
		/* firmware >= 2.x supports ISO/non-ISO switching */
		dev->can.ctrlmode_supported |= CAN_CTRLMODE_FD_NON_ISO;

If a firmware revision ignored these TX flags, "ip link set canX type can
loopback on" would be accepted but would do nothing.

This may well be fine. ONE_SHOT is already advertised statically, and it
is also just a uCAN TX flag (PUCAN_MSG_SINGLE_SHOT). The non-FD driver's
pcan_usb_init() does gate LOOPBACK on device_rev >= 41, but it gates
ONE_SHOT the same way, so that comparison may not apply here. The commit
message also says all USB-to-CAN FD interfaces support this mode.

Is there any FD firmware version that should be excluded here?

[ ... ]

[Severity: High]
This is a pre-existing issue and was not introduced by this patch. Can a
single-channel adapter cause a NULL pointer dereference in the bulk-IN
decode path?

PCAN-USB FD and PCAN-Chip USB use ctrl_count = PCAN_USBFD_CHANNEL_COUNT
(1). struct pcan_usb_fd_if, however, has dev[PCAN_USB_MAX_CHANNEL] (2).
pcan_usb_fd_init() only fills the slot of the controller being registered:

	pdev->usb_if->dev[dev->ctrl_idx] = dev;

On those adapters, usb_if->dev[1] stays NULL.

Before dereferencing, pcan_usb_fd_decode_canmsg() only checks the channel
against the array size:

	if (pucan_msg_get_channel(rm) >= ARRAY_SIZE(usb_if->dev))
		return -ENOMEM;

	dev = usb_if->dev[pucan_msg_get_channel(rm)];
	netdev = dev->netdev;

A faulty or malicious device could send a PUCAN_MSG_CAN_RX record with
channel 1. That record passes this check, and the code then dereferences
NULL in URB completion context.

pcan_usb_fd_decode_status() and pcan_usb_fd_decode_overrun() use the same
pattern, for PUCAN_MSG_STATUS and PCAN_UFD_MSG_OVERRUN records.

pcan_usb_fd_decode_error() already handles this case. The guard came in
with "can: peak_usb: Add bus error reporting for the PCAN-USB FD family",
but it was added only to this decoder:

	/* 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;

Should the CAN_RX, STATUS and OVERRUN decoders get the same NULL check?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261002-peak_usb_enhancements-v1-0-50e965755c06%40pengutronix.de