| Message ID | 20231211070808.v2.1.If27eb3bf7812f91ab83810f232292f032f4203e0@changeid (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v2] usb: core: Fix crash w/ usb_choose_configuration() if no driver | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Not a local patch |
On Mon, Dec 11, 2023 at 07:08:14AM -0800, Douglas Anderson wrote: > It's possible that usb_choose_configuration() can get called when a > USB device has no driver. In this case the recent commit a87b8e3be926 > ("usb: core: Allow subclassed USB drivers to override > usb_choose_configuration()") can cause a crash since it dereferenced > the driver structure without checking for NULL. Let's add a check. > > A USB device with no driver is an anomaly, so make > usb_choose_configuration() return immediately if there is no driver. > > This was seen in the real world when usbguard got ahold of a r8152 > device at the wrong time. It can also be simulated via this on a > computer with one r8152-based USB Ethernet adapter: > cd /sys/bus/usb/drivers/r8152-cfgselector > to_unbind="$(ls -d *-*)" > real_dir="$(readlink -f "${to_unbind}")" > echo "${to_unbind}" > unbind > cd "${real_dir}" > echo 0 > authorized > echo 1 > authorized > > Fixes: a87b8e3be926 ("usb: core: Allow subclassed USB drivers to override usb_choose_configuration()") > Signed-off-by: Douglas Anderson <dianders@chromium.org> > --- > > Changes in v2: > - Return immediately if no driver, as per Alan. > > drivers/usb/core/generic.c | 6 +++++- > 1 file changed, 5 insertions(+), 1 deletion(-) > > diff --git a/drivers/usb/core/generic.c b/drivers/usb/core/generic.c > index dcb897158228..2be1e8901e2f 100644 > --- a/drivers/usb/core/generic.c > +++ b/drivers/usb/core/generic.c > @@ -59,7 +59,11 @@ int usb_choose_configuration(struct usb_device *udev) > int num_configs; > int insufficient_power = 0; > struct usb_host_config *c, *best; > - struct usb_device_driver *udriver = to_usb_device_driver(udev->dev.driver); > + struct usb_device_driver *udriver; > + > + if (!udev->dev.driver) > + return -1; This is a rather unusual condition. It would be good to put a comment just before the test, explaining that if a USB device (not an interface) doesn't have a driver then the kernel has no business trying to select or install a configuration for it. Along with the comment, feel free to add: Reviewed-by: Alan Stern <stern@rowland.harvard.edu> Alan Stern > + udriver = to_usb_device_driver(udev->dev.driver); > > if (usb_device_is_owned(udev)) > return 0; > -- > 2.43.0.472.g3155946c3a-goog >
Hi, On Mon, Dec 11, 2023 at 7:27 AM Alan Stern <stern@rowland.harvard.edu> wrote: > > On Mon, Dec 11, 2023 at 07:08:14AM -0800, Douglas Anderson wrote: > > It's possible that usb_choose_configuration() can get called when a > > USB device has no driver. In this case the recent commit a87b8e3be926 > > ("usb: core: Allow subclassed USB drivers to override > > usb_choose_configuration()") can cause a crash since it dereferenced > > the driver structure without checking for NULL. Let's add a check. > > > > A USB device with no driver is an anomaly, so make > > usb_choose_configuration() return immediately if there is no driver. > > > > This was seen in the real world when usbguard got ahold of a r8152 > > device at the wrong time. It can also be simulated via this on a > > computer with one r8152-based USB Ethernet adapter: > > cd /sys/bus/usb/drivers/r8152-cfgselector > > to_unbind="$(ls -d *-*)" > > real_dir="$(readlink -f "${to_unbind}")" > > echo "${to_unbind}" > unbind > > cd "${real_dir}" > > echo 0 > authorized > > echo 1 > authorized > > > > Fixes: a87b8e3be926 ("usb: core: Allow subclassed USB drivers to override usb_choose_configuration()") > > Signed-off-by: Douglas Anderson <dianders@chromium.org> > > --- > > > > Changes in v2: > > - Return immediately if no driver, as per Alan. > > > > drivers/usb/core/generic.c | 6 +++++- > > 1 file changed, 5 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/usb/core/generic.c b/drivers/usb/core/generic.c > > index dcb897158228..2be1e8901e2f 100644 > > --- a/drivers/usb/core/generic.c > > +++ b/drivers/usb/core/generic.c > > @@ -59,7 +59,11 @@ int usb_choose_configuration(struct usb_device *udev) > > int num_configs; > > int insufficient_power = 0; > > struct usb_host_config *c, *best; > > - struct usb_device_driver *udriver = to_usb_device_driver(udev->dev.driver); > > + struct usb_device_driver *udriver; > > + > > + if (!udev->dev.driver) > > + return -1; > > This is a rather unusual condition. It would be good to put a comment > just before the test, explaining that if a USB device (not an interface) > doesn't have a driver then the kernel has no business trying to select > or install a configuration for it. > > Along with the comment, feel free to add: > > Reviewed-by: Alan Stern <stern@rowland.harvard.edu> Thanks for the quick reply. I've added your comment almost verbatim to the code and sent out a quick v3 with your tag: https://lore.kernel.org/r/20231211073237.v3.1.If27eb3bf7812f91ab83810f232292f032f4203e0@changeid
diff --git a/drivers/usb/core/generic.c b/drivers/usb/core/generic.c index dcb897158228..2be1e8901e2f 100644 --- a/drivers/usb/core/generic.c +++ b/drivers/usb/core/generic.c @@ -59,7 +59,11 @@ int usb_choose_configuration(struct usb_device *udev) int num_configs; int insufficient_power = 0; struct usb_host_config *c, *best; - struct usb_device_driver *udriver = to_usb_device_driver(udev->dev.driver); + struct usb_device_driver *udriver; + + if (!udev->dev.driver) + return -1; + udriver = to_usb_device_driver(udev->dev.driver); if (usb_device_is_owned(udev)) return 0;
It's possible that usb_choose_configuration() can get called when a USB device has no driver. In this case the recent commit a87b8e3be926 ("usb: core: Allow subclassed USB drivers to override usb_choose_configuration()") can cause a crash since it dereferenced the driver structure without checking for NULL. Let's add a check. A USB device with no driver is an anomaly, so make usb_choose_configuration() return immediately if there is no driver. This was seen in the real world when usbguard got ahold of a r8152 device at the wrong time. It can also be simulated via this on a computer with one r8152-based USB Ethernet adapter: cd /sys/bus/usb/drivers/r8152-cfgselector to_unbind="$(ls -d *-*)" real_dir="$(readlink -f "${to_unbind}")" echo "${to_unbind}" > unbind cd "${real_dir}" echo 0 > authorized echo 1 > authorized Fixes: a87b8e3be926 ("usb: core: Allow subclassed USB drivers to override usb_choose_configuration()") Signed-off-by: Douglas Anderson <dianders@chromium.org> --- Changes in v2: - Return immediately if no driver, as per Alan. drivers/usb/core/generic.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-)