[PATCH] can: esd_usb: add endpoint type validation

[PATCH] can: esd_usb: add endpoint type validation

[Ziyi Guo]

esd_usb_probe() constructs bulk pipes for two endpoints without
verifying their transfer types:

  - usb_rcvbulkpipe(dev->udev, 1) for RX (version reply, async RX data)
  - usb_sndbulkpipe(dev->udev, 2) for TX (version query, CAN frames)

A malformed USB device can present these endpoints with transfer types
that differ from what the driver assumes, triggering the WARNING in
usb_submit_urb().

Add usb_check_bulk_endpoints() before the first bulk transfer to verify
endpoint types, rejecting devices with mismatched descriptors at probe
time.

Similar to
- commit 90b7f2961798 ("net: usb: rtl8150: enable basic endpoint checking")
  which established the usb_check_bulk_endpoints() validation pattern.

Fixes: 96d8e90382dc ("can: Add driver for esd CAN-USB/2 device")
Signed-off-by: Ziyi Guo <n7l8m4@u.northwestern.edu>
---
 drivers/net/can/usb/esd_usb.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/drivers/net/can/usb/esd_usb.c b/drivers/net/can/usb/esd_usb.c
index 8cc924c47042..054ded490eb3 100644
--- a/drivers/net/can/usb/esd_usb.c
+++ b/drivers/net/can/usb/esd_usb.c
@@ -1301,6 +1301,10 @@ static int esd_usb_probe(struct usb_interface *intf,
 	struct esd_usb *dev;
 	union esd_usb_msg *msg;
 	int i, err;
+	static const u8 bulk_ep_addr[] = {
+		USB_DIR_IN | 1,		/* EP 1 IN  (RX) */
+		USB_DIR_OUT | 2,	/* EP 2 OUT (TX) */
+		0};
 
 	dev = kzalloc(sizeof(*dev), GFP_KERNEL);
 	if (!dev) {
@@ -1320,6 +1324,13 @@ static int esd_usb_probe(struct usb_interface *intf,
 		goto free_msg;
 	}
 
+	/* Verify that the required bulk endpoints are present */
+	if (!usb_check_bulk_endpoints(intf, bulk_ep_addr)) {
+		dev_err(&intf->dev, "Missing or invalid bulk endpoints\n");
+		err = -ENODEV;
+		goto free_msg;
+	}
+
 	/* 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 */
-- 
2.34.1

Re: [PATCH] can: esd_usb: add endpoint type validation

[Vincent Mailhol]

On 10/02/2026 at 04:59, Ziyi Guo wrote:
> esd_usb_probe() constructs bulk pipes for two endpoints without
> verifying their transfer types:
> 
>   - usb_rcvbulkpipe(dev->udev, 1) for RX (version reply, async RX data)
>   - usb_sndbulkpipe(dev->udev, 2) for TX (version query, CAN frames)
> 
> A malformed USB device can present these endpoints with transfer types
> that differ from what the driver assumes, triggering the WARNING in
> usb_submit_urb().
> 
> Add usb_check_bulk_endpoints() before the first bulk transfer to verify
> endpoint types, rejecting devices with mismatched descriptors at probe
> time.
> 
> Similar to
> - commit 90b7f2961798 ("net: usb: rtl8150: enable basic endpoint checking")
>   which established the usb_check_bulk_endpoints() validation pattern.
> 
> Fixes: 96d8e90382dc ("can: Add driver for esd CAN-USB/2 device")
> Signed-off-by: Ziyi Guo <n7l8m4@u.northwestern.edu>
> ---
>  drivers/net/can/usb/esd_usb.c | 11 +++++++++++
>  1 file changed, 11 insertions(+)
> 
> diff --git a/drivers/net/can/usb/esd_usb.c b/drivers/net/can/usb/esd_usb.c
> index 8cc924c47042..054ded490eb3 100644
> --- a/drivers/net/can/usb/esd_usb.c
> +++ b/drivers/net/can/usb/esd_usb.c
> @@ -1301,6 +1301,10 @@ static int esd_usb_probe(struct usb_interface *intf,
>  	struct esd_usb *dev;
>  	union esd_usb_msg *msg;
>  	int i, err;
> +	static const u8 bulk_ep_addr[] = {
> +		USB_DIR_IN | 1,		/* EP 1 IN  (RX) */
> +		USB_DIR_OUT | 2,	/* EP 2 OUT (TX) */
> +		0};

Instead of using magic numbers with comments declare a macro:

#define ESD_USB_EP_IN 1
#define ESD_USB_EP_OUT 2

and use it throughout the driver.

>  	dev = kzalloc(sizeof(*dev), GFP_KERNEL);
>  	if (!dev) {
> @@ -1320,6 +1324,13 @@ static int esd_usb_probe(struct usb_interface *intf,
>  		goto free_msg;
>  	}
>  
> +	/* Verify that the required bulk endpoints are present */
> +	if (!usb_check_bulk_endpoints(intf, bulk_ep_addr)) {
> +		dev_err(&intf->dev, "Missing or invalid bulk endpoints\n");
> +		err = -ENODEV;
> +		goto free_msg;
> +	}

usb_check_bulk_endpoints() calls usb_find_endpoint(). Here, I would
rather just call usb_find_endpoint() and use the found endpoints. Like
this:

------------8<------------
diff --git a/drivers/net/can/usb/esd_usb.c b/drivers/net/can/usb/esd_usb.c
index 8cc924c47042b..d2261046be91d 100644
--- a/drivers/net/can/usb/esd_usb.c
+++ b/drivers/net/can/usb/esd_usb.c
@@ -272,6 +272,9 @@ struct esd_usb {
 
 	struct usb_anchor rx_submitted;
 
+	unsigned int rx_pipe;
+	unsigned int tx_pipe;
+
 	int net_count;
 	u32 version;
 	int rxinitdone;
@@ -537,7 +540,7 @@ static void esd_usb_read_bulk_callback(struct urb *urb)
 	}
 
 resubmit_urb:
-	usb_fill_bulk_urb(urb, dev->udev, usb_rcvbulkpipe(dev->udev, 1),
+	usb_fill_bulk_urb(urb, dev->udev, dev->rx_pipe,
 			  urb->transfer_buffer, ESD_USB_RX_BUFFER_SIZE,
 			  esd_usb_read_bulk_callback, dev);
 
@@ -626,9 +629,7 @@ static int esd_usb_send_msg(struct esd_usb *dev, union esd_usb_msg *msg)
 {
 	int actual_length;
 
-	return usb_bulk_msg(dev->udev,
-			    usb_sndbulkpipe(dev->udev, 2),
-			    msg,
+	return usb_bulk_msg(dev->udev, dev->tx_pipe, msg,
 			    msg->hdr.len * sizeof(u32), /* convert to # of bytes */
 			    &actual_length,
 			    1000);
@@ -639,12 +640,8 @@ static int esd_usb_wait_msg(struct esd_usb *dev,
 {
 	int actual_length;
 
-	return usb_bulk_msg(dev->udev,
-			    usb_rcvbulkpipe(dev->udev, 1),
-			    msg,
-			    sizeof(*msg),
-			    &actual_length,
-			    1000);
+	return usb_bulk_msg(dev->udev, dev->rx_pipe, msg,
+			    sizeof(*msg), &actual_length, 1000);
 }
 
 static int esd_usb_setup_rx_urbs(struct esd_usb *dev)
@@ -677,8 +674,7 @@ static int esd_usb_setup_rx_urbs(struct esd_usb *dev)
 
 		urb->transfer_dma = buf_dma;
 
-		usb_fill_bulk_urb(urb, dev->udev,
-				  usb_rcvbulkpipe(dev->udev, 1),
+		usb_fill_bulk_urb(urb, dev->udev, dev->tx_pipe,
 				  buf, ESD_USB_RX_BUFFER_SIZE,
 				  esd_usb_read_bulk_callback, dev);
 		urb->transfer_flags |= URB_NO_TRANSFER_DMA_MAP;
@@ -903,7 +899,7 @@ static netdev_tx_t esd_usb_start_xmit(struct sk_buff *skb,
 	/* hnd must not be 0 - MSB is stripped in txdone handling */
 	msg->tx.hnd = BIT(31) | i; /* returned in TX done message */
 
-	usb_fill_bulk_urb(urb, dev->udev, usb_sndbulkpipe(dev->udev, 2), buf,
+	usb_fill_bulk_urb(urb, dev->udev, dev->tx_pipe, buf,
 			  msg->hdr.len * sizeof(u32), /* convert to # of bytes */
 			  esd_usb_write_bulk_callback, context);
 
@@ -1298,10 +1294,16 @@ static int esd_usb_probe_one_net(struct usb_interface *intf, int index)
 static int esd_usb_probe(struct usb_interface *intf,
 			 const struct usb_device_id *id)
 {
+	struct usb_endpoint_descriptor *ep_in, *ep_out;
 	struct esd_usb *dev;
 	union esd_usb_msg *msg;
 	int i, err;
 
+	err = usb_find_common_endpoints(intf->cur_altsetting, &ep_in, &ep_out,
+					NULL, NULL);
+	if (err)
+		return err;
+
 	dev = kzalloc(sizeof(*dev), GFP_KERNEL);
 	if (!dev) {
 		err = -ENOMEM;
@@ -1309,6 +1311,8 @@ static int esd_usb_probe(struct usb_interface *intf,
 	}
 
 	dev->udev = interface_to_usbdev(intf);
+	dev->rx_pipe = usb_rcvbulkpipe(dev->udev, ep_in->bEndpointAddress);
+	dev->tx_pipe = usb_sndbulkpipe(dev->udev, ep_out->bEndpointAddress);
 
 	init_usb_anchor(&dev->rx_submitted);
 
------------>8------------

This also enforce that we are using existing endpoints while being
more concise.


Yours sincerely,
Vincent Mailhol

Re: [PATCH] can: esd_usb: add endpoint type validation

[Ziyi Guo]

On Fri, Feb 13, 2026 at 11:01 AM Vincent Mailhol <mailhol@kernel.org> wrote:
> Instead of using magic numbers with comments declare a macro:
>
> #define ESD_USB_EP_IN 1
> #define ESD_USB_EP_OUT 2
>
> and use it throughout the driver.
>
>
> This also enforce that we are using existing endpoints while being
> more concise.
>

Hi Vincent,

Thank you so much for your time and review, I'll revise the code and
send a v2 version patch!

Re: [PATCH] can: esd_usb: add endpoint type validation

[Vincent Mailhol]

On 13/02/2026 at 17:01, Vincent Mailhol wrote:
> On 10/02/2026 at 04:59, Ziyi Guo wrote:
>> esd_usb_probe() constructs bulk pipes for two endpoints without
>> verifying their transfer types:
>>
>>   - usb_rcvbulkpipe(dev->udev, 1) for RX (version reply, async RX data)
>>   - usb_sndbulkpipe(dev->udev, 2) for TX (version query, CAN frames)
>>
>> A malformed USB device can present these endpoints with transfer types
>> that differ from what the driver assumes, triggering the WARNING in
>> usb_submit_urb().
>>
>> Add usb_check_bulk_endpoints() before the first bulk transfer to verify
>> endpoint types, rejecting devices with mismatched descriptors at probe
>> time.
>>
>> Similar to
>> - commit 90b7f2961798 ("net: usb: rtl8150: enable basic endpoint checking")
>>   which established the usb_check_bulk_endpoints() validation pattern.
>>
>> Fixes: 96d8e90382dc ("can: Add driver for esd CAN-USB/2 device")
>> Signed-off-by: Ziyi Guo <n7l8m4@u.northwestern.edu>
>> ---
>>  drivers/net/can/usb/esd_usb.c | 11 +++++++++++
>>  1 file changed, 11 insertions(+)
>>
>> diff --git a/drivers/net/can/usb/esd_usb.c b/drivers/net/can/usb/esd_usb.c
>> index 8cc924c47042..054ded490eb3 100644
>> --- a/drivers/net/can/usb/esd_usb.c
>> +++ b/drivers/net/can/usb/esd_usb.c
>> @@ -1301,6 +1301,10 @@ static int esd_usb_probe(struct usb_interface *intf,
>>  	struct esd_usb *dev;
>>  	union esd_usb_msg *msg;
>>  	int i, err;
>> +	static const u8 bulk_ep_addr[] = {
>> +		USB_DIR_IN | 1,		/* EP 1 IN  (RX) */
>> +		USB_DIR_OUT | 2,	/* EP 2 OUT (TX) */
>> +		0};
> 
> Instead of using magic numbers with comments declare a macro:
> 
> #define ESD_USB_EP_IN 1
> #define ESD_USB_EP_OUT 2
> 
> and use it throughout the driver.
> 
>>  	dev = kzalloc(sizeof(*dev), GFP_KERNEL);
>>  	if (!dev) {
>> @@ -1320,6 +1324,13 @@ static int esd_usb_probe(struct usb_interface *intf,
>>  		goto free_msg;
>>  	}
>>  
>> +	/* Verify that the required bulk endpoints are present */
>> +	if (!usb_check_bulk_endpoints(intf, bulk_ep_addr)) {
>> +		dev_err(&intf->dev, "Missing or invalid bulk endpoints\n");
>> +		err = -ENODEV;
>> +		goto free_msg;
>> +	}
> 
> usb_check_bulk_endpoints() calls usb_find_endpoint(). Here, I would
> rather just call usb_find_endpoint() and use the found endpoints. Like
> this:
> 
> ------------8<------------
> diff --git a/drivers/net/can/usb/esd_usb.c b/drivers/net/can/usb/esd_usb.c
> index 8cc924c47042b..d2261046be91d 100644
> --- a/drivers/net/can/usb/esd_usb.c
> +++ b/drivers/net/can/usb/esd_usb.c
> @@ -272,6 +272,9 @@ struct esd_usb {
>  
>  	struct usb_anchor rx_submitted;
>  
> +	unsigned int rx_pipe;
> +	unsigned int tx_pipe;
> +
>  	int net_count;
>  	u32 version;
>  	int rxinitdone;
> @@ -537,7 +540,7 @@ static void esd_usb_read_bulk_callback(struct urb *urb)
>  	}
>  
>  resubmit_urb:
> -	usb_fill_bulk_urb(urb, dev->udev, usb_rcvbulkpipe(dev->udev, 1),
> +	usb_fill_bulk_urb(urb, dev->udev, dev->rx_pipe,
>  			  urb->transfer_buffer, ESD_USB_RX_BUFFER_SIZE,
>  			  esd_usb_read_bulk_callback, dev);
>  
> @@ -626,9 +629,7 @@ static int esd_usb_send_msg(struct esd_usb *dev, union esd_usb_msg *msg)
>  {
>  	int actual_length;
>  
> -	return usb_bulk_msg(dev->udev,
> -			    usb_sndbulkpipe(dev->udev, 2),
> -			    msg,
> +	return usb_bulk_msg(dev->udev, dev->tx_pipe, msg,
>  			    msg->hdr.len * sizeof(u32), /* convert to # of bytes */
>  			    &actual_length,
>  			    1000);
> @@ -639,12 +640,8 @@ static int esd_usb_wait_msg(struct esd_usb *dev,
>  {
>  	int actual_length;
>  
> -	return usb_bulk_msg(dev->udev,
> -			    usb_rcvbulkpipe(dev->udev, 1),
> -			    msg,
> -			    sizeof(*msg),
> -			    &actual_length,
> -			    1000);
> +	return usb_bulk_msg(dev->udev, dev->rx_pipe, msg,
> +			    sizeof(*msg), &actual_length, 1000);
>  }
>  
>  static int esd_usb_setup_rx_urbs(struct esd_usb *dev)
> @@ -677,8 +674,7 @@ static int esd_usb_setup_rx_urbs(struct esd_usb *dev)
>  
>  		urb->transfer_dma = buf_dma;
>  
> -		usb_fill_bulk_urb(urb, dev->udev,
> -				  usb_rcvbulkpipe(dev->udev, 1),
> +		usb_fill_bulk_urb(urb, dev->udev, dev->tx_pipe,
                                                       ^^^^^^^
Typo: this is meant to be rx_pipe.

>  				  buf, ESD_USB_RX_BUFFER_SIZE,
>  				  esd_usb_read_bulk_callback, dev);
>  		urb->transfer_flags |= URB_NO_TRANSFER_DMA_MAP;
> @@ -903,7 +899,7 @@ static netdev_tx_t esd_usb_start_xmit(struct sk_buff *skb,
>  	/* hnd must not be 0 - MSB is stripped in txdone handling */
>  	msg->tx.hnd = BIT(31) | i; /* returned in TX done message */
>  
> -	usb_fill_bulk_urb(urb, dev->udev, usb_sndbulkpipe(dev->udev, 2), buf,
> +	usb_fill_bulk_urb(urb, dev->udev, dev->tx_pipe, buf,
>  			  msg->hdr.len * sizeof(u32), /* convert to # of bytes */
>  			  esd_usb_write_bulk_callback, context);
>  
> @@ -1298,10 +1294,16 @@ static int esd_usb_probe_one_net(struct usb_interface *intf, int index)
>  static int esd_usb_probe(struct usb_interface *intf,
>  			 const struct usb_device_id *id)
>  {
> +	struct usb_endpoint_descriptor *ep_in, *ep_out;
>  	struct esd_usb *dev;
>  	union esd_usb_msg *msg;
>  	int i, err;
>  
> +	err = usb_find_common_endpoints(intf->cur_altsetting, &ep_in, &ep_out,
> +					NULL, NULL);
> +	if (err)
> +		return err;
> +
>  	dev = kzalloc(sizeof(*dev), GFP_KERNEL);
>  	if (!dev) {
>  		err = -ENOMEM;
> @@ -1309,6 +1311,8 @@ static int esd_usb_probe(struct usb_interface *intf,
>  	}
>  
>  	dev->udev = interface_to_usbdev(intf);
> +	dev->rx_pipe = usb_rcvbulkpipe(dev->udev, ep_in->bEndpointAddress);
> +	dev->tx_pipe = usb_sndbulkpipe(dev->udev, ep_out->bEndpointAddress);
>  
>  	init_usb_anchor(&dev->rx_submitted);
>  
> ------------>8------------
> 
> This also enforce that we are using existing endpoints while being
> more concise.


Yours sincerely,
Vincent Mailhol