Re: [PATCH v7 13/13] can: peak_usb: don't return an uninitialized PCAN-USB CAN channel ID

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 1 potential
issue(s) to consider.

Critical: 0 · High: 0 · Medium: 1 (1) · Low: 0 (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).

- [Medium] The same ETHTOOL_GEEPROM disconnect-window leak is still open
  for PCAN-USB Pro, through the same shared handler.

Pre-existing issues:
- [Medium] Baseline decision: false (kept only because
  requires_validation=true).
- [Low] Baseline decision: false (kept only because
  requires_validation=true).

--

can: peak_usb: don't return an uninitialized PCAN-USB CAN channel ID

This patch zero-initializes args[] in pcan_usb_get_can_channel_id().
After disconnect has cleared PCAN_USB_STATE_CONNECTED, pcan_usb_wait_rsp()
returns success without filling args[]. Without the initializer, an
ETHTOOL_GEEPROM request in that window copies an uninitialized stack byte
to userspace.

> diff --git a/drivers/net/can/usb/peak_usb/pcan_usb.c b/drivers/net/can/usb/peak_usb/pcan_usb.c
> index 626593251e5d7..4f255f2a33feb 100644
> --- a/drivers/net/can/usb/peak_usb/pcan_usb.c
> +++ b/drivers/net/can/usb/peak_usb/pcan_usb.c
> @@ -386,7 +386,7 @@ static int pcan_usb_get_serial(struct peak_usb_device *dev, u32 *serial_number)
>   */
>  static int pcan_usb_get_can_channel_id(struct peak_usb_device *dev, u32 *can_ch_id)
>  {
> -	u8 args[PCAN_USB_CMD_ARGS_LEN];
> +	u8 args[PCAN_USB_CMD_ARGS_LEN] = {};

[Severity: Medium]
Is the same ETHTOOL_GEEPROM disconnect window still open for the PCAN-USB
Pro? It goes through the same peak_usb_get_eeprom() handler.

In pcan_usb_pro.c, pcan_usb_pro_get_can_channel_id() builds the GETDEVID
record with only one variadic argument:

	pcan_msg_add_rec(&um, PCAN_USBPRO_GETDEVID, dev->ctrl_idx);

pcan_msg_add_rec() handles GETDEVID in the same case as SETBTR and
SETDEVID. That case reads a second u32 argument, which this caller never
passes:

	case PCAN_USBPRO_SETBTR:
	case PCAN_USBPRO_GETDEVID:
	case PCAN_USBPRO_SETDEVID:
		*pc++ = va_arg(ap, int);
		pc += 2;
		*(__le32 *)pc = cpu_to_le32(va_arg(ap, u32));

That va_arg() is undefined behavior. In practice, whatever leftover
register or stack value it picks up ends up in the dev_num field of the
record in dev->cmd_buf.

The window opens after peak_usb_stop_rx() clears PCAN_USB_STATE_CONNECTED
and drops rtnl, and closes when unregister_candev() runs. Inside it,
pcan_usb_pro_send_cmd() and pcan_usb_pro_wait_rsp() both return 0 without
doing any USB I/O. Then this returns those leftover bytes as the channel
ID:

	pdn = (struct pcan_usb_pro_devid *)pc;
	*can_ch_id = le32_to_cpu(pdn->dev_num);

peak_usb_get_eeprom() then copies them to userspace. It also caches them
in dev->can_channel_id, which backs the can_channel_id sysfs attribute.

While the device is connected, the same 4 bytes are also sent to the
device with every GETDEVID request.

Would passing an explicit 0 as the second argument to pcan_msg_add_rec()
in pcan_usb_pro_get_can_channel_id() fix the Pro adapter? That could go
in a separate follow-up patch.

>  	int err;
>  
>  	err = pcan_usb_wait_rsp(dev, PCAN_USB_CMD_DEVID, PCAN_USB_GET, args);

[Severity: Low]
This isn't a bug introduced by this patch, but pcan_usb_wait_rsp() ignores
actual_length from the CMDIN usb_bulk_msg(). It also doesn't check that
cmd_buf[PCAN_USB_CMD_FUNC] and cmd_buf[PCAN_USB_CMD_NUM] match the
request:

	err = usb_bulk_msg(dev->udev,
		usb_rcvbulkpipe(dev->udev, PCAN_USB_EP_CMDIN),
		dev->cmd_buf, PCAN_USB_CMD_LEN, &actual_length,
		PCAN_USB_COMMAND_TIMEOUT);
	if (err)
		...
	else if (p)
		memcpy(p, dev->cmd_buf + PCAN_USB_CMD_ARGS,
			PCAN_USB_CMD_ARGS_LEN);

Suppose the device sends a short reply, or a late reply to an earlier
command that timed out. Could stale bytes from dev->cmd_buf then
overwrite the new zero initializer in args[]?

pcan_usb_send_cmd(..., NULL) only rewrites cmd_buf[0] and cmd_buf[1]. So
pcan_usb_get_can_channel_id() could report a byte left over from an
earlier outgoing command as the channel ID, for example btr1 from
pcan_usb_set_bittiming().

[Severity: Medium]
This is a pre-existing issue, but two setters in this file have the same
uninitialized args[] problem on the transmit side.
pcan_usb_set_can_channel_id() only sets args[0]:

	u8 args[PCAN_USB_CMD_ARGS_LEN];
	...
	args[0] = can_ch_id;
	return pcan_usb_send_cmd(dev, PCAN_USB_CMD_DEVID, PCAN_USB_SET, args);

pcan_usb_set_bittiming() only sets args[0] and args[1].

pcan_usb_send_cmd() copies all PCAN_USB_CMD_ARGS_LEN bytes into
dev->cmd_buf and sends the whole command over the bulk OUT endpoint:

	if (p)
		memcpy(dev->cmd_buf + PCAN_USB_CMD_ARGS,
			p, PCAN_USB_CMD_ARGS_LEN);

Does this send 12 or 13 bytes of uninitialized kernel stack to the USB
device on every channel ID write and every bitrate change?

The other setters in pcan_usb.c, such as pcan_usb_set_sja1000(),
pcan_usb_set_bus() and pcan_usb_set_led(), zero-initialize their args[].

Could these two args[] arrays get the same {} initializer?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261030-canfd_check_channel_idx-v7-0-fc4a3faf3acf%40peak-system.fr