Message-ID: <20250206-wild-masked-hoatzin-b194e1-mkl@pengutronix.de>
Date: Thu, 6 Feb 2025 13:45:27 +0100
From: Marc Kleine-Budde <mkl@...gutronix.de>
To: Stefan Mätje <stefan.maetje@....eu>
Cc: Vincent Mailhol <mailhol.vincent@...adoo.fr>, 
	Frank Jungclaus <frank.jungclaus@....eu>, linux-can@...r.kernel.org, Simon Horman <horms@...nel.org>, 
	netdev@...r.kernel.org
Subject: Re: [PATCH 1/1] can: esd_usb: Fix not detecting version reply in
 probe routine

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@....eu>
> ---
>  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   |

Download attachment "signature.asc" of type "application/pgp-signature" (489 bytes)

Powered by blists - more mailing lists