On 03.02.2025 15:58:10, Stefan Mätje wrote:
> This patch fixes some problems in the esd_usb_probe routine that render
> the CAN interface unusable.
>
> The probe routine sends a version request message to the USB device to
> receive a version reply with the number of CAN ports and the hard-
> & firmware versions. Then for each CAN port a CAN netdev is registered.
>
> The previous code assumed that the version reply would be received
> immediately. But if the driver was reloaded without power cycling the
> USB device (i. e. on a reboot) there could already be other incoming
> messages in the USB buffers. These would be in front of the version
> reply and need to be skipped.
>
> In the previous code these problems were present:
> - Only one usb_bulk_msg() read was done into a buffer of
> sizeof(union esd_usb_msg) which is smaller than ESD_USB_RX_BUFFER_SIZE
> which could lead to an overflow error from the USB stack.
> - The first bytes of the received data were taken without checking for
> the message type. This could lead to zero detected CAN interfaces.
> - On kmalloc() fail for the "union esd_usb_msg msg" (i. e. msg == NULL)
> kfree() would be called with this NULL pointer.
>
> To mitigate these problems:
> - Use a transfer buffer "buf" with ESD_USB_RX_BUFFER_SIZE.
> - Fix the error exit path taken after allocation failure for the
> transfer buffer.
> - Added a function esd_usb_recv_version() that reads and skips incoming
> "esd_usb_msg" messages until a version reply message is found. This
> is evaluated to return the count of CAN ports and version information.
>
> Fixes: 80662d943075 ("can: esd_usb: Add support for esd CAN-USB/3")
> Signed-off-by: Stefan Mätje <stefan.maetje@xxxxxx>
> ---
> drivers/net/can/usb/esd_usb.c | 122 +++++++++++++++++++++++++---------
> 1 file changed, 92 insertions(+), 30 deletions(-)
>
> diff --git a/drivers/net/can/usb/esd_usb.c b/drivers/net/can/usb/esd_usb.c
> index 03ad10b01867..a6b3b100f8ac 100644
> --- a/drivers/net/can/usb/esd_usb.c
> +++ b/drivers/net/can/usb/esd_usb.c
> @@ -625,17 +625,86 @@ static int esd_usb_send_msg(struct esd_usb *dev, union esd_usb_msg *msg)
> 1000);
> }
>
> -static int esd_usb_wait_msg(struct esd_usb *dev,
> - union esd_usb_msg *msg)
> +static int esd_usb_req_version(struct esd_usb *dev, void *buf)
> +{
> + union esd_usb_msg *msg = buf;
> +
> + msg->hdr.cmd = ESD_USB_CMD_VERSION;
> + msg->hdr.len = sizeof(struct esd_usb_version_msg) / sizeof(u32); /* # of 32bit words */
> + msg->version.rsvd = 0;
> + msg->version.flags = 0;
> + msg->version.drv_version = 0;
> +
> + return esd_usb_send_msg(dev, msg);
> +}
> +
> +static int esd_usb_recv_version(struct esd_usb *dev,
> + void *rx_buf,
> + u32 *version,
> + int *net_count)
static int esd_usb_recv_version(struct esd_usb *dev, void *rx_buf,
u32 *version, int *net_count)
> {
> int actual_length;
> + int cnt_other = 0;
> + int cnt_ts = 0;
> + int cnt_vs = 0;
> + int attempt;
> + int pos;
> + int err;
Try to reduce the scope of the variables and move them into the for-loop.
>
> - return usb_bulk_msg(dev->udev,
> - usb_rcvbulkpipe(dev->udev, 1),
> - msg,
> - sizeof(*msg),
> - &actual_length,
> - 1000);
> + for (attempt = 0; attempt < 8 && cnt_vs == 0; ++attempt) {
Can you create a #define for the "8" to avoid a magic number here?
> + err = usb_bulk_msg(dev->udev,
> + usb_rcvbulkpipe(dev->udev, 1),
> + rx_buf,
> + ESD_USB_RX_BUFFER_SIZE,
> + &actual_length,
> + 1000);
> + if (err)
> + break;
nitpick: I would make it explicit with "goto bail", should be the same?
> +
> + err = -ENOENT;
> + pos = 0;
> + while (pos < actual_length) {
> + union esd_usb_msg *p_msg;
> +
> + p_msg = (union esd_usb_msg *)(rx_buf + pos);
> +
> + switch (p_msg->hdr.cmd) {
> + case ESD_USB_CMD_VERSION:
> + ++cnt_vs;
> + *net_count = (int)p_msg->version_reply.nets;
Cast not needed.
What happens if nets is > 2? Please sanitize input from outside against
ESD_USB_MAX_NETS.
> + *version = le32_to_cpu(p_msg->version_reply.version);
> + err = 0;
> + dev_dbg(&dev->udev->dev, "TS 0x%08x, V 0x%08x, N %u, F 0x%02x, %.16s\n",
> + le32_to_cpu(p_msg->version_reply.ts),
> + le32_to_cpu(p_msg->version_reply.version),
> + p_msg->version_reply.nets,
> + p_msg->version_reply.features,
> + (char *)p_msg->version_reply.name
Is this cast needed?
What about using '"%.*s", sizeof(p_msg->version_reply.name)'?
> + );
Please move the closing ")" into the previous line.
> + break;
Why keep parsing after you've found the version?
> + case ESD_USB_CMD_TS:
> + ++cnt_ts;
> + dev_dbg(&dev->udev->dev, "TS 0x%08x\n",
> + le32_to_cpu(p_msg->rx.ts));
> + break;
> + default:
> + ++cnt_other;
> + dev_dbg(&dev->udev->dev, "HDR %d\n", p_msg->hdr.cmd);
> + break;
> + }
> + pos += p_msg->hdr.len * sizeof(u32); /* convert to # of bytes */
> +
> + if (pos > actual_length) {
> + dev_err(&dev->udev->dev, "format error\n");
> + err = -EPROTO;
> + goto bail;
> + }
> + }
> + }
> +bail:
> + dev_dbg(&dev->udev->dev, "%s()->%d; ATT=%d, TS=%d, VS=%d, O=%d\n",
> + __func__, err, attempt, cnt_ts, cnt_vs, cnt_other);
> + return err;
> }
>
> static int esd_usb_setup_rx_urbs(struct esd_usb *dev)
> @@ -1258,7 +1327,7 @@ static int esd_usb_probe_one_net(struct usb_interface *intf, int index)
> }
>
> dev->nets[index] = priv;
> - netdev_info(netdev, "device %s registered\n", netdev->name);
> + netdev_info(netdev, "registered\n");
>
> done:
> return err;
> @@ -1273,13 +1342,13 @@ static int esd_usb_probe(struct usb_interface *intf,
> const struct usb_device_id *id)
> {
> struct esd_usb *dev;
> - union esd_usb_msg *msg;
> + void *buf;
> int i, err;
>
> dev = kzalloc(sizeof(*dev), GFP_KERNEL);
> if (!dev) {
> err = -ENOMEM;
> - goto done;
> + goto bail;
> }
>
> dev->udev = interface_to_usbdev(intf);
> @@ -1288,34 +1357,25 @@ static int esd_usb_probe(struct usb_interface *intf,
>
> usb_set_intfdata(intf, dev);
>
> - msg = kmalloc(sizeof(*msg), GFP_KERNEL);
> - if (!msg) {
> + buf = kmalloc(ESD_USB_RX_BUFFER_SIZE, GFP_KERNEL);
> + if (!buf) {
> err = -ENOMEM;
> - goto free_msg;
> + goto free_dev;
> }
>
> /* query number of CAN interfaces (nets) */
> - msg->hdr.cmd = ESD_USB_CMD_VERSION;
> - msg->hdr.len = sizeof(struct esd_usb_version_msg) / sizeof(u32); /* # of 32bit words */
> - msg->version.rsvd = 0;
> - msg->version.flags = 0;
> - msg->version.drv_version = 0;
> -
> - err = esd_usb_send_msg(dev, msg);
> + err = esd_usb_req_version(dev, buf);
> if (err < 0) {
> dev_err(&intf->dev, "sending version message failed\n");
> - goto free_msg;
> + goto free_buf;
> }
>
> - err = esd_usb_wait_msg(dev, msg);
> + err = esd_usb_recv_version(dev, buf, &dev->version, &dev->net_count);
Why pass the "&dev->version, &dev->net_count" pointers, if you already
pass dev?
> if (err < 0) {
> dev_err(&intf->dev, "no version message answer\n");
> - goto free_msg;
> + goto free_buf;
> }
>
> - dev->net_count = (int)msg->version_reply.nets;
> - dev->version = le32_to_cpu(msg->version_reply.version);
> -
> if (device_create_file(&intf->dev, &dev_attr_firmware))
> dev_err(&intf->dev,
> "Couldn't create device file for firmware\n");
> @@ -1332,11 +1392,12 @@ static int esd_usb_probe(struct usb_interface *intf,
> for (i = 0; i < dev->net_count; i++)
> esd_usb_probe_one_net(intf, i);
Return values are not checked here. :/
>
> -free_msg:
> - kfree(msg);
> +free_buf:
> + kfree(buf);
> +free_dev:
> if (err)
> kfree(dev);
> -done:
> +bail:
> return err;
> }
>
> @@ -1357,6 +1418,7 @@ static void esd_usb_disconnect(struct usb_interface *intf)
> for (i = 0; i < dev->net_count; i++) {
> if (dev->nets[i]) {
> netdev = dev->nets[i]->netdev;
> + netdev_info(netdev, "unregister\n");
> unregister_netdev(netdev);
> free_candev(netdev);
> }
> --
> 2.34.1
>
>
>
regards,
Marc
--
Pengutronix e.K. | Marc Kleine-Budde |
Embedded Linux | https://www.pengutronix.de |
Vertretung Nürnberg | Phone: +49-5121-206917-129 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-9 |
Attachment:
signature.asc
Description: PGP signature
