| Message ID | 20231130114006.31760-1-oneukum@suse.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [RFC] USB: check for transmissible packet sizes when matching endpoints | expand |
On Thu, Nov 30, 2023 at 12:39:45PM +0100, Oliver Neukum wrote: > Looking for a bulk endpoint to transfer data over > we need something that can transmit data. > > Signed-off-by: Oliver Neukum <oneukum@suse.com> > --- > drivers/usb/core/usb.c | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/drivers/usb/core/usb.c b/drivers/usb/core/usb.c > index 2a938cf47ccd..d163bd279021 100644 > --- a/drivers/usb/core/usb.c > +++ b/drivers/usb/core/usb.c > @@ -80,6 +80,9 @@ static bool match_endpoint(struct usb_endpoint_descriptor *epd, > { > switch (usb_endpoint_type(epd)) { > case USB_ENDPOINT_XFER_BULK: > + if (!usb_endpoint_maxp(epd)) > + return false; Why would a bulk endpoint descriptor's maxpacket size ever be 0? Are there any devices that have such a thing? If we do encounter one, it will trigger a dev_notice() in config.c's usb_parse_endpoint(). Alan Stern
On 30.11.23 17:52, Alan Stern wrote: > Why would a bulk endpoint descriptor's maxpacket size ever be 0? Are Because evil people connect evil devices to nice computers. > there any devices that have such a thing? > > If we do encounter one, it will trigger a dev_notice() in config.c's > usb_parse_endpoint(). Yes, but that does not change what drivers will do when they try to use the endpoint. Regards Oliver
On Thu, Nov 30, 2023 at 07:22:24PM +0100, Oliver Neukum wrote: > On 30.11.23 17:52, Alan Stern wrote: > > > Why would a bulk endpoint descriptor's maxpacket size ever be 0? Are > > Because evil people connect evil devices to nice computers. > > > there any devices that have such a thing? > > > > If we do encounter one, it will trigger a dev_notice() in config.c's > > usb_parse_endpoint(). > > Yes, but that does not change what drivers will do when they > try to use the endpoint. In theory, it _is_ possible to use an endpoint whose maxpacket value is 0. All you can do with it is transfer zero-length packets -- but somebody might want to do just that. On the other hand, the USB-2 spec (section 5.8.3) does say that the only valid sizes for bulk-endpoint maxpacket values are 8, 16, 32, 64, and 512, depending on the speed (add 1024 for USB-3). We do accept other sizes, but perhaps we should rule out size 0. The right place to do this is in usb_parse_endpoint(). Oddly enough, the spec does allow the maxpacket size for interrupt endpoints to be 0 (section 5.7.3). usb_submit_urb() should check for this case and fail a submission if the transfer_length != 0. Regardless, the endpoint-matching routines are not the right place for these checks. Alan Stern
On Thu, Nov 30, 2023 at 12:39:45PM +0100, Oliver Neukum wrote: > Looking for a bulk endpoint to transfer data over > we need something that can transmit data. > > Signed-off-by: Oliver Neukum <oneukum@suse.com> > --- > drivers/usb/core/usb.c | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/drivers/usb/core/usb.c b/drivers/usb/core/usb.c > index 2a938cf47ccd..d163bd279021 100644 > --- a/drivers/usb/core/usb.c > +++ b/drivers/usb/core/usb.c > @@ -80,6 +80,9 @@ static bool match_endpoint(struct usb_endpoint_descriptor *epd, > { > switch (usb_endpoint_type(epd)) { > case USB_ENDPOINT_XFER_BULK: > + if (!usb_endpoint_maxp(epd)) > + return false; > + > if (usb_endpoint_dir_in(epd)) { > if (bulk_in && !*bulk_in) { > *bulk_in = epd; This reminds me of 2548288b4fb0 ("USB: Fix: Don't skip endpoint descriptors with maxpacket=0") and https://lore.kernel.org/all/20200102112045.GA17614@localhost/ We have at least one ftdi device with broken descriptors that would be hurt by this if usb serial used this helper to look up the endpoint. That isn't the case currently, but in theory there could be other devices like that. Is there a real issue you're trying to address here? Johan
diff --git a/drivers/usb/core/usb.c b/drivers/usb/core/usb.c index 2a938cf47ccd..d163bd279021 100644 --- a/drivers/usb/core/usb.c +++ b/drivers/usb/core/usb.c @@ -80,6 +80,9 @@ static bool match_endpoint(struct usb_endpoint_descriptor *epd, { switch (usb_endpoint_type(epd)) { case USB_ENDPOINT_XFER_BULK: + if (!usb_endpoint_maxp(epd)) + return false; + if (usb_endpoint_dir_in(epd)) { if (bulk_in && !*bulk_in) { *bulk_in = epd;
Looking for a bulk endpoint to transfer data over we need something that can transmit data. Signed-off-by: Oliver Neukum <oneukum@suse.com> --- drivers/usb/core/usb.c | 3 +++ 1 file changed, 3 insertions(+)