Re: [PATCH v2 1/3] can: ixxat_usb: add driver skeleton for IXXAT USB-to-CAN adapters

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 9 potential issue(s) to consider:
- [Critical] net/can: FORTIFY_SOURCE critical kernel panic from field-spanning memcpy in USB command payload
- [High] net/can: kernel BUG_ON triggered by unregistering an unregistered CAN device
- [High] net/can: infinite loop and divide-by-zero panic from unvalidated USB timestamp parameters
- [High] net/can: FORTIFY_SOURCE read overflow panic from oversized CAN-FD messages on CL1 adapters
- [High] net/can: permanent leakage of transmission slots and echo_skb buffers on hard USB errors
- [High] net/can: out-of-bounds read indexing endpoint arrays via untrusted controller count
- [Medium] net/can: memory leak of devdata and cmdbuf on probe if no CAN controllers are found
- [Medium] net/can: driver drops the DLC length bits when transmitting CAN RTR frames
- [Medium] net/can: TOCTOU race condition exposing uninitialized MAC address and sysfs nodes
--

commit b6d4cd22829b6f53b45ff0c4a4fce1127ba119e7
Author: Stéphane Grosjean <s.grosjean@peak-system.fr>

    can: ixxat_usb: add driver skeleton for IXXAT USB-to-CAN adapters
    
    Add the initial skeleton of the ixxat_usb driver: Kconfig, Makefile
    and the USB core (probe/disconnect, CAN netdev ops, USB URB handling,
    device command channel, ethtool/sysfs glue).

> diff --git a/drivers/net/can/usb/ixxat_usb/ixxat_usb_core.c b/drivers/net/can/usb/ixxat_usb/ixxat_usb_core.c
> index 0000000000000..3602ff9cc2c1d
> --- /dev/null
> +++ b/drivers/net/can/usb/ixxat_usb/ixxat_usb_core.c
[ ... ]
> +static int ixxat_usb_send_cmd_internal(struct usb_device *dev,
> +				       struct ixxat_usb_device_data *devdata,
> +				       const u16 port,
> +				       struct ixxat_usb_dal_req *cmd,
> +				       u16 cmd_size,
> +				       struct ixxat_usb_dal_res *res,
> +				       u16 res_size,
> +				       const unsigned long cmd_delay)
> +{
[ ... ]
> +	/* copy request to command buffer */
> +	memcpy(req_buf, cmd, cmd_size);

[Severity: Critical]
The inner USB command transport ixxat_usb_send_cmd_internal() accepts the
outgoing command and incoming response destinations via rigidly typed parameters
struct ixxat_usb_dal_req *cmd and struct ixxat_usb_dal_res *res, both of which
are strictly 12 bytes in size. Callers pass composite buffer sizes corresponding
to larger aggregated structures (e.g. 24 bytes and 68 bytes), which span beyond
the bounds of the specific req and res structure members.

Because ixxat_usb_send_cmd_internal() is static and analyzed
inter-procedurally, does memcpy() combined with CONFIG_FORTIFY_SOURCE enforce
the tight 12-byte structural boundaries based on the pointer types, instantly
causing a kernel panic on boot due to a field-spanning buffer overflow
violation?

[ ... ]
> +	/* copy response to user buffer */
> +	memcpy(res, res_buf, res_size);
[ ... ]
> +static void ixxat_usb_ts_set_cancaps(struct ixxat_time_ref *timeref,
> +				     u32 ts_clock_divisor,
> +				     u32 ts_clock_freq)
> +{
[ ... ]
> +	/* remove not significant zero bits from multiplier and divider */
> +	while (!(timeref->tick_multiplier & 0x1) &&
> +	       !(timeref->tick_divider & 0x1)) {
> +		timeref->tick_multiplier >>= 1;
> +		timeref->tick_divider >>= 1;
> +	}

[Severity: High]
The driver parses devdata->caps.ts_clock_divisor and ts_clock_freq directly
from the USB device capabilities payload without validating them for zero.

If a malicious or buggy USB device provides 0 for both fields, could this
loop run infinitely, causing a complete CPU hang? If only the frequency is 0,
does the subsequent do_div(..., timeref->tick_divider) cause a divide-by-zero
exception?

[ ... ]
> +static void ixxat_convert(const struct ixxat_usb_adapter *adapter,
> +			  struct canfd_frame *cf,
> +			  struct ixxat_can_msg *rx,
> +			  u8 datalen)
> +{
[ ... ]
> +	if (ixx_flags & IXXAT_USB_MSG_FLAGS_RTR)
> +		cf->can_id |= CAN_RTR_FLAG;
> +	else if (adapter->cl1_msg_format)
> +		memcpy(cf->data, rx->cl1.data, datalen);

[Severity: High]
For CL1 adapters, the driver blindly derives datalen from the USB message
flags. If a malicious USB device sets the IXXAT_USB_FDMSG_FLAGS_EDL flag,
datalen calculates to up to 64 bytes.

The code then calls memcpy() to copy datalen bytes out of the rx->cl1.data
array. Because rx->cl1.data is statically sized at 8 bytes (CAN_MAX_DLEN),
doesn't reading up to 64 bytes via memcpy() trigger a fatal FORTIFY_SOURCE
read-overflow panic?

[ ... ]
> +static int ixxat_usb_encode_msg(struct ixxat_usb_candevice *dev,
> +				struct sk_buff *skb, u8 *obuf)
> +{
[ ... ]
> +	if (can_is_canfd_skb(skb)) {
> +		flags |= IXXAT_USB_FDMSG_FLAGS_EDL;
> +
> +		if (cf->flags & CANFD_BRS)
> +			flags |= IXXAT_USB_FDMSG_FLAGS_FDR;
> +
> +		if (cf->flags & CANFD_ESI)
> +			flags |= IXXAT_USB_FDMSG_FLAGS_ESI;
> +
> +		flags |= FIELD_PREP(IXXAT_USB_MSG_FLAGS_DLC_MASK,
> +				    can_fd_len2dlc(cf->len));
> +	} else if (cf->can_id & CAN_RTR_FLAG) {
> +		flags |= IXXAT_USB_MSG_FLAGS_RTR;
> +	} else {
> +		flags |= FIELD_PREP(IXXAT_USB_MSG_FLAGS_DLC_MASK, cf->len);
> +	}

[Severity: Medium]
When encoding a CAN message into the USB protocol buffer, the driver checks
if the message is a Remote Transmission Request (RTR). If it is, it sets the
IXXAT_USB_MSG_FLAGS_RTR flag but exclusively skips packing the original length
(cf->len) into the IXXAT_USB_MSG_FLAGS_DLC_MASK bits.

Because CAN RTR frames carry zero payload data but still require a valid Data
Length Code to signal the requested response size, does emitting a frame with
DLC=0 corrupt the outgoing RTR request on the bus?

[ ... ]
> +static void ixxat_usb_write_bulk_callback(struct urb *urb)
> +{
[ ... ]
> +	/* Fix sashiko-bot v1 issue:
> +	 * don't skip the cleanup block if a status error occurs
> +	 */
> +	err = ixxat_evaluate_usb_status(netdev, urb, dev->ep_msg_out);
> +	if (err == -1)
> +		goto lbl_rel;

[Severity: High]
If a bulk transmit URB completes with a fatal error code (such as -EPROTO or
-ESHUTDOWN), the completion callback explicitly skips the resource cleanup by
jumping directly to lbl_rel.

This fails to call both can_get_echo_skb() and ixxat_usb_msg_free_idx(). The
internal bitmask of active message indices (dev->msgs) will never clear the
failed slot, and the CAN stack's echo buffer permanently locks up the SKB.

Does this cause a permanent leakage of transmission slots and echo_skb buffers
on hard USB errors?

[ ... ]
> +static int ixxat_usb_create_ctrl(struct usb_interface *intf,
> +				 const struct ixxat_usb_adapter *adapter,
> +				 u16 ctrl_index,
> +				 struct ixxat_usb_device_data *devdata)
> +{
[ ... ]
> +	dev->adapter->get_ctrl_caps(dev, &devdata->caps);
> +
> +	i = ctrl_index + adapter->ep_offs;
> +	dev->ep_msg_in = adapter->ep_msg_in[i];
> +	dev->ep_msg_out = adapter->ep_msg_out[i];

[Severity: High]
The maximum number of controllers an IXXAT device can advertise is bounded by
IXXAT_USB_MAX_TYPES (32) during capability extraction. However, the static
endpoint mapping arrays ep_msg_in and ep_msg_out inside the device's adapter
definition only have IXXAT_USB_MAX_CHANNEL (5) slots.

If a malicious USB device returns a bus_ctrl_count greater than 5, does this
force the driver to read out-of-bounds memory to determine its communication
endpoints, eventually submitting broken URBs with garbage parameters?

[ ... ]
> +	err = register_candev(netdev);
> +	if (err) {
> +		dev_err(&intf->dev, "Error %d: Failed to register can device\n",
> +			err);
> +		goto free_candev;
> +	}
> +
> +	netdev->addr_len = sizeof(devdata->dev_info.device_id);
> +	dev_addr_mod(netdev, 0, devdata->dev_info.device_id,
> +		     sizeof(devdata->dev_info.device_id));
> +
> +	err = sysfs_create_group(&netdev->dev.kobj, &ixxat_pdev_group);

[Severity: Medium]
During driver probe, register_candev(netdev) is invoked immediately after
memory allocation and queue setup. Registration makes the device globally
visible to userspace, dispatching NETDEV_REGISTER uevents to udev.

However, the driver writes the hardware MAC address (dev_addr_mod) and creates
its proprietary sysfs hierarchy (sysfs_create_group) strictly after the
registration call returns.

Can a userspace network daemon triggered by the netdev birth event read
missing or garbage properties, breaking network initialization policies?

[ ... ]
> +free_candev:
> +	/* sashiko-bot v1: missing unregister_candev() */
> +	unregister_candev(netdev);

[Severity: High]
If register_candev(netdev) fails during controller initialization, the
code jumps to the free_candev error label where it blindly executes
unregister_candev(netdev).

Since unregister_candev() directly delegates to unregister_netdev(), this
violates the network core's strict state tracking. Is it safe to call it on a
device that is not in the NETREG_REGISTERED state, or does it trigger an
immediate BUG_ON() kernel panic?

[ ... ]
> +static int ixxat_usb_probe(struct usb_interface *intf,
> +			   const struct usb_device_id *id)
> +{
[ ... ]
> +		if (err) {
> +			/* Deregister already created devices */
> +			ixxat_usb_disconnect(intf);
> +
> +			/* sashiko-bot v1:
> +			 * if 1st device creation failed, then shared resources
> +			 * must be released too.
> +			 */
> +			if (!ctrlidx)
> +				goto lbl_err;
> +
> +			/* Otherwise, they have been released by
> +			 * ixxat_usb_disconnect().
> +			 */
> +			return err;
> +		}
> +	}
> +
> +	return 0;
> +
> +lbl_err:
> +	kfree(cmdbuf);
> +	kfree(devdata);
> +	return err;
> +}

[Severity: Medium]
If the probe loop completes without finding any controllers of type
IXXAT_USB_BUSTYPE_CAN (or if ctrl_count is 0), the function successfully
returns 0. However, usb_set_intfdata() is never called for these shared
structures.

When the device is later unplugged, ixxat_usb_disconnect() fetches dev via
usb_get_intfdata(), finds it to be NULL, and returns early without freeing
devdata and cmdbuf. Returning an error directly on intermediate initialization
failures also circumvents the cleanup.

Will this leak devdata and cmdbuf on probe if no CAN controllers are found?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260916-ixxat_usb_can-v2-0-9b892e362d9e@peak-system.fr?part=1