Message-ID: <20250813-just-independent-angelfish-909305-mkl@pengutronix.de>
Date: Wed, 13 Aug 2025 10:26:17 +0200
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, socketcan@....eu, 
	Simon Horman <horms@...nel.org>, Olivier Sobrie <olivier@...rie.be>, 
	Oliver Hartkopp <socketcan@...tkopp.net>, netdev@...r.kernel.org
Subject: Re: [PATCH 2/6] can: esd_usb: Fix not detecting version reply in
 probe routine

On 11.08.2025 23:06:07, 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.
> 
> To mitigate these problems:
> - Use a transfer buffer "buf" with ESD_USB_RX_BUFFER_SIZE.
> - 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.
> - The data drain loop is limited by a maximum # of bytes to read from
>   the device based on its internal buffer sizes and a timeout
>   ESD_USB_DRAIN_TIMEOUT_MS.
> 
> This version of the patch incorporates changes recommended on the
> linux-can list for a first version.
> 
> References:
> https://lore.kernel.org/linux-can/d7fd564775351ea8a60a6ada83a0368a99ea6b19.camel@esd.eu/#r
> 
> 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 | 125 ++++++++++++++++++++++++++--------
>  1 file changed, 97 insertions(+), 28 deletions(-)
> 
> diff --git a/drivers/net/can/usb/esd_usb.c b/drivers/net/can/usb/esd_usb.c
> index 05ed664cf59d..dbdfffe3a4a0 100644
> --- a/drivers/net/can/usb/esd_usb.c
> +++ b/drivers/net/can/usb/esd_usb.c
> @@ -44,6 +44,9 @@ MODULE_LICENSE("GPL v2");
>  #define ESD_USB_CMD_TS			5 /* also used for TS_REPLY */
>  #define ESD_USB_CMD_IDADD		6 /* also used for IDADD_REPLY */
>  
> +/* esd version message name size */
> +#define ESD_USB_FW_NAME_SZ 16
> +
>  /* esd CAN message flags - dlc field */
>  #define ESD_USB_RTR	BIT(4)
>  #define ESD_USB_NO_BRS	BIT(4)
> @@ -95,6 +98,7 @@ MODULE_LICENSE("GPL v2");
>  #define ESD_USB_RX_BUFFER_SIZE		1024
>  #define ESD_USB_MAX_RX_URBS		4
>  #define ESD_USB_MAX_TX_URBS		16 /* must be power of 2 */
> +#define ESD_USB_DRAIN_TIMEOUT_MS	100
>  
>  /* Modes for CAN-USB/3, to be used for esd_usb_3_set_baudrate_msg_x.mode */
>  #define ESD_USB_3_BAUDRATE_MODE_DISABLE		0 /* remove from bus */
> @@ -131,7 +135,7 @@ struct esd_usb_version_reply_msg {
>  	u8 nets;
>  	u8 features;
>  	__le32 version;
> -	u8 name[16];
> +	u8 name[ESD_USB_FW_NAME_SZ];
>  	__le32 rsvd;
>  	__le32 ts;
>  };
> @@ -625,17 +629,91 @@ 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)
>  {
> -	int actual_length;
> +	union esd_usb_msg *msg = buf;
>  
> -	return usb_bulk_msg(dev->udev,
> -			    usb_rcvbulkpipe(dev->udev, 1),
> -			    msg,
> -			    sizeof(*msg),
> -			    &actual_length,
> -			    1000);
> +	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)
> +{
> +	/* Device hardware has 2 RX buffers with ESD_USB_RX_BUFFER_SIZE, * 4 to give some slack. */
> +	const int max_drain_bytes = (4 * ESD_USB_RX_BUFFER_SIZE);
> +	unsigned long end_jiffies;
> +	int cnt_other = 0;
> +	int cnt_ts = 0;
> +	int cnt_vs = 0;
> +	int len_sum = 0;
> +	int attempt = 0;
> +	int err;
> +
> +	end_jiffies = jiffies + msecs_to_jiffies(ESD_USB_DRAIN_TIMEOUT_MS);
> +	do {
> +		int actual_length;
> +		int pos;
> +
> +		err = usb_bulk_msg(dev->udev,
> +				   usb_rcvbulkpipe(dev->udev, 1),
> +				   rx_buf,
> +				   ESD_USB_RX_BUFFER_SIZE,
> +				   &actual_length,
> +				   ESD_USB_DRAIN_TIMEOUT_MS);
> +		dev_dbg(&dev->udev->dev, "AT %d, LEN %d, ERR %d\n", attempt, actual_length, err);
> +		if (err)
> +			goto bail;
> +
> +		err = -ENOENT;
> +		len_sum += actual_length;
> +		pos = 0;
> +		while (pos < actual_length) {

You have to check that the actual offset you will access later is within
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;
> +				dev->net_count = min(p_msg->version_reply.nets, ESD_USB_MAX_NETS);
> +				dev->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, %.*s\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,
> +					ESD_USB_FW_NAME_SZ, p_msg->version_reply.name);
> +				break;
> +			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 */

Can pos overflow? hdr.len is a u8, so probably not.

> +			if (pos > actual_length) {
> +				dev_err(&dev->udev->dev, "format error\n");
> +				err = -EPROTO;
> +				goto bail;
> +			}
> +		}
> +		++attempt;
> +	} while (cnt_vs == 0 && len_sum < max_drain_bytes && time_before(jiffies, end_jiffies));
> +bail:
> +	dev_dbg(&dev->udev->dev, "RC=%d; ATT=%d, TS=%d, VS=%d, O=%d, B=%d\n",
> +		err, attempt, cnt_ts, cnt_vs, cnt_other, len_sum);
> +	return err;
>  }
>  
>  static int esd_usb_setup_rx_urbs(struct esd_usb *dev)
> @@ -1274,7 +1352,7 @@ 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);
> @@ -1289,34 +1367,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_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);

I'm a bit uneasy with the passing of the buffer as an argument, but not
its size.

>  	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);
>  	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");
> @@ -1333,8 +1402,8 @@ static int esd_usb_probe(struct usb_interface *intf,
>  	for (i = 0; i < dev->net_count; i++)
>  		esd_usb_probe_one_net(intf, i);
>  
> -free_msg:
> -	kfree(msg);
> +free_buf:
> +	kfree(buf);
>  free_dev:
>  	if (err)
>  		kfree(dev);
> -- 
> 2.34.1
> 
> 
> 

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