usb: interface: allow drivers declare number of endpoints they need
PATCH 1/1 usb: interface: allow drivers declare number of endpoints they need
USB interface drivers need to check number of endpoints before trying to access/use them. Quite a few drivers only use the default setting (altsetting 0), so let's allow them to declare number of endpoints in altsetting 0 they require to operate and have USB core check it for us instead of having every driver implement check manually.
For compatibility, if driver does not specify number of endpoints (i.e. number of endpoints is left at 0) we bypass the check in USB core and expect the driver perform necessary checks on its own.
---
Greg, if the patch is reasonable I wonder if I can take it through my tree, as I have a few drivers that do not check number of endpoints properly and will crash the kernel when specially crafted device is plugged in, as reported by Vladis Dronov.
drivers/usb/core/driver.c | 9 +++++++++ include/linux/usb.h | 7 +++++++ 2 files changed, 16 insertions(+)
drivers/usb/core/driver.c +9 -0
@@ -306,6 +306,15 @@ static int usb_probe_interface(struct device *dev)dev_dbg (dev,"%s - got id\n" , __func__); +if (driver->num_endpoints && + intf->altsetting[0 ].desc.bNumEndpoints < driver->num_endpoints) { + +dev_err (dev,"Not enough endpoints %d (want %d)\n" , + intf->altsetting[0 ].desc.bNumEndpoints, + driver->num_endpoints); +return -EINVAL ; + } + error =usb_autoresume_device (udev);if (error)return error;
include/linux/usb.h +7 -0
@@ -1051,6 +1051,11 @@ struct usbdrv_wrap {* @id_table: USB drivers use ID table to support hotplugging. * Export this with MODULE_DEVICE_TABLE(usb,...). This must be set * or your driver's probe function will never get called. +* @num_endpoints: Number of endpoints that should be present in default +* setting (altsetting 0) the driver needs to operate properly. +* The probe will be aborted if actual number of endpoints is less +* than what the driver specified here. 0 means no check should be +* performed. * @dynids: used internally to hold the list of dynamically added device * ids for this driver. * @drvwrap: Driver-model core structure wrapper.
@@ -1099,6 +1104,8 @@ struct usb_driver {const struct usb_device_id *id_table; +unsigned int num_endpoints; +struct usb_dynids dynids;struct usbdrv_wrap drvwrap; unsigned int no_dynamic_id:1;
Signature · 5 lines
2.6.0.rc2.230.g3dd15c0 -- Dmitry
Hi,
Quoted 28 lines · Dmitry Torokhov
USB interface drivers need to check number of endpoints before trying to access/use them. Quite a few drivers only use the default setting (altsetting 0), so let's allow them to declare number of endpoints in altsetting 0 they require to operate and have USB core check it for us instead of having every driver implement check manually.
For compatibility, if driver does not specify number of endpoints (i.e. number of endpoints is left at 0) we bypass the check in USB core and expect the driver perform necessary checks on its own.
Acked-by Alan Stern <[email protected]>Signed-off-by Dmitry Torokhov <[email protected]>---
Greg, if the patch is reasonable I wonder if I can take it through my tree, as I have a few drivers that do not check number of endpoints properly and will crash the kernel when specially crafted device is plugged in, as reported by Vladis Dronov.
drivers/usb/core/driver.c | 9 +++++++++ include/linux/usb.h | 7 +++++++ 2 files changed, 16 insertions(+)drivers/usb/core/driver.c +0
-0
drivers/usb/core/driver.c static int usb_probe_interface(struct device *dev) · line 309
Quoted 4 lines
drivers/usb/core/driver.c +1
-0@@ -306,6 +306,15 @@ static int usb_probe_interface(struct device *dev)dev_dbg (dev,"%s - got id\n" , __func__); + if (driver->num_endpoints &&
this part of the check is pointless, right ?
drivers/usb/core/driver.c static int usb_probe_interface(struct device *dev) · line 310
Quoted 1 line
drivers/usb/core/driver.c +1
-0+ intf->altsetting[0].desc.bNumEndpoints < driver->num_endpoints) {
bNumEndpoints will never be less than 0 and if it is, we're gonna have issues elsewhere anyway.
Signature · 1 line
balbi
Greg Kroah-Hartman
maintainerQuoted 28 lines · Dmitry Torokhov
USB interface drivers need to check number of endpoints before trying to access/use them. Quite a few drivers only use the default setting (altsetting 0), so let's allow them to declare number of endpoints in altsetting 0 they require to operate and have USB core check it for us instead of having every driver implement check manually.
For compatibility, if driver does not specify number of endpoints (i.e. number of endpoints is left at 0) we bypass the check in USB core and expect the driver perform necessary checks on its own.
Acked-by Alan Stern <[email protected]>Signed-off-by Dmitry Torokhov <[email protected]>---
Greg, if the patch is reasonable I wonder if I can take it through my tree, as I have a few drivers that do not check number of endpoints properly and will crash the kernel when specially crafted device is plugged in, as reported by Vladis Dronov.
drivers/usb/core/driver.c | 9 +++++++++ include/linux/usb.h | 7 +++++++ 2 files changed, 16 insertions(+)drivers/usb/core/driver.c +0
-0
drivers/usb/core/driver.c static int usb_probe_interface(struct device *dev) · line 311
Quoted 6 lines
drivers/usb/core/driver.c +3
-0@@ -306,6 +306,15 @@ static int usb_probe_interface(struct device *dev)dev_dbg (dev,"%s - got id\n" , __func__); +if (driver->num_endpoints && + intf->altsetting[0 ].desc.bNumEndpoints < driver->num_endpoints) { +
Empty line :(
drivers/usb/core/driver.c static int usb_probe_interface(struct device *dev) · line 314
Quoted 3 lines
drivers/usb/core/driver.c +3
-0+dev_err (dev,"Not enough endpoints %d (want %d)\n" , + intf->altsetting[0 ].desc.bNumEndpoints, + driver->num_endpoints);
What can a user do with this?
Quoted 11 lines
drivers/usb/core/driver.c +3
-0+return -EINVAL ; + } + error =usb_autoresume_device (udev);if (error)return error;include/linux/usb.h +0
-0
include/linux/usb.h struct usbdrv_wrap { · line 1058
Quoted 8 lines
include/linux/usb.h +5
-0@@ -1051,6 +1051,11 @@ struct usbdrv_wrap {* @id_table: USB drivers use ID table to support hotplugging. * Export this with MODULE_DEVICE_TABLE(usb,...). This must be set * or your driver's probe function will never get called. +* @num_endpoints: Number of endpoints that should be present in default +* setting (altsetting 0) the driver needs to operate properly. +* The probe will be aborted if actual number of endpoints is less +* than what the driver specified here. 0 means no check should be +* performed.
I don't understand, a driver can do whatever it wants with the endpoints of the interface, why do we need to check/know this ahead of time? What is crashing without this?
It's up to the driver to check this, if it cares about it. How many drivers do you have that is going to care? Why is this suddenly a new thing that we haven't run into in the past 15+ years?
thanks,
greg k-h
Quoted 35 lines · Felipe Balbi
Hi,
Quoted 31 lines · Dmitry Torokhov
USB interface drivers need to check number of endpoints before trying to access/use them. Quite a few drivers only use the default setting (altsetting 0), so let's allow them to declare number of endpoints in altsetting 0 they require to operate and have USB core check it for us instead of having every driver implement check manually.
For compatibility, if driver does not specify number of endpoints (i.e. number of endpoints is left at 0) we bypass the check in USB core and expect the driver perform necessary checks on its own.
Acked-by Alan Stern <[email protected]>Signed-off-by Dmitry Torokhov <[email protected]>---
Greg, if the patch is reasonable I wonder if I can take it through my tree, as I have a few drivers that do not check number of endpoints properly and will crash the kernel when specially crafted device is plugged in, as reported by Vladis Dronov.
drivers/usb/core/driver.c | 9 +++++++++ include/linux/usb.h | 7 +++++++ 2 files changed, 16 insertions(+)drivers/usb/core/driver.c +0
-0@@ -306,6 +306,15 @@ static int usb_probe_interface(struct device *dev)dev_dbg (dev,"%s - got id\n" , __func__);
Quoted 8 lines
drivers/usb/core/driver.c static int usb_probe_interface(struct device *dev) · line 309
Quoted 1 line
drivers/usb/core/driver.c +1
-0+ if (driver->num_endpoints &&this part of the check is pointless, right ?
drivers/usb/core/driver.c static int usb_probe_interface(struct device *dev) · line 310
Quoted 1 line
drivers/usb/core/driver.c +1
-0+ intf->altsetting[0].desc.bNumEndpoints < driver->num_endpoints) {bNumEndpoints will never be less than 0 and if it is, we're gonna have issues elsewhere anyway.
Fair enough, I'll drop it.
Thanks.
Signature · 1 line
Dmitry
Quoted 38 lines · Greg Kroah-Hartman
Quoted 28 lines · Dmitry Torokhov
USB interface drivers need to check number of endpoints before trying to access/use them. Quite a few drivers only use the default setting (altsetting 0), so let's allow them to declare number of endpoints in altsetting 0 they require to operate and have USB core check it for us instead of having every driver implement check manually.
For compatibility, if driver does not specify number of endpoints (i.e. number of endpoints is left at 0) we bypass the check in USB core and expect the driver perform necessary checks on its own.
Acked-by Alan Stern <[email protected]>Signed-off-by Dmitry Torokhov <[email protected]>---
Greg, if the patch is reasonable I wonder if I can take it through my tree, as I have a few drivers that do not check number of endpoints properly and will crash the kernel when specially crafted device is plugged in, as reported by Vladis Dronov.
drivers/usb/core/driver.c | 9 +++++++++ include/linux/usb.h | 7 +++++++ 2 files changed, 16 insertions(+)drivers/usb/core/driver.c +0
-0drivers/usb/core/driver.c static int usb_probe_interface(struct device *dev) · line 311
Quoted 6 lines
drivers/usb/core/driver.c +3
-0@@ -306,6 +306,15 @@ static int usb_probe_interface(struct device *dev)dev_dbg (dev,"%s - got id\n" , __func__); +if (driver->num_endpoints && + intf->altsetting[0 ].desc.bNumEndpoints < driver->num_endpoints) { +Empty line :(
Quoted 5 lines
drivers/usb/core/driver.c static int usb_probe_interface(struct device *dev) · line 314
Quoted 3 lines
drivers/usb/core/driver.c +3
-0+dev_err (dev,"Not enough endpoints %d (want %d)\n" , + intf->altsetting[0 ].desc.bNumEndpoints, + driver->num_endpoints);What can a user do with this?
Report on the lists or throw such device into a bin.
Quoted 16 lines
Quoted 16 lines
drivers/usb/core/driver.c +3
-0+return -EINVAL ; + } + error =usb_autoresume_device (udev);if (error)return error;include/linux/usb.h +1
-0@@ -1051,6 +1051,11 @@ struct usbdrv_wrap {* @id_table: USB drivers use ID table to support hotplugging. * Export this with MODULE_DEVICE_TABLE(usb,...). This must be set * or your driver's probe function will never get called. +* @num_endpoints: Number of endpoints that should be present in default
Quoted 8 lines
include/linux/usb.h struct usbdrv_wrap { · line 1058
Quoted 4 lines
include/linux/usb.h +4
-0+* setting (altsetting 0) the driver needs to operate properly. +* The probe will be aborted if actual number of endpoints is less +* than what the driver specified here. 0 means no check should be +* performed. I don't understand, a driver can do whatever it wants with the endpoints of the interface, why do we need to check/know this ahead of time? What is crashing without this?
The kernel because some drivers do not verify that intf->altsetting[0].desc.bNumEndpoints >= 1 before referencing intf->altsetting[0].endpoints[0].
Quoted 2 lines
It's up to the driver to check this, if it cares about it.
Instead of duplicating the check in almost every driver is it more efficient to allow USB core check it for them (if driver requests it to do so).
Quoted 2 lines
How many
drivers do you have that is going to care?
I saw at least 3 that did not check, that's from cursory glance. Plus we have many that do check explicitly.
Quoted 2 lines
Why is this suddenly a new
thing that we haven't run into in the past 15+ years?
We are less trusting now. Before we/some of the drivers believed that if device has VID/PID that they recognize the rest of descriptors will have the data we expect, but we can't rely on this anymore.
Thanks.
Signature · 1 line
Dmitry
Greg Kroah-Hartman
maintainerQuoted 73 lines · Dmitry Torokhov
Quoted 38 lines · Greg Kroah-Hartman
Quoted 28 lines · Dmitry Torokhov
USB interface drivers need to check number of endpoints before trying to access/use them. Quite a few drivers only use the default setting (altsetting 0), so let's allow them to declare number of endpoints in altsetting 0 they require to operate and have USB core check it for us instead of having every driver implement check manually.
For compatibility, if driver does not specify number of endpoints (i.e. number of endpoints is left at 0) we bypass the check in USB core and expect the driver perform necessary checks on its own.
Acked-by Alan Stern <[email protected]>Signed-off-by Dmitry Torokhov <[email protected]>---
Greg, if the patch is reasonable I wonder if I can take it through my tree, as I have a few drivers that do not check number of endpoints properly and will crash the kernel when specially crafted device is plugged in, as reported by Vladis Dronov.
drivers/usb/core/driver.c | 9 +++++++++ include/linux/usb.h | 7 +++++++ 2 files changed, 16 insertions(+)drivers/usb/core/driver.c +0
-0drivers/usb/core/driver.c static int usb_probe_interface(struct device *dev) · line 311
Quoted 6 lines
drivers/usb/core/driver.c +3
-0@@ -306,6 +306,15 @@ static int usb_probe_interface(struct device *dev)dev_dbg (dev,"%s - got id\n" , __func__); +if (driver->num_endpoints && + intf->altsetting[0 ].desc.bNumEndpoints < driver->num_endpoints) { +Empty line :(
Quoted 5 lines
drivers/usb/core/driver.c static int usb_probe_interface(struct device *dev) · line 314
Quoted 3 lines
drivers/usb/core/driver.c +3
-0+dev_err (dev,"Not enough endpoints %d (want %d)\n" , + intf->altsetting[0 ].desc.bNumEndpoints, + driver->num_endpoints);What can a user do with this?
Report on the lists or throw such device into a bin.
Quoted 16 lines
Quoted 16 lines
drivers/usb/core/driver.c +3
-0+return -EINVAL ; + } + error =usb_autoresume_device (udev);if (error)return error;include/linux/usb.h +1
-0@@ -1051,6 +1051,11 @@ struct usbdrv_wrap {* @id_table: USB drivers use ID table to support hotplugging. * Export this with MODULE_DEVICE_TABLE(usb,...). This must be set * or your driver's probe function will never get called. +* @num_endpoints: Number of endpoints that should be present in default Quoted 8 lines
include/linux/usb.h struct usbdrv_wrap { · line 1058
Quoted 4 lines
include/linux/usb.h +4
-0+* setting (altsetting 0) the driver needs to operate properly. +* The probe will be aborted if actual number of endpoints is less +* than what the driver specified here. 0 means no check should be +* performed. I don't understand, a driver can do whatever it wants with the endpoints of the interface, why do we need to check/know this ahead of time? What is crashing without this?
Quoted 3 lines
The kernel because some drivers do not verify that intf->altsetting[0].desc.bNumEndpoints >= 1 before referencing intf->altsetting[0].endpoints[0].
The USB core does that? Or just a driver, and if it's just a driver, we should fix that in the driver itself as there are lots of other validation checks the drivers should be doing becides just this one about endpoints, sizes, and directions that we can't catch in the core.
Quoted 5 lines
Quoted 1 line
It's up to the driver to check this, if it cares about it.
Instead of duplicating the check in almost every driver is it more efficient to allow USB core check it for them (if driver requests it to do so).
ok, fair enough, but it's just one of many things they should be checking, this doesn't provide all that much "protection".
Quoted 6 lines
Quoted 2 lines
How many
drivers do you have that is going to care?I saw at least 3 that did not check, that's from cursory glance. Plus we have many that do check explicitly.
Quoted 6 lines
Quoted 2 lines
Why is this suddenly a new
thing that we haven't run into in the past 15+ years?We are less trusting now. Before we/some of the drivers believed that if device has VID/PID that they recognize the rest of descriptors will have the data we expect, but we can't rely on this anymore.
There's lots of things we can't "rely on", and we have never been able to rely on, but this is going to require we touch every USB driver to make those changes, this one change isn't going to really do all that much to help out with that.
Every USB driver _should_ be having a loop over all endpoints to find what they need/expect, and if it isn't there, then it needs to abort. Just checking the number of endpoints isn't ok, so I really think this isn't going to help all that much in the end...
thanks,
greg k-h
Quoted 110 lines · Greg Kroah-Hartman
Quoted 73 lines · Dmitry Torokhov
Quoted 38 lines · Greg Kroah-Hartman
Quoted 28 lines · Dmitry Torokhov
USB interface drivers need to check number of endpoints before trying to access/use them. Quite a few drivers only use the default setting (altsetting 0), so let's allow them to declare number of endpoints in altsetting 0 they require to operate and have USB core check it for us instead of having every driver implement check manually.
For compatibility, if driver does not specify number of endpoints (i.e. number of endpoints is left at 0) we bypass the check in USB core and expect the driver perform necessary checks on its own.
Acked-by Alan Stern <[email protected]>Signed-off-by Dmitry Torokhov <[email protected]>---
Greg, if the patch is reasonable I wonder if I can take it through my tree, as I have a few drivers that do not check number of endpoints properly and will crash the kernel when specially crafted device is plugged in, as reported by Vladis Dronov.
drivers/usb/core/driver.c | 9 +++++++++ include/linux/usb.h | 7 +++++++ 2 files changed, 16 insertions(+)drivers/usb/core/driver.c +0
-0drivers/usb/core/driver.c static int usb_probe_interface(struct device *dev) · line 311
Quoted 6 lines
drivers/usb/core/driver.c +3
-0@@ -306,6 +306,15 @@ static int usb_probe_interface(struct device *dev)dev_dbg (dev,"%s - got id\n" , __func__); +if (driver->num_endpoints && + intf->altsetting[0 ].desc.bNumEndpoints < driver->num_endpoints) { +Empty line :(
Quoted 5 lines
drivers/usb/core/driver.c static int usb_probe_interface(struct device *dev) · line 314
Quoted 3 lines
drivers/usb/core/driver.c +3
-0+dev_err (dev,"Not enough endpoints %d (want %d)\n" , + intf->altsetting[0 ].desc.bNumEndpoints, + driver->num_endpoints);What can a user do with this?
Report on the lists or throw such device into a bin.
Quoted 16 lines
Quoted 16 lines
drivers/usb/core/driver.c +3
-0+return -EINVAL ; + } + error =usb_autoresume_device (udev);if (error)return error;include/linux/usb.h +1
-0@@ -1051,6 +1051,11 @@ struct usbdrv_wrap {* @id_table: USB drivers use ID table to support hotplugging. * Export this with MODULE_DEVICE_TABLE(usb,...). This must be set * or your driver's probe function will never get called. +* @num_endpoints: Number of endpoints that should be present in default Quoted 8 lines
include/linux/usb.h struct usbdrv_wrap { · line 1058
Quoted 4 lines
include/linux/usb.h +4
-0+* setting (altsetting 0) the driver needs to operate properly. +* The probe will be aborted if actual number of endpoints is less +* than what the driver specified here. 0 means no check should be +* performed. I don't understand, a driver can do whatever it wants with the endpoints of the interface, why do we need to check/know this ahead of time? What is crashing without this?
Quoted 3 lines
The kernel because some drivers do not verify that intf->altsetting[0].desc.bNumEndpoints >= 1 before referencing intf->altsetting[0].endpoints[0].
The USB core does that? Or just a driver, and if it's just a driver, we should fix that in the driver itself as there are lots of other validation checks the drivers should be doing becides just this one about endpoints, sizes, and directions that we can't catch in the core.
Quoted 5 lines
Quoted 1 line
It's up to the driver to check this, if it cares about it.
Instead of duplicating the check in almost every driver is it more efficient to allow USB core check it for them (if driver requests it to do so).
ok, fair enough, but it's just one of many things they should be checking, this doesn't provide all that much "protection".
Quoted 6 lines
Quoted 2 lines
How many
drivers do you have that is going to care?I saw at least 3 that did not check, that's from cursory glance. Plus we have many that do check explicitly.
Quoted 6 lines
Quoted 2 lines
Why is this suddenly a new
thing that we haven't run into in the past 15+ years?We are less trusting now. Before we/some of the drivers believed that if device has VID/PID that they recognize the rest of descriptors will have the data we expect, but we can't rely on this anymore.
There's lots of things we can't "rely on", and we have never been able to rely on, but this is going to require we touch every USB driver to make those changes, this one change isn't going to really do all that much to help out with that.
Quoted 4 lines
Every USB driver _should_ be having a loop over all endpoints to find what they need/expect, and if it isn't there, then it needs to abort. Just checking the number of endpoints isn't ok, so I really think this isn't going to help all that much in the end...
OK, fair enough. Maybe what is missing is something like:
ep = usb_locate_endpoint(altsetting, type, direction);
if (!ep) {
...
return -EINVAL;
}that would loop through endpoints so that drivers do not have to open-code the loop and we indeed need to fix the drivers that blindly grab endpoints at fixed offsets and expect them to be there and have correct types.
Let's consider this patch dropped.
Thanks.
Signature · 1 line
Dmitry
Greg Kroah-Hartman
maintainerQuoted 125 lines · Dmitry Torokhov
Quoted 110 lines · Greg Kroah-Hartman
Quoted 73 lines · Dmitry Torokhov
Quoted 38 lines · Greg Kroah-Hartman
Quoted 28 lines · Dmitry Torokhov
USB interface drivers need to check number of endpoints before trying to access/use them. Quite a few drivers only use the default setting (altsetting 0), so let's allow them to declare number of endpoints in altsetting 0 they require to operate and have USB core check it for us instead of having every driver implement check manually.
For compatibility, if driver does not specify number of endpoints (i.e. number of endpoints is left at 0) we bypass the check in USB core and expect the driver perform necessary checks on its own.
Acked-by Alan Stern <[email protected]>Signed-off-by Dmitry Torokhov <[email protected]>---
Greg, if the patch is reasonable I wonder if I can take it through my tree, as I have a few drivers that do not check number of endpoints properly and will crash the kernel when specially crafted device is plugged in, as reported by Vladis Dronov.
drivers/usb/core/driver.c | 9 +++++++++ include/linux/usb.h | 7 +++++++ 2 files changed, 16 insertions(+)drivers/usb/core/driver.c +0
-0drivers/usb/core/driver.c static int usb_probe_interface(struct device *dev) · line 311
Quoted 6 lines
drivers/usb/core/driver.c +3
-0@@ -306,6 +306,15 @@ static int usb_probe_interface(struct device *dev)dev_dbg (dev,"%s - got id\n" , __func__); +if (driver->num_endpoints && + intf->altsetting[0 ].desc.bNumEndpoints < driver->num_endpoints) { +Empty line :(
Quoted 5 lines
drivers/usb/core/driver.c static int usb_probe_interface(struct device *dev) · line 314
Quoted 3 lines
drivers/usb/core/driver.c +3
-0+dev_err (dev,"Not enough endpoints %d (want %d)\n" , + intf->altsetting[0 ].desc.bNumEndpoints, + driver->num_endpoints);What can a user do with this?
Report on the lists or throw such device into a bin.
Quoted 16 lines
Quoted 16 lines
drivers/usb/core/driver.c +3
-0+return -EINVAL ; + } + error =usb_autoresume_device (udev);if (error)return error;include/linux/usb.h +1
-0@@ -1051,6 +1051,11 @@ struct usbdrv_wrap {* @id_table: USB drivers use ID table to support hotplugging. * Export this with MODULE_DEVICE_TABLE(usb,...). This must be set * or your driver's probe function will never get called. +* @num_endpoints: Number of endpoints that should be present in default Quoted 8 lines
include/linux/usb.h struct usbdrv_wrap { · line 1058
Quoted 4 lines
include/linux/usb.h +4
-0+* setting (altsetting 0) the driver needs to operate properly. +* The probe will be aborted if actual number of endpoints is less +* than what the driver specified here. 0 means no check should be +* performed. I don't understand, a driver can do whatever it wants with the endpoints of the interface, why do we need to check/know this ahead of time? What is crashing without this?
Quoted 3 lines
The kernel because some drivers do not verify that intf->altsetting[0].desc.bNumEndpoints >= 1 before referencing intf->altsetting[0].endpoints[0].
The USB core does that? Or just a driver, and if it's just a driver, we should fix that in the driver itself as there are lots of other validation checks the drivers should be doing becides just this one about endpoints, sizes, and directions that we can't catch in the core.
Quoted 5 lines
Quoted 1 line
It's up to the driver to check this, if it cares about it.
Instead of duplicating the check in almost every driver is it more efficient to allow USB core check it for them (if driver requests it to do so).
ok, fair enough, but it's just one of many things they should be checking, this doesn't provide all that much "protection".
Quoted 6 lines
Quoted 2 lines
How many
drivers do you have that is going to care?I saw at least 3 that did not check, that's from cursory glance. Plus we have many that do check explicitly.
Quoted 6 lines
Quoted 2 lines
Why is this suddenly a new
thing that we haven't run into in the past 15+ years?We are less trusting now. Before we/some of the drivers believed that if device has VID/PID that they recognize the rest of descriptors will have the data we expect, but we can't rely on this anymore.
There's lots of things we can't "rely on", and we have never been able to rely on, but this is going to require we touch every USB driver to make those changes, this one change isn't going to really do all that much to help out with that.
Quoted 4 lines
Every USB driver _should_ be having a loop over all endpoints to find what they need/expect, and if it isn't there, then it needs to abort. Just checking the number of endpoints isn't ok, so I really think this isn't going to help all that much in the end...
OK, fair enough. Maybe what is missing is something like:
ep =
usb_locate_endpoint(altsetting, type, direction);if (!ep) { ... return -EINVAL; }
Quoted 4 lines
that would loop through endpoints so that drivers do not have to open-code the loop and we indeed need to fix the drivers that blindly grab endpoints at fixed offsets and expect them to be there and have correct types.
Yes, that would work for one single type of endpoint, but lots of drivers need/have 2 of the same type/direction, so what would this function do then? Error out? Hm, that might work, and it would reduce a bunch of common code, care to make up a patch for that?
thanks,
greg k-h
Quoted 11 lines · Greg Kroah-Hartman
Quoted 2 lines
that would loop through endpoints so that drivers do not have to open-code the loop and we indeed need to fix the drivers that
blindly
Quoted 2 lines
grab endpoints at fixed offsets and expect them to be there and have correct types.
Yes, that would work for one single type of endpoint, but lots of drivers need/have 2 of the same type/direction, so what would this function do then? Error out? Hm, that might work, and it would reduce
a bunch of common code, care to make up a patch for that?
Hi,
in that case let us go the whole way. Give drivers a way to describe what they need that covers all possibilities up to exactly telling the core what it expects and in which order and numbers. Actually that would be better in the interface matching code path.
Regards
Oliver
Greg Kroah-Hartman
maintainerQuoted 13 lines · Oliver Neukum
Quoted 11 lines · Greg Kroah-Hartman
Quoted 2 lines
that would loop through endpoints so that drivers do not have to open-code the loop and we indeed need to fix the drivers that
blindly
Quoted 2 lines
grab endpoints at fixed offsets and expect them to be there and have correct types.
Yes, that would work for one single type of endpoint, but lots of drivers need/have 2 of the same type/direction, so what would this function do then? Error out? Hm, that might work, and it would reduce
a bunch of common code, care to make up a patch for that?
Quoted 6 lines
Hi,
in that case let us go the whole way. Give drivers a way to describe what they need that covers all possibilities up to exactly telling the core what it expects and in which order and numbers. Actually that would be better in the interface matching code path.
Trying to describe a static, or variable, number of endpoints, for each interface and endpoint type, and then add that information to the usb_device_id structure and common macros, and then touch all users of those macros, might be a bit too much work to do here... :)
thanks,
greg k-h
Quoted 125 lines · Dmitry Torokhov
Quoted 110 lines · Greg Kroah-Hartman
Quoted 73 lines · Dmitry Torokhov
Quoted 38 lines · Greg Kroah-Hartman
Quoted 28 lines · Dmitry Torokhov
USB interface drivers need to check number of endpoints before trying to access/use them. Quite a few drivers only use the default setting (altsetting 0), so let's allow them to declare number of endpoints in altsetting 0 they require to operate and have USB core check it for us instead of having every driver implement check manually.
For compatibility, if driver does not specify number of endpoints (i.e. number of endpoints is left at 0) we bypass the check in USB core and expect the driver perform necessary checks on its own.
Acked-by Alan Stern <[email protected]>Signed-off-by Dmitry Torokhov <[email protected]>---
Greg, if the patch is reasonable I wonder if I can take it through my tree, as I have a few drivers that do not check number of endpoints properly and will crash the kernel when specially crafted device is plugged in, as reported by Vladis Dronov.
drivers/usb/core/driver.c | 9 +++++++++ include/linux/usb.h | 7 +++++++ 2 files changed, 16 insertions(+)drivers/usb/core/driver.c +0
-0drivers/usb/core/driver.c static int usb_probe_interface(struct device *dev) · line 311
Quoted 6 lines
drivers/usb/core/driver.c +3
-0@@ -306,6 +306,15 @@ static int usb_probe_interface(struct device *dev)dev_dbg (dev,"%s - got id\n" , __func__); +if (driver->num_endpoints && + intf->altsetting[0 ].desc.bNumEndpoints < driver->num_endpoints) { +Empty line :(
Quoted 5 lines
drivers/usb/core/driver.c static int usb_probe_interface(struct device *dev) · line 314
Quoted 3 lines
drivers/usb/core/driver.c +3
-0+dev_err (dev,"Not enough endpoints %d (want %d)\n" , + intf->altsetting[0 ].desc.bNumEndpoints, + driver->num_endpoints);What can a user do with this?
Report on the lists or throw such device into a bin.
Quoted 16 lines
Quoted 16 lines
drivers/usb/core/driver.c +3
-0+return -EINVAL ; + } + error =usb_autoresume_device (udev);if (error)return error;include/linux/usb.h +1
-0@@ -1051,6 +1051,11 @@ struct usbdrv_wrap {* @id_table: USB drivers use ID table to support hotplugging. * Export this with MODULE_DEVICE_TABLE(usb,...). This must be set * or your driver's probe function will never get called. +* @num_endpoints: Number of endpoints that should be present in default Quoted 8 lines
include/linux/usb.h struct usbdrv_wrap { · line 1058
Quoted 4 lines
include/linux/usb.h +4
-0+* setting (altsetting 0) the driver needs to operate properly. +* The probe will be aborted if actual number of endpoints is less +* than what the driver specified here. 0 means no check should be +* performed. I don't understand, a driver can do whatever it wants with the endpoints of the interface, why do we need to check/know this ahead of time? What is crashing without this?
Quoted 3 lines
The kernel because some drivers do not verify that intf->altsetting[0].desc.bNumEndpoints >= 1 before referencing intf->altsetting[0].endpoints[0].
The USB core does that? Or just a driver, and if it's just a driver, we should fix that in the driver itself as there are lots of other validation checks the drivers should be doing becides just this one about endpoints, sizes, and directions that we can't catch in the core.
Quoted 5 lines
Quoted 1 line
It's up to the driver to check this, if it cares about it.
Instead of duplicating the check in almost every driver is it more efficient to allow USB core check it for them (if driver requests it to do so).
ok, fair enough, but it's just one of many things they should be checking, this doesn't provide all that much "protection".
Quoted 6 lines
Quoted 2 lines
How many
drivers do you have that is going to care?I saw at least 3 that did not check, that's from cursory glance. Plus we have many that do check explicitly.
Quoted 6 lines
Quoted 2 lines
Why is this suddenly a new
thing that we haven't run into in the past 15+ years?We are less trusting now. Before we/some of the drivers believed that if device has VID/PID that they recognize the rest of descriptors will have the data we expect, but we can't rely on this anymore.
There's lots of things we can't "rely on", and we have never been able to rely on, but this is going to require we touch every USB driver to make those changes, this one change isn't going to really do all that much to help out with that.
Quoted 4 lines
Every USB driver _should_ be having a loop over all endpoints to find what they need/expect, and if it isn't there, then it needs to abort. Just checking the number of endpoints isn't ok, so I really think this isn't going to help all that much in the end...
OK, fair enough. Maybe what is missing is something like:
ep =
usb_locate_endpoint(altsetting, type, direction);if (!ep) { ... return -EINVAL; }
Quoted 6 lines
that would loop through endpoints so that drivers do not have to open-code the loop and we indeed need to fix the drivers that blindly grab endpoints at fixed offsets and expect them to be there and have correct types.
Let's consider this patch dropped.
Since you're dropping this patch, are you going to take the patch Vladis originally sent for the aiptek driver? I'm not objecting to fixing this in a broader sense, but it might be good to get existing fixes in before the whole rework is done.
josh
Quoted 132 lines · Josh Boyer
Quoted 130 lines · Dmitry Torokhov
Quoted 110 lines · Greg Kroah-Hartman
Quoted 73 lines · Dmitry Torokhov
Quoted 38 lines · Greg Kroah-Hartman
Quoted 28 lines · Dmitry Torokhov
USB interface drivers need to check number of endpoints before trying to access/use them. Quite a few drivers only use the default setting (altsetting 0), so let's allow them to declare number of endpoints in altsetting 0 they require to operate and have USB core check it for us instead of having every driver implement check manually.
For compatibility, if driver does not specify number of endpoints (i.e. number of endpoints is left at 0) we bypass the check in USB core and expect the driver perform necessary checks on its own.
Acked-by Alan Stern <[email protected]>Signed-off-by Dmitry Torokhov <[email protected]>---
Greg, if the patch is reasonable I wonder if I can take it through my tree, as I have a few drivers that do not check number of endpoints properly and will crash the kernel when specially crafted device is plugged in, as reported by Vladis Dronov.
drivers/usb/core/driver.c | 9 +++++++++ include/linux/usb.h | 7 +++++++ 2 files changed, 16 insertions(+)drivers/usb/core/driver.c +0
-0drivers/usb/core/driver.c static int usb_probe_interface(struct device *dev) · line 311
Quoted 6 lines
drivers/usb/core/driver.c +3
-0@@ -306,6 +306,15 @@ static int usb_probe_interface(struct device *dev)dev_dbg (dev,"%s - got id\n" , __func__); +if (driver->num_endpoints && + intf->altsetting[0 ].desc.bNumEndpoints < driver->num_endpoints) { +Empty line :(
Quoted 5 lines
drivers/usb/core/driver.c static int usb_probe_interface(struct device *dev) · line 314
Quoted 3 lines
drivers/usb/core/driver.c +3
-0+dev_err (dev,"Not enough endpoints %d (want %d)\n" , + intf->altsetting[0 ].desc.bNumEndpoints, + driver->num_endpoints);What can a user do with this?
Report on the lists or throw such device into a bin.
Quoted 16 lines
Quoted 16 lines
drivers/usb/core/driver.c +3
-0+return -EINVAL ; + } + error =usb_autoresume_device (udev);if (error)return error;include/linux/usb.h +1
-0@@ -1051,6 +1051,11 @@ struct usbdrv_wrap {* @id_table: USB drivers use ID table to support hotplugging. * Export this with MODULE_DEVICE_TABLE(usb,...). This must be set * or your driver's probe function will never get called. +* @num_endpoints: Number of endpoints that should be present in default Quoted 8 lines
include/linux/usb.h struct usbdrv_wrap { · line 1058
Quoted 4 lines
include/linux/usb.h +4
-0+* setting (altsetting 0) the driver needs to operate properly. +* The probe will be aborted if actual number of endpoints is less +* than what the driver specified here. 0 means no check should be +* performed. I don't understand, a driver can do whatever it wants with the endpoints of the interface, why do we need to check/know this ahead of time? What is crashing without this?
Quoted 3 lines
The kernel because some drivers do not verify that intf->altsetting[0].desc.bNumEndpoints >= 1 before referencing intf->altsetting[0].endpoints[0].
The USB core does that? Or just a driver, and if it's just a driver, we should fix that in the driver itself as there are lots of other validation checks the drivers should be doing becides just this one about endpoints, sizes, and directions that we can't catch in the core.
Quoted 5 lines
Quoted 1 line
It's up to the driver to check this, if it cares about it.
Instead of duplicating the check in almost every driver is it more efficient to allow USB core check it for them (if driver requests it to do so).
ok, fair enough, but it's just one of many things they should be checking, this doesn't provide all that much "protection".
Quoted 6 lines
Quoted 2 lines
How many
drivers do you have that is going to care?I saw at least 3 that did not check, that's from cursory glance. Plus we have many that do check explicitly.
Quoted 6 lines
Quoted 2 lines
Why is this suddenly a new
thing that we haven't run into in the past 15+ years?We are less trusting now. Before we/some of the drivers believed that if device has VID/PID that they recognize the rest of descriptors will have the data we expect, but we can't rely on this anymore.
There's lots of things we can't "rely on", and we have never been able to rely on, but this is going to require we touch every USB driver to make those changes, this one change isn't going to really do all that much to help out with that.
Quoted 4 lines
Every USB driver _should_ be having a loop over all endpoints to find what they need/expect, and if it isn't there, then it needs to abort. Just checking the number of endpoints isn't ok, so I really think this isn't going to help all that much in the end...
OK, fair enough. Maybe what is missing is something like:
ep =
usb_locate_endpoint(altsetting, type, direction);if (!ep) { ... return -EINVAL; }that would loop through endpoints so that drivers do not have to open-code the loop and we indeed need to fix the drivers that blindly grab endpoints at fixed offsets and expect them to be there and have correct types.
Quoted 6 lines
Quoted 1 line
Let's consider this patch dropped.
Since you're dropping this patch, are you going to take the patch Vladis originally sent for the aiptek driver? I'm not objecting to fixing this in a broader sense, but it might be good to get existing fixes in before the whole rework is done.
Yeah, I'd better.
Thanks.
Signature · 1 line
Dmitry