| Message ID | dd2c8e8c-2c87-44ea-ba17-c64b97e201c9@rowland.harvard.edu (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 13890626501ffda22b18213ddaf7930473da5792 |
| Headers | show |
| Series | [1/3] USB: core: Add routines for endpoint checks in old drivers | expand |
On 10.04.23 21:37, Alan Stern wrote: Hi, > To make this checking as simple as possible, we now add a couple of > utility routines to the USB core. usb_check_bulk_endpoints() and > usb_check_int_endpoints() take an interface pointer together with a > list of endpoint addresses (numbers and directions). They check that > the interface's current alternate setting includes endpoints with > those addresses and that each of these endpoints has the right type: > bulk or interrupt, respectively. > > Although we already have usb_find_common_endpoints() and related > routines meant for a similar purpose, they are not well suited for > this kind of checking. Those routines find endpoints of various > kinds, but only one (either the first or the last) of each kind, and > they don't verify that the endpoints' addresses agree with what the > caller expects. these will do the job. Yet this strikes me as unelegant. That is if you define a data structure to match against, why not add a pointer to it to struct usb_device_id and use that? Basically the table of endpoints you are creating is a description of a device. Why add code for checking it to each probe() method that needs it? Regards Oliver
On Wed, Apr 12, 2023 at 01:54:12PM +0200, Oliver Neukum wrote: > On 10.04.23 21:37, Alan Stern wrote: > > Hi, > > To make this checking as simple as possible, we now add a couple of > > utility routines to the USB core. usb_check_bulk_endpoints() and > > usb_check_int_endpoints() take an interface pointer together with a > > list of endpoint addresses (numbers and directions). They check that > > the interface's current alternate setting includes endpoints with > > those addresses and that each of these endpoints has the right type: > > bulk or interrupt, respectively. > > > > Although we already have usb_find_common_endpoints() and related > > routines meant for a similar purpose, they are not well suited for > > this kind of checking. Those routines find endpoints of various > > kinds, but only one (either the first or the last) of each kind, and > > they don't verify that the endpoints' addresses agree with what the > > caller expects. > > these will do the job. Yet this strikes me as unelegant. That is > if you define a data structure to match against, why not > add a pointer to it to struct usb_device_id and use that? Struct usb_device_id doesn't seem like the right place. Struct usb_driver would be more appropriate. The drivers that need this have only one entry in their match table, which means that drivers with large match tables (which would require a lot of extra space for the new pointers) don't need it. > Basically the table of endpoints you are creating is a description of > a device. Why add code for checking it to each probe() method > that needs it? True, the checks could be centralized in usb_probe_interface(). What do you think about doing it that way? Alan Stern
On 12.04.23 17:08, Alan Stern wrote: > On Wed, Apr 12, 2023 at 01:54:12PM +0200, Oliver Neukum wrote: >> these will do the job. Yet this strikes me as unelegant. That is >> if you define a data structure to match against, why not >> add a pointer to it to struct usb_device_id and use that? > > Struct usb_device_id doesn't seem like the right place. Struct Conceptually it belongs there. It describes a device, not a driver. I would say that the name is not well chosen, but it is the right place. > usb_driver would be more appropriate. The drivers that need this have > only one entry in their match table, which means that drivers with large Why would that be the case? As far as I can see everything that is not a class driver will need this or an equivalent and many of them support multiple types of devices. > match tables (which would require a lot of extra space for the new > pointers) don't need it. A few dozen bytes. > True, the checks could be centralized in usb_probe_interface(). What do > you think about doing it that way? That really is the best place to put the code for checking. And you might put into a comment that the way USB works the endpoint number includes the direction. It is not obvious to a casual reader. Regards Oliver
On Wed, Apr 12, 2023 at 08:52:45PM +0200, Oliver Neukum wrote: > > > On 12.04.23 17:08, Alan Stern wrote: > > On Wed, Apr 12, 2023 at 01:54:12PM +0200, Oliver Neukum wrote: > > > > these will do the job. Yet this strikes me as unelegant. That is > > > if you define a data structure to match against, why not > > > add a pointer to it to struct usb_device_id and use that? > > > > Struct usb_device_id doesn't seem like the right place. Struct > > Conceptually it belongs there. It describes a device, not a driver. > I would say that the name is not well chosen, but it is the right place. > > > usb_driver would be more appropriate. The drivers that need this have > > only one entry in their match table, which means that drivers with large > > Why would that be the case? As far as I can see everything that is not > a class driver will need this or an equivalent and many of them > support multiple types of devices. In fact, I'm not sure where to find examples of drivers needing this. Apparently only one in drivers/usb/misc/ could benefit (uss720). The other already use usb_find_common_endpoints() or related functions. Some of the drivers in atm/ also could use some work. But there must be plenty of others; I just don't know where to look. The point about how many different devices a driver supports is irrelevant; what matters is how it checks the endpoints it will use. If a driver assumes that all the devices it supports will have three bulk-OUT endpoints numbered 1, 2, and 3 then it doesn't need a separate entry for each usb_device_id in its match table. > > match tables (which would require a lot of extra space for the new > > pointers) don't need it. > > A few dozen bytes. Ho, ho. Look at usb-storage or uas and think again: two useless 8-byte pointers added to each and every entry in the unusual_devs tables. > > True, the checks could be centralized in usb_probe_interface(). What do > > you think about doing it that way? > > That really is the best place to put the code for checking. > And you might put into a comment that the way USB works the endpoint > number includes the direction. It is not obvious to a casual reader. The patches already contain such comments, in the patch description and in the kerneldoc. Alan Stern
Index: usb-devel/drivers/usb/core/usb.c =================================================================== --- usb-devel.orig/drivers/usb/core/usb.c +++ usb-devel/drivers/usb/core/usb.c @@ -207,6 +207,82 @@ int usb_find_common_endpoints_reverse(st EXPORT_SYMBOL_GPL(usb_find_common_endpoints_reverse); /** + * usb_find_endpoint() - Given an endpoint address, search for the endpoint's + * usb_host_endpoint structure in an interface's current altsetting. + * @intf: the interface whose current altsetting should be searched + * @ep_addr: the endpoint address (number and direction) to find + * + * Search the altsetting's list of endpoints for one with the specified address. + * + * Return: Pointer to the usb_host_endpoint if found, %NULL otherwise. + */ +static const struct usb_host_endpoint *usb_find_endpoint( + const struct usb_interface *intf, unsigned int ep_addr) +{ + int n; + const struct usb_host_endpoint *ep; + + n = intf->cur_altsetting->desc.bNumEndpoints; + ep = intf->cur_altsetting->endpoint; + for (; n > 0; (--n, ++ep)) { + if (ep->desc.bEndpointAddress == ep_addr) + return ep; + } + return NULL; +} + +/** + * usb_check_bulk_endpoints - Check whether an interface's current altsetting + * contains a set of bulk endpoints with the given addresses. + * @intf: the interface whose current altsetting should be searched + * @ep_addrs: 0-terminated array of the endpoint addresses (number and + * direction) to look for + * + * Search for endpoints with the specified addresses and check their types. + * + * Return: %true if all the endpoints are found and are bulk, %false otherwise. + */ +bool usb_check_bulk_endpoints( + const struct usb_interface *intf, const u8 *ep_addrs) +{ + const struct usb_host_endpoint *ep; + + for (; *ep_addrs; ++ep_addrs) { + ep = usb_find_endpoint(intf, *ep_addrs); + if (!ep || !usb_endpoint_xfer_bulk(&ep->desc)) + return false; + } + return true; +} +EXPORT_SYMBOL_GPL(usb_check_bulk_endpoints); + +/** + * usb_check_int_endpoints - Check whether an interface's current altsetting + * contains a set of interrupt endpoints with the given addresses. + * @intf: the interface whose current altsetting should be searched + * @ep_addrs: 0-terminated array of the endpoint addresses (number and + * direction) to look for + * + * Search for endpoints with the specified addresses and check their types. + * + * Return: %true if all the endpoints are found and are interrupt, + * %false otherwise. + */ +bool usb_check_int_endpoints( + const struct usb_interface *intf, const u8 *ep_addrs) +{ + const struct usb_host_endpoint *ep; + + for (; *ep_addrs; ++ep_addrs) { + ep = usb_find_endpoint(intf, *ep_addrs); + if (!ep || !usb_endpoint_xfer_int(&ep->desc)) + return false; + } + return true; +} +EXPORT_SYMBOL_GPL(usb_check_int_endpoints); + +/** * usb_find_alt_setting() - Given a configuration, find the alternate setting * for the given interface. * @config: the configuration to search (not necessarily the current config). Index: usb-devel/include/linux/usb.h =================================================================== --- usb-devel.orig/include/linux/usb.h +++ usb-devel/include/linux/usb.h @@ -292,6 +292,11 @@ void usb_put_intf(struct usb_interface * #define USB_MAXINTERFACES 32 #define USB_MAXIADS (USB_MAXINTERFACES/2) +bool usb_check_bulk_endpoints( + const struct usb_interface *intf, const u8 *ep_addrs); +bool usb_check_int_endpoints( + const struct usb_interface *intf, const u8 *ep_addrs); + /* * USB Resume Timer: Every Host controller driver should drive the resume * signalling on the bus for the amount of time defined by this macro.
Many of the older USB drivers in the Linux USB stack were written based simply on a vendor's device specification. They use the endpoint information in the spec and assume these endpoints will always be present, with the properties listed, in any device matching the given vendor and product IDs. While that may have been true back then, with spoofing and fuzzing it is not true any more. More and more we are finding that those old drivers need to perform at least a minimum of checking before they try to use any endpoint other than ep0. To make this checking as simple as possible, we now add a couple of utility routines to the USB core. usb_check_bulk_endpoints() and usb_check_int_endpoints() take an interface pointer together with a list of endpoint addresses (numbers and directions). They check that the interface's current alternate setting includes endpoints with those addresses and that each of these endpoints has the right type: bulk or interrupt, respectively. Although we already have usb_find_common_endpoints() and related routines meant for a similar purpose, they are not well suited for this kind of checking. Those routines find endpoints of various kinds, but only one (either the first or the last) of each kind, and they don't verify that the endpoints' addresses agree with what the caller expects. In theory the new routines could be more general: They could take a particular altsetting as their argument instead of always using the interface's current altsetting. In practice I think this won't matter too much; multiple altsettings tend to be used for transferring media (audio or visual) over isochronous endpoints, not bulk or interrupt. Drivers for such devices will generally require more sophisticated checking than these simplistic routines provide. Signed-off-by: Alan Stern <stern@rowland.harvard.edu> --- This patch and the one for the radio-shark drivers have changed since Hans reviewed them, so I'm not including his Reviewed-by: tag. [as1992] drivers/usb/core/usb.c | 76 +++++++++++++++++++++++++++++++++++++++++++++++++ include/linux/usb.h | 5 +++ 2 files changed, 81 insertions(+)