Re: [PATCH 4/4] can: usb: ixxat_usb: add core driver and Kconfig/Makefile

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 11 potential issue(s) to consider:
- [Critical] ixxat_usb_start_xmit returns a negative error code upon URB submission failure, causing a double-free of the SKB.
- [Critical] Missing unregister_candev() in the error path of ixxat_usb_create_ctrl() causes a freed network device to remain registered.
- [High] TX URBs and their transfer buffers are permanently leaked across interface toggles and on device disconnect.
- [High] Transient USB bus errors or memory allocation failures permanently halt the TX and RX paths.
- [High] Out-of-bounds read in ixxat_usb_decode_buf when parsing USB packets due to reversed validation logic.
- [High] The command buffer used for USB control messages is undersized for the request-response cycle of certain commands, leading to a heap buffer overflow.
- [High] Missing netif_stop_queue() in ixxat_usb_start_xmit causes a CPU spin when TX resources are exhausted.
- [Medium] The devdata and cmdbuf structures are leaked during USB probe if no CAN endpoints are found or if the first controller fails to initialize.
- [Medium] Missing state change check in ixxat_usb_handle_status may flood userspace with redundant error frames.
- [Medium] Uninitialized kernel stack memory is leaked to the USB device during CAN message transmission.
- [Medium] Unaligned memory access when casting packed byte arrays to __le32 pointer.
--

--- Patch [4]: [PATCH 4/4] can: usb: ixxat_usb: add core driver and Kconfig/Makefile ---
commit 8336d17fb638f567613895aa76d6c98b3f55faa7
Author: Stéphane Grosjean <s.grosjean@peak-system.fr>
Subject: can: usb: ixxat_usb: add core driver and Kconfig/Makefile

This commit adds the main USB driver entry point for the IXXAT USB-to-CAN 
adapter family. It implements device matching, protocol selection, per-channel 
net_device allocation, URB management, and TX/RX data paths.

> --- /dev/null
> +++ b/drivers/net/can/usb/ixxat_usb/ixxat_usb_core.c
> @@ -0,0 +1,2567 @@
[ ... ]
> +static int ixxat_usb_send_cmd_internal(struct usb_device *dev,
> +				       struct ixxat_usb_device_data *devdata,
> +				       const u16 port, void *req,
> +				       const u16 req_size, void *res,
> +				       const u16 res_size,
> +				       const unsigned long cmd_delay)
> +{
> +	struct ixxat_usb_dal_req *dal_req = req;
> +	struct ixxat_usb_dal_res *dal_res = res;
> +	u8 *req_buf = (u8 *)devdata->cmdbuf;
> +	u8 *res_buf = (u8 *)devdata->cmdbuf + req_size;

[Severity: High]
Does this offset calculation overflow the allocation for the command buffer?

The cmdbuf is allocated to the size of union ixxat_usb_cmd. The response
buffer is offset by req_size, and the driver reads res_size bytes into it.

Since req_size includes both sizeof(cmd.req) and sizeof(cmd.res), and
res_size is sizeof(cmd) - sizeof(cmd.req), the total span accessed reaches
sizeof(cmd) + sizeof(cmd.res), overrunning the union allocation by 12 bytes.

[ ... ]
> +static void ixxat_usb_free_usb_communication(struct ixxat_usb_candevice *dev)
> +{
> +	struct net_device *netdev = dev->netdev;
> +	u32 skb_idx, urb_idx;
> +
> +	netif_stop_queue(netdev);
> +	usb_kill_anchored_urbs(&dev->rx_anchor);
> +	usb_kill_anchored_urbs(&dev->tx_anchor);
> +	atomic_set(&dev->active_tx_urbs, 0);
> +
> +	/* reset msg idx store */
> +	ixxat_usb_msg_free_idx(dev, 0xFFFFFFFF);
> +
> +	for (skb_idx = 0; skb_idx < dev->can.echo_skb_max; skb_idx++)
> +		can_free_echo_skb(netdev, skb_idx, NULL);
> +
> +	for (urb_idx = 0; urb_idx < IXXAT_USB_MAX_TX_URBS; urb_idx++)
> +		dev->tx_contexts[urb_idx].urb_index = IXXAT_USB_FREE_ENTRY;
> +}

[Severity: High]
Does this leak the URBs and transfer buffers allocated for transmission?

In ixxat_usb_setup_tx_urbs, the driver dynamically allocates URBs and
transfer buffers for TX. When stopping the interface here, the URBs are
never freed via usb_free_urb. 

If the interface is brought down and back up, the setup function will blindly
allocate new URBs and overwrite the pointers, permanently leaking the previous
allocations.

[ ... ]
> +static int ixxat_usb_handle_status(struct ixxat_usb_candevice *dev,
> +				   struct ixxat_can_msg *rx)
> +{
> +	struct net_device *netdev = dev->netdev;
> +	struct can_frame *can_frame;
> +	struct sk_buff *skb;
> +	enum can_state new_state = CAN_STATE_ERROR_ACTIVE;
> +	u32 raw_status;
> +	u8 min_size = sizeof(rx->base) + sizeof(raw_status);
> +
> +	if (dev->adapter == &usb2can_cl1)
> +		min_size += sizeof(rx->cl1) - sizeof(rx->cl1.data);
> +	else
> +		min_size += sizeof(rx->cl2) - sizeof(rx->cl2.data);
> +
> +	if (rx->base.size < (min_size - 1)) {
> +		netdev_err(netdev, "Error: CAN Invalid status message size\n");
> +		return -EBADMSG;
> +	}
> +
> +	raw_status = (dev->adapter == &usb2can_cl1) ?
> +		le32_to_cpup((__le32 *)rx->cl1.data) :
> +		le32_to_cpup((__le32 *)rx->cl2.data);

[Severity: Medium]
Can this pointer cast trigger unaligned memory accesses?

Casting a u8 array embedded inside a __packed structure directly to a
__le32 pointer discards the packed attribute. This allows the compiler to
generate an aligned 32-bit load instruction, which may trap or panic on
architectures that enforce strict alignment if the payload offset is unaligned.

[ ... ]
> +	if (new_state == CAN_STATE_ERROR_ACTIVE) {
> +		dev->bec.txerr = 0;
> +		dev->bec.rxerr = 0;
> +	}
> +
> +	if (new_state != CAN_STATE_MAX)
> +		dev->can.state = new_state;
> +
> +	skb = alloc_can_err_skb(netdev, &can_frame);

[Severity: Medium]
Is it intended to allocate and emit an error frame on every status update
even if the hardware state has not changed?

This logic bypasses the standard can_change_state checks and will emit a
stream of CAN_ERR_CRTL active error frames into userspace on every periodic
status message from the controller if the state remains active.

[ ... ]
> +static int ixxat_usb_decode_buf(struct urb *urb)
> +{
> +	struct ixxat_usb_candevice *dev = urb->context;
> +	struct net_device *netdev = dev->netdev;
> +	int err = 0;
> +	u8 *data = urb->transfer_buffer;
> +	u32 len = urb->actual_length, pos, size;
> +	u64 time;
> +
> +	for (pos = 0; pos < len; pos += size) {
> +		struct ixxat_can_msg can_msg;
> +		u32 type;
> +
> +		/* Since struct ixxat_can_msg is packed and starts with a byte,
> +		 * we have no choice but to copy the whole message into a local
> +		 * variable to avoid bus violation.
> +		 */
> +		size = data[pos] + 1;
> +		if (size > sizeof(can_msg) ||
> +		    size < sizeof(struct ixxat_can_msg_base)) {
> +			netdev_err(netdev, "Error: USB Invalid msg size %u\n",
> +				   size);
> +			return -EBADMSG;
> +		}
> +
> +		memcpy(&can_msg, data + pos, size);
> +		if (!can_msg.base.size) {
> +			netdev_err(netdev, "Error: USB Unsupported msg size\n");
> +			return -EOPNOTSUPP;
> +		}
> +
> +		size = can_msg.base.size + 1;
> +		if (size < sizeof(can_msg.base) || (pos + size) > len) {

[Severity: High]
Could this memcpy result in an out-of-bounds heap read?

The code copies 'size' bytes (calculated from the untrusted data buffer)
into the can_msg stack variable before verifying if pos + size > len. A
truncated or malformed USB payload can cause the memcpy to read past the end
of the URB transfer buffer.

[ ... ]
> +static int ixxat_usb_encode_msg(struct ixxat_usb_candevice *dev,
> +				struct sk_buff *skb, u8 *obuf)
> +{
> +	int size;
> +	struct canfd_frame *cf = (struct canfd_frame *)skb->data;
> +	struct ixxat_can_msg can_msg;

[Severity: Medium]
Does this expose uninitialized kernel stack memory to the USB device?

The can_msg struct is allocated on the stack without zero-initialization.
Not all members and padding bytes are explicitly written before the entire
relevant length of the structure is copied into obuf and sent over the USB bus,
potentially leaking stale kernel stack bytes.

[ ... ]
> +static void ixxat_usb_read_bulk_callback(struct urb *urb)
> +{
> +	struct ixxat_usb_candevice *dev = urb->context;
> +	struct net_device *netdev;
> +	int err;
> +
> +	if (WARN_ON_ONCE(!dev))
> +		return;
> +
> +	netdev = dev->netdev;
> +
> +	err = ixxat_evaluate_usb_status(netdev, urb, dev->ep_msg_in);
> +	switch (err) {
> +	case 0:
> +		if (urb->actual_length > 0 &&
> +		    dev->state & IXXAT_USB_STATE_STARTED) {
> +			err = ixxat_usb_decode_buf(urb);
> +			if (err)
> +				return;

[Severity: High]
Will an error from ixxat_usb_decode_buf permanently halt the RX path?

Returning early on a decoding error or memory allocation failure entirely skips
the usb_submit_urb block at the end of the callback. This leaves the endpoint
un-armed and permanently wedges the receive path until the device is restarted.

[ ... ]
> +static void ixxat_usb_write_bulk_callback(struct urb *urb)
> +{
> +	struct ixxat_tx_urb_context *context = urb->context;
> +	struct ixxat_usb_candevice *dev;
> +	struct net_device *netdev;
> +	int echo_bytes;
> +	u32 msg_idx;
> +	int err;
> +
> +	if (WARN_ON_ONCE(!context))
> +		return;
> +
> +	dev = context->dev;
> +	netdev = dev->netdev;
> +
> +	err = ixxat_evaluate_usb_status(netdev, urb, dev->ep_msg_out);
> +	if (err == -1)
> +		return;

[Severity: High]
Does this early return permanently leak the TX context on failure?

If a status error occurs, returning early skips the cleanup block that frees
the message index and decrements active_tx_urbs. This permanently wedges
the transmission path once all contexts are exhausted by transient USB errors.

[ ... ]
> +static netdev_tx_t ixxat_usb_start_xmit(struct sk_buff *skb,
> +					struct net_device *netdev)
> +{
> +	int err, size;
> +	struct ixxat_usb_candevice *dev = netdev_priv(netdev);
> +	struct ixxat_tx_urb_context *context;
> +	struct net_device_stats *stats = &netdev->stats;
> +	struct urb *urb;
> +	u8 *obuf;
> +	u32 msg_idx;
> +
> +	if (can_dev_dropped_skb(netdev, skb))
> +		return NETDEV_TX_OK;
> +
> +	/* find free URB */
> +	context = ixxat_usb_get_tx_context(dev);
> +	if (!context)
> +		return NETDEV_TX_BUSY;

[Severity: High]
Does this omit pausing the transmission queue when TX resources are empty?

Returning NETDEV_TX_BUSY without first calling netif_stop_queue causes the
kernel's queueing discipline to immediately retry transmitting the exact same
packet. This creates an infinite spin loop that consumes 100% of the CPU in
softirq context until an active TX URB completes.

[ ... ]
> +	err = usb_submit_urb(urb, GFP_ATOMIC);
> +	if (err) {
> +		/* submit failed. Should only free if it's exist */
> +		can_free_echo_skb(netdev, msg_idx, NULL);
> +		ixxat_usb_msg_free_idx(dev, msg_idx);
> +		ixxat_usb_rel_tx_context(dev, context);
> +
> +		usb_unanchor_urb(urb);
> +		atomic_dec(&dev->active_tx_urbs);
> +
> +		if (err == -ENODEV) {
> +			netif_device_detach(netdev);
> +		} else {
> +			stats->tx_dropped++;
> +			netdev_err(netdev, "Submitting tx-urb failed err %d\n",
> +				   err);
> +		}
> +	} else {
> +		/* update trans_start */
> +		netif_trans_update(netdev);
> +	}
> +
> +	return err;

[Severity: Critical]
Is this returning a negative error code instead of a valid netdev_tx_t?

When usb_submit_urb fails, the driver explicitly frees the SKB via
can_free_echo_skb but then propagates the negative error code (such as
-ENOMEM).

The ndo_start_xmit API expects a netdev_tx_t constant like NETDEV_TX_OK.
Returning a negative value causes the network core's dev_hard_start_xmit to
hit the default case and call kfree_skb on the already-freed SKB, triggering
a double-free.

[ ... ]
> +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)
> +{
[ ... ]
> +	SET_NETDEV_DEV(netdev, &intf->dev);
> +	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);
> +	if (err < 0) {
> +		netdev_err(netdev, "Error: %d: sysfs group failed\n", err);
> +		goto free_candev;
> +	}
> +
> +	netdev_info(netdev, "%.*s: Connected channel %u (device %.*s)\n",
> +		    (int)sizeof(devdata->dev_info.device_name),
> +		    devdata->dev_info.device_name, ctrl_index,
> +		    (int)sizeof(devdata->dev_info.device_id),
> +		    devdata->dev_info.device_id);
> +
> +	return err;
> +
> +free_candev:
> +	sysfs_remove_group(&netdev->dev.kobj, &ixxat_pdev_group);
> +	usb_set_intfdata(intf, dev->prev_dev);
> +	free_candev(netdev);
> +
> +	return err;
> +}

[Severity: Critical]
Does this error path fail to unregister the network device before freeing it?

The network device is registered to the core network stack earlier in this
function via register_candev. If sysfs_create_group subsequently fails, the
code jumps to the free_candev label and releases the memory, but misses a call
to unregister_candev(netdev). This leaves the core network stack with a
dangling pointer, leading to a use-after-free.

[ ... ]
> +static int ixxat_usb_probe(struct usb_interface *intf,
> +			   const struct usb_device_id *id)
> +{
[ ... ]
> +	ctrl_count = le16_to_cpu(dev_caps.bus_ctrl_count);
> +	for (ctrlidx = 0; ctrlidx < ctrl_count; ctrlidx++) {
> +		u16 dev_bustype = le16_to_cpu(dev_caps.bus_ctrl_types[ctrlidx]);
> +		u8 bustype = IXXAT_USB_BUS_TYPE(dev_bustype);
> +
> +		if (bustype != IXXAT_USB_BUS_CAN)
> +			continue;
> +
> +		err = ixxat_usb_create_ctrl(intf, adapter, ctrlidx,
> +					    devdata);
> +		if (err) {
> +			/* deregister already created devices, free devdata
> +			 * and return immediately
> +			 */
> +			ixxat_usb_disconnect(intf);
> +			return err;
> +		}
> +	}
> +
> +	return 0;
> +
> +lbl_err:
> +	kfree(cmdbuf);
> +	kfree(devdata);
> +	return err;
> +}

[Severity: Medium]
Will this early return leak the devdata and cmdbuf allocations?

If ixxat_usb_create_ctrl fails for the very first controller, it sets intfdata
back to NULL. Calling ixxat_usb_disconnect then returns early because
usb_get_intfdata is NULL, which skips the kfree(devdata) and
kfree(devdata->cmdbuf) cleanup. The probe function returns the error directly
without falling through to the lbl_err cleanup block.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260723072955.32575-1-stephane.grosjean@free.fr?part=4