Re: [PATCH] usb: core: Fix bandwidth for devices with invalid wBytesPerInterval

[Greg KH]

On Thu, Apr 02, 2026 at 02:59:35PM +0800, Xuetao (kirin) wrote:
> 
> 
> 在 2026/4/2 11:51, Greg KH 写道:
> > On Thu, Apr 02, 2026 at 10:14:00AM +0800, Tao Xue wrote:
> > > As specified in Section 4.14.2 of the xHCI Specification, the xHC
> > > reserves bandwidth for periodic endpoints according to bInterval and
> > > wBytesPerInterval (Max ESIT Payload).
> > > 
> > > Some peripherals report an invalid wBytesPerInterval in their device
> > > descriptor, which is either 0 or smaller than the actual data length
> > > transmitted. This issue is observed on ASIX AX88179 series USB 3.0
> > > Ethernet adapters.
> > > 
> > > These errors may lead to unexpected behavior on certain USB host
> > > controllers, causing USB peripherals to malfunction.
> > > 
> > > To address the issue, return max(wBytesPerInterval, max_payload) when
> > > calculating bandwidth reservation.
> > > 
> > > Fixes: 9238f25d5d32 ("USB: xhci: properly set endpoint context fields for periodic eps.")
> > > Cc: <stable@xxxxxxxxxx>
> > > Signed-off-by: Tao Xue <xuetao09@xxxxxxxxxx>
> > > ---
> > >   drivers/usb/core/usb.c | 9 ++++++++-
> > >   1 file changed, 8 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/drivers/usb/core/usb.c b/drivers/usb/core/usb.c
> > > index e9a10a33534c..8f2e05a5a015 100644
> > > --- a/drivers/usb/core/usb.c
> > > +++ b/drivers/usb/core/usb.c
> > > @@ -1125,6 +1125,8 @@ EXPORT_SYMBOL_GPL(usb_free_noncoherent);
> > >   u32 usb_endpoint_max_periodic_payload(struct usb_device *udev,
> > >   				      const struct usb_host_endpoint *ep)
> > >   {
> > > +	u32 max_payload;
> > > +
> > >   	if (!usb_endpoint_xfer_isoc(&ep->desc) &&
> > >   	    !usb_endpoint_xfer_int(&ep->desc))
> > >   		return 0;
> > > @@ -1135,7 +1137,12 @@ u32 usb_endpoint_max_periodic_payload(struct usb_device *udev,
> > >   			return le32_to_cpu(ep->ssp_isoc_ep_comp.dwBytesPerInterval);
> > >   		fallthrough;
> > >   	case USB_SPEED_SUPER:
> > > -		return le16_to_cpu(ep->ss_ep_comp.wBytesPerInterval);
> > > +		max_payload = usb_endpoint_maxp(&ep->desc) * (ep->ss_ep_comp.bMaxBurst + 1);
> > > +		if (usb_endpoint_xfer_isoc(&ep->desc))
> > > +			return max_t(u32, max_payload * USB_SS_MULT(ep->ss_ep_comp.bmAttributes),
> > > +					ep->ss_ep_comp.wBytesPerInterval);
> > > +		else
> > > +			return max_t(u32, max_payload, ep->ss_ep_comp.wBytesPerInterval);
> > 
> > You dropped the conversion from le16 to cpu?  Why?
> > 
> > thanks,
> > 
> > greg k-h
> 
> Hi Greg,
> 
> Thank you for the review.
> 
> 1、You're right, that was an oversight. I should keep the le16_to_cpu().
> Here's the corrected version:
> 
>     max_payload = usb_endpoint_maxp(&ep->desc) * (ep->ss_ep_comp.bMaxBurst +
> 1);
>     if (usb_endpoint_xfer_isoc(&ep->desc))
>         return max_t(u32, max_payload *
> USB_SS_MULT(ep->ss_ep_comp.bmAttributes),
>                         le16_to_cpu(ep->ss_ep_comp.wBytesPerInterval));
>     else
>         return max_t(u32, max_payload,
> le16_to_cpu(ep->ss_ep_comp.wBytesPerInterval));

That's hard to follow as it is line-wrapped, just fix it up for your
next version and I'll be glad to review it then.

> 2、Following Alan's suggestion in another email, should I check whether
> wBytesPerInterval is a valid value and handle it in the
> usb_parse_ss_endpoint_companion() ?
> 
> However, when parsing the device descriptor, we do not know whether the
> actual data length transmitted by the peripheral is greater than
> wBytesPerInterval.
> 
> Therefore, would it be sufficient to only add a check for whether
> wBytesPerInterval is 0 in the existing flow, and if it is 0, set
> wBytesPerInterval to cpu_to_le16(max_tx) by default?

No, don't override a value given by a device.  Mark the descriptor as
invalid and fail attaching to the device.

> For example, modify it in the following way：
> 
>      if (le16_to_cpu(desc->wBytesPerInterval) > max_tx ||
> le16_to_cpu(desc->wBytesPerInterval) == 0) {
>         dev_notice(ddev, "%s endpoint with wBytesPerInterval of %d in "
>                 "config %d interface %d altsetting %d ep %d: "
>                 "setting to %d\n",
>                 usb_endpoint_xfer_isoc(&ep->desc) ? "Isoc" : "Int",
>                 le16_to_cpu(desc->wBytesPerInterval),
>                 cfgno, inum, asnum, ep->desc.bEndpointAddress,
>                 max_tx);
>         ep->ss_ep_comp.wBytesPerInterval = cpu_to_le16(max_tx);
>     }

There's nothing a user can do with this type of error, and yet the
kernel is supposed to fix it up?  We should just fail it and tell the
user then, like we do for other broken descriptors.

Did you find this issue with a real device, or is this just due to
fuzzing invalid descriptor values?

thanks,

greg k-h