On Mon, Mar 06, 2017 at 04:56:31PM +0100, Johan Hovold wrote: > On Mon, Mar 06, 2017 at 04:01:36PM +0100, Johan Hovold wrote: > > On Mon, Mar 06, 2017 at 02:14:51PM +0100, Oliver Neukum wrote: > > > Am Montag, den 06.03.2017, 12:27 +0100 schrieb Johan Hovold: > > > > On Mon, Mar 06, 2017 at 10:54:44AM +0100, Oliver Neukum wrote: > > > > > > True, but again not specific to serial drivers. DVB-T will face > > > > > the same issues. > > > > > > > > Certainly, and I should have elaborated a bit more on the > > > > differences here. For USB-serial endpoint-resources are allocated > > > > only after subdriver probe returns, while for most (all) other USB > > > > driver this would need to be done in the USB probe callback > > > > directly. In that case, checking before probe might be too soon (fw > > > > download, altsetting) and checking after would be too late (endpoint > > > > resources would already need to have been setup). > > > > > > Now that argues for usbserial to trigger the check itself, but not for > > > the code doing the check living in usbserial. In other words put > > > > > > struct usb_serial_endpoints and find_endpoints into usbcore and > > > call it from usbserial. Add a flag to tell usbcore to not check > > > this by itself and you are done. (With some generic name changes) > > > > > > That way everybody can use it almost for free. > > > > An idea would be to always construct the endpoint-data struct (as a > > member of usb_interface) when setting an alternate setting (or always > > keep it around in usb_host_interface), and provide minimal constraints > > per usb_driver which core verified before probe unless a flag is set. > > > > Would the overhead for this be acceptable? I guess some more memory > > would be required for control and isochronous endpoints too. > > A variant would be to always store the endpoint-type counts in struct > usb_host_interface. That's often all that's needed to do the sanity > checks (including the cdc-acm case) and only adds 7 bytes per > altsetting. > > Then a find_endpoints helper can be added to partition the endpoints of > a given altsetting. After surveying most USB drivers, I decided against generalising the usb-serial endpoint helper that this series adds simply because basically no other driver would require it in its present form (and should therefore not need to allocate the memory needed to hold all the descriptors, etc). Instead I just posted a series that adds a few basic helpers to look-up endpoints (typically 1-3 endpoints of distinct types) that should cover the vast majority of drivers. As for having USB core verify endpoint counts before calling driver probe, I'm not sure whether it is worth the effort. Sure adding the endpoint counts to each altsetting and checking against minimal constraints in struct usb_driver when matching is easy enough, but the value of this is limited. Firstly, it would not cover drivers that switch altsettings (although, such drivers could possibly use the counts themselves). Secondly, and perhaps most importantly, it would only amount to a small reduction of time spent when probing *malicious* devices (and the odd interface which cannot be matched on other attributes). Specifically, you'd still require the drivers to look up the endpoints they need (and bail out if not found). In the end I think it's more preferred to have an explicit check where the endpoints are first dereferenced (e.g. probe) than to have a constraint set in some static struct somewhere (which could get out of sync with the code, and would not cover optional endpoints, etc). Now, usb-serial is special in that usb-serial core allocates resources for the subdrivers, which do not deal with the descriptors directly themselves. So here, such static constraints, which can even be specified for several classes per driver, has its value. Johan -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
