Message-ID: <abc6aa4858c869545d47fb58481b71d00697d8b1.camel@esd.eu>
Date: Tue, 19 Aug 2025 16:23:06 +0000
From: Stefan Mätje <stefan.maetje@....eu>
To: "mkl@...gutronix.de" <mkl@...gutronix.de>
CC: "socketcan@...tkopp.net" <socketcan@...tkopp.net>,
	"linux-can@...r.kernel.org" <linux-can@...r.kernel.org>, socketcan
	<socketcan@....eu>, Frank Jungclaus <frank.jungclaus@....eu>,
	"netdev@...r.kernel.org" <netdev@...r.kernel.org>, "horms@...nel.org"
	<horms@...nel.org>, "olivier@...rie.be" <olivier@...rie.be>,
	"mailhol.vincent@...adoo.fr" <mailhol.vincent@...adoo.fr>
Subject: Re: [PATCH 2/6] can: esd_usb: Fix not detecting version reply in
 probe routine

Am Mittwoch, dem 13.08.2025 um 10:26 +0200 schrieb Marc Kleine-Budde:
> 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);
> > +

I have moved the calculation of the next position and the check if it fits into
actual_length of the buffer to this point. This will do the check for access is
in actual length.

> > 
> > +			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.

pos is limited in the while loop to be < actual_length which can't be bigger than ESD_USB_RX_BUFFER_SIZE (1kB).
And "p_msg->hdr.len * sizeof(u32)" is limited to 1024 being hdr.len an u8 as you mentioned. So overflow
is not possible here.

> 
> > +			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
> 

Powered by blists - more mailing lists