| Message ID | 20211201141513.2107815-2-gregkh@linuxfoundation.org (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Jiri Kosina |
| Headers | show |
| Series | [1/2] HID: add hid_is_usb() function to make it simpler for USB detection | expand |
On Wed, Dec 1, 2021 at 3:15 PM Greg Kroah-Hartman <gregkh@linuxfoundation.org> wrote: > > Many HID drivers assume that the HID device assigned to them is a USB > device as that was the only way HID devices used to be able to be > created in Linux. However, with the additional ways that HID devices > can be created for many different bus types, that is no longer true, so > properly check that we have a USB device associated with the HID device > before allowing a driver that makes this assumption to claim it. > > Cc: Jiri Kosina <jikos@kernel.org> > Cc: Benjamin Tissoires <benjamin.tissoires@redhat.com> > Cc: Michael Zaidman <michael.zaidman@gmail.com> > Cc: Stefan Achatz <erazor_de@users.sourceforge.net> > Cc: Maxime Coquelin <mcoquelin.stm32@gmail.com> > Cc: Alexandre Torgue <alexandre.torgue@foss.st.com> > Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org> > --- > drivers/hid/hid-chicony.c | 3 +++ > drivers/hid/hid-corsair.c | 7 ++++++- > drivers/hid/hid-elan.c | 2 +- > drivers/hid/hid-elo.c | 3 +++ > drivers/hid/hid-ft260.c | 3 +++ > drivers/hid/hid-holtek-kbd.c | 3 +++ > drivers/hid/hid-holtek-mouse.c | 9 +++++++++ > drivers/hid/hid-lg.c | 10 ++++++++-- > drivers/hid/hid-prodikeys.c | 10 ++++++++-- > drivers/hid/hid-roccat-arvo.c | 3 +++ > drivers/hid/hid-roccat-isku.c | 3 +++ > drivers/hid/hid-roccat-kone.c | 3 +++ > drivers/hid/hid-roccat-koneplus.c | 3 +++ > drivers/hid/hid-roccat-konepure.c | 3 +++ > drivers/hid/hid-roccat-kovaplus.c | 3 +++ > drivers/hid/hid-roccat-lua.c | 3 +++ > drivers/hid/hid-roccat-pyra.c | 3 +++ > drivers/hid/hid-roccat-ryos.c | 3 +++ > drivers/hid/hid-roccat-savu.c | 3 +++ > drivers/hid/hid-samsung.c | 3 +++ > drivers/hid/hid-thrustmaster.c | 3 +++ > drivers/hid/hid-uclogic-core.c | 3 +++ > drivers/hid/wacom_sys.c | 10 ++++++++-- > 23 files changed, 91 insertions(+), 8 deletions(-) > > diff --git a/drivers/hid/hid-chicony.c b/drivers/hid/hid-chicony.c > index ca556d39da2a..f04d2aa23efe 100644 > --- a/drivers/hid/hid-chicony.c > +++ b/drivers/hid/hid-chicony.c > @@ -114,6 +114,9 @@ static int ch_probe(struct hid_device *hdev, const struct hid_device_id *id) > { > int ret; > > + if (!hid_is_usb(hdev)) > + return -EINVAL; A few lines above, we have: struct usb_interface *intf = to_usb_interface(hdev->dev.parent); I assume we also want to move that statement below. > + > hdev->quirks |= HID_QUIRK_INPUT_PER_APP; > ret = hid_parse(hdev); > if (ret) { > diff --git a/drivers/hid/hid-corsair.c b/drivers/hid/hid-corsair.c > index 902a60e249ed..8c895c820b67 100644 > --- a/drivers/hid/hid-corsair.c > +++ b/drivers/hid/hid-corsair.c > @@ -553,7 +553,12 @@ static int corsair_probe(struct hid_device *dev, const struct hid_device_id *id) > int ret; > unsigned long quirks = id->driver_data; > struct corsair_drvdata *drvdata; > - struct usb_interface *usbif = to_usb_interface(dev->dev.parent); > + struct usb_interface *usbif; > + > + if (!hid_is_usb(dev)) > + return -EINVAL; > + > + usbif = to_usb_interface(dev->dev.parent); > > drvdata = devm_kzalloc(&dev->dev, sizeof(struct corsair_drvdata), > GFP_KERNEL); > diff --git a/drivers/hid/hid-elan.c b/drivers/hid/hid-elan.c > index 021049805bb7..3091355d48df 100644 > --- a/drivers/hid/hid-elan.c > +++ b/drivers/hid/hid-elan.c > @@ -50,7 +50,7 @@ struct elan_drvdata { > > static int is_not_elan_touchpad(struct hid_device *hdev) > { > - if (hdev->bus == BUS_USB) { > + if (hid_is_usb(hdev)) { > struct usb_interface *intf = to_usb_interface(hdev->dev.parent); > > return (intf->altsetting->desc.bInterfaceNumber != > diff --git a/drivers/hid/hid-elo.c b/drivers/hid/hid-elo.c > index 383dfda8c12f..8e960d7b233b 100644 > --- a/drivers/hid/hid-elo.c > +++ b/drivers/hid/hid-elo.c > @@ -230,6 +230,9 @@ static int elo_probe(struct hid_device *hdev, const struct hid_device_id *id) > int ret; > struct usb_device *udev; > > + if (!hid_is_usb(hdev)) > + return -EINVAL; > + > priv = kzalloc(sizeof(*priv), GFP_KERNEL); > if (!priv) > return -ENOMEM; > diff --git a/drivers/hid/hid-ft260.c b/drivers/hid/hid-ft260.c > index 4ef1c3b8094e..183eeb3863b3 100644 > --- a/drivers/hid/hid-ft260.c > +++ b/drivers/hid/hid-ft260.c > @@ -915,6 +915,9 @@ static int ft260_probe(struct hid_device *hdev, const struct hid_device_id *id) > struct ft260_get_chip_version_report version; > int ret; > > + if (!hid_is_usb(hdev)) > + return -EINVAL; We also want to add here the initialization of `usbif` and `interface` from a few lines above > + > dev = devm_kzalloc(&hdev->dev, sizeof(*dev), GFP_KERNEL); > if (!dev) > return -ENOMEM; > diff --git a/drivers/hid/hid-holtek-kbd.c b/drivers/hid/hid-holtek-kbd.c > index 0a38e8e9bc78..a438d8ac00e2 100644 > --- a/drivers/hid/hid-holtek-kbd.c > +++ b/drivers/hid/hid-holtek-kbd.c > @@ -143,6 +143,9 @@ static int holtek_kbd_probe(struct hid_device *hdev, > struct usb_interface *intf = to_usb_interface(hdev->dev.parent); > int ret = hid_parse(hdev); > > + if (!hid_is_usb(hdev)) > + return -EINVAL; Same here, intf should likely be initialized here. > + > if (!ret) > ret = hid_hw_start(hdev, HID_CONNECT_DEFAULT); > > diff --git a/drivers/hid/hid-holtek-mouse.c b/drivers/hid/hid-holtek-mouse.c > index 195b735b001d..b7172c48ef9f 100644 > --- a/drivers/hid/hid-holtek-mouse.c > +++ b/drivers/hid/hid-holtek-mouse.c > @@ -62,6 +62,14 @@ static __u8 *holtek_mouse_report_fixup(struct hid_device *hdev, __u8 *rdesc, > return rdesc; > } > > +static int holtek_mouse_probe(struct hid_device *hdev, > + const struct hid_device_id *id) Do we really need to add a probe here? AFAICT, the only usb calls are in the report fixup, so the rest is usb safe. > +{ > + if (!hid_is_usb(hdev)) > + return -EINVAL; > + return 0; > +} > + > static const struct hid_device_id holtek_mouse_devices[] = { > { HID_USB_DEVICE(USB_VENDOR_ID_HOLTEK_ALT, > USB_DEVICE_ID_HOLTEK_ALT_MOUSE_A067) }, > @@ -83,6 +91,7 @@ static struct hid_driver holtek_mouse_driver = { > .name = "holtek_mouse", > .id_table = holtek_mouse_devices, > .report_fixup = holtek_mouse_report_fixup, > + .probe = holtek_mouse_probe, > }; > > module_hid_driver(holtek_mouse_driver); > diff --git a/drivers/hid/hid-lg.c b/drivers/hid/hid-lg.c > index d40af911df63..fb3f7258009c 100644 > --- a/drivers/hid/hid-lg.c > +++ b/drivers/hid/hid-lg.c > @@ -749,12 +749,18 @@ static int lg_raw_event(struct hid_device *hdev, struct hid_report *report, > > static int lg_probe(struct hid_device *hdev, const struct hid_device_id *id) > { > - struct usb_interface *iface = to_usb_interface(hdev->dev.parent); > - __u8 iface_num = iface->cur_altsetting->desc.bInterfaceNumber; > + struct usb_interface *iface; > + __u8 iface_num; > unsigned int connect_mask = HID_CONNECT_DEFAULT; > struct lg_drv_data *drv_data; > int ret; > > + if (!hid_is_usb(hdev)) > + return -EINVAL; > + > + iface = to_usb_interface(hdev->dev.parent); > + iface_num = iface->cur_altsetting->desc.bInterfaceNumber; > + > /* G29 only work with the 1st interface */ > if ((hdev->product == USB_DEVICE_ID_LOGITECH_G29_WHEEL) && > (iface_num != 0)) { > diff --git a/drivers/hid/hid-prodikeys.c b/drivers/hid/hid-prodikeys.c > index 2666af02d5c1..e4e9471d0f1e 100644 > --- a/drivers/hid/hid-prodikeys.c > +++ b/drivers/hid/hid-prodikeys.c > @@ -798,12 +798,18 @@ static int pk_raw_event(struct hid_device *hdev, struct hid_report *report, > static int pk_probe(struct hid_device *hdev, const struct hid_device_id *id) > { > int ret; > - struct usb_interface *intf = to_usb_interface(hdev->dev.parent); > - unsigned short ifnum = intf->cur_altsetting->desc.bInterfaceNumber; > + struct usb_interface *intf; > + unsigned short ifnum; > unsigned long quirks = id->driver_data; > struct pk_device *pk; > struct pcmidi_snd *pm = NULL; > > + if (!hid_is_usb(hdev)) > + return -EINVAL; > + > + intf = to_usb_interface(hdev->dev.parent); > + ifnum = intf->cur_altsetting->desc.bInterfaceNumber; > + > pk = kzalloc(sizeof(*pk), GFP_KERNEL); > if (pk == NULL) { > hid_err(hdev, "can't alloc descriptor\n"); > diff --git a/drivers/hid/hid-roccat-arvo.c b/drivers/hid/hid-roccat-arvo.c > index 4556d2a50f75..d94ee0539421 100644 > --- a/drivers/hid/hid-roccat-arvo.c > +++ b/drivers/hid/hid-roccat-arvo.c > @@ -344,6 +344,9 @@ static int arvo_probe(struct hid_device *hdev, > { > int retval; > > + if (!hid_is_usb(hdev)) > + return -EINVAL; > + > retval = hid_parse(hdev); > if (retval) { > hid_err(hdev, "parse failed\n"); > diff --git a/drivers/hid/hid-roccat-isku.c b/drivers/hid/hid-roccat-isku.c > index ce5f22519956..e95d59cd8d07 100644 > --- a/drivers/hid/hid-roccat-isku.c > +++ b/drivers/hid/hid-roccat-isku.c > @@ -324,6 +324,9 @@ static int isku_probe(struct hid_device *hdev, > { > int retval; > > + if (!hid_is_usb(hdev)) > + return -EINVAL; > + > retval = hid_parse(hdev); > if (retval) { > hid_err(hdev, "parse failed\n"); > diff --git a/drivers/hid/hid-roccat-kone.c b/drivers/hid/hid-roccat-kone.c > index ea17abc7ad52..76da04801ca9 100644 > --- a/drivers/hid/hid-roccat-kone.c > +++ b/drivers/hid/hid-roccat-kone.c > @@ -749,6 +749,9 @@ static int kone_probe(struct hid_device *hdev, const struct hid_device_id *id) > { > int retval; > > + if (!hid_is_usb(hdev)) > + return -EINVAL; > + > retval = hid_parse(hdev); > if (retval) { > hid_err(hdev, "parse failed\n"); > diff --git a/drivers/hid/hid-roccat-koneplus.c b/drivers/hid/hid-roccat-koneplus.c > index 0316edf8c5bb..1896c69ea512 100644 > --- a/drivers/hid/hid-roccat-koneplus.c > +++ b/drivers/hid/hid-roccat-koneplus.c > @@ -431,6 +431,9 @@ static int koneplus_probe(struct hid_device *hdev, > { > int retval; > > + if (!hid_is_usb(hdev)) > + return -EINVAL; > + > retval = hid_parse(hdev); > if (retval) { > hid_err(hdev, "parse failed\n"); > diff --git a/drivers/hid/hid-roccat-konepure.c b/drivers/hid/hid-roccat-konepure.c > index 5248b3c7cf78..cf8eeb33a125 100644 > --- a/drivers/hid/hid-roccat-konepure.c > +++ b/drivers/hid/hid-roccat-konepure.c > @@ -133,6 +133,9 @@ static int konepure_probe(struct hid_device *hdev, > { > int retval; > > + if (!hid_is_usb(hdev)) > + return -EINVAL; > + > retval = hid_parse(hdev); > if (retval) { > hid_err(hdev, "parse failed\n"); > diff --git a/drivers/hid/hid-roccat-kovaplus.c b/drivers/hid/hid-roccat-kovaplus.c > index 960012881570..6fb9b9563769 100644 > --- a/drivers/hid/hid-roccat-kovaplus.c > +++ b/drivers/hid/hid-roccat-kovaplus.c > @@ -501,6 +501,9 @@ static int kovaplus_probe(struct hid_device *hdev, > { > int retval; > > + if (!hid_is_usb(hdev)) > + return -EINVAL; > + > retval = hid_parse(hdev); > if (retval) { > hid_err(hdev, "parse failed\n"); > diff --git a/drivers/hid/hid-roccat-lua.c b/drivers/hid/hid-roccat-lua.c > index 4a88a76d5c62..d5ddf0d68346 100644 > --- a/drivers/hid/hid-roccat-lua.c > +++ b/drivers/hid/hid-roccat-lua.c > @@ -160,6 +160,9 @@ static int lua_probe(struct hid_device *hdev, > { > int retval; > > + if (!hid_is_usb(hdev)) > + return -EINVAL; > + > retval = hid_parse(hdev); > if (retval) { > hid_err(hdev, "parse failed\n"); > diff --git a/drivers/hid/hid-roccat-pyra.c b/drivers/hid/hid-roccat-pyra.c > index 989927defe8d..4fcc8e7d276f 100644 > --- a/drivers/hid/hid-roccat-pyra.c > +++ b/drivers/hid/hid-roccat-pyra.c > @@ -449,6 +449,9 @@ static int pyra_probe(struct hid_device *hdev, const struct hid_device_id *id) > { > int retval; > > + if (!hid_is_usb(hdev)) > + return -EINVAL; > + > retval = hid_parse(hdev); > if (retval) { > hid_err(hdev, "parse failed\n"); > diff --git a/drivers/hid/hid-roccat-ryos.c b/drivers/hid/hid-roccat-ryos.c > index 3956a6c9c521..5bf1971a2b14 100644 > --- a/drivers/hid/hid-roccat-ryos.c > +++ b/drivers/hid/hid-roccat-ryos.c > @@ -141,6 +141,9 @@ static int ryos_probe(struct hid_device *hdev, > { > int retval; > > + if (!hid_is_usb(hdev)) > + return -EINVAL; > + > retval = hid_parse(hdev); > if (retval) { > hid_err(hdev, "parse failed\n"); > diff --git a/drivers/hid/hid-roccat-savu.c b/drivers/hid/hid-roccat-savu.c > index 818701f7a028..a784bb4ee651 100644 > --- a/drivers/hid/hid-roccat-savu.c > +++ b/drivers/hid/hid-roccat-savu.c > @@ -113,6 +113,9 @@ static int savu_probe(struct hid_device *hdev, > { > int retval; > > + if (!hid_is_usb(hdev)) > + return -EINVAL; > + > retval = hid_parse(hdev); > if (retval) { > hid_err(hdev, "parse failed\n"); > diff --git a/drivers/hid/hid-samsung.c b/drivers/hid/hid-samsung.c > index 2e1c31156eca..cf5992e97094 100644 > --- a/drivers/hid/hid-samsung.c > +++ b/drivers/hid/hid-samsung.c > @@ -152,6 +152,9 @@ static int samsung_probe(struct hid_device *hdev, > int ret; > unsigned int cmask = HID_CONNECT_DEFAULT; > > + if (!hid_is_usb(hdev)) > + return -EINVAL; > + > ret = hid_parse(hdev); > if (ret) { > hid_err(hdev, "parse failed\n"); > diff --git a/drivers/hid/hid-thrustmaster.c b/drivers/hid/hid-thrustmaster.c > index d44550aa8805..0c92b7f9b8b8 100644 > --- a/drivers/hid/hid-thrustmaster.c > +++ b/drivers/hid/hid-thrustmaster.c > @@ -274,6 +274,9 @@ static int thrustmaster_probe(struct hid_device *hdev, const struct hid_device_i > int ret = 0; > struct tm_wheel *tm_wheel = 0; > > + if (!hid_is_usb(hdev)) > + return -EINVAL; > + > ret = hid_parse(hdev); > if (ret) { > hid_err(hdev, "parse failed with error %d\n", ret); > diff --git a/drivers/hid/hid-uclogic-core.c b/drivers/hid/hid-uclogic-core.c > index 6a9865dd703c..d8ab0139e5cd 100644 > --- a/drivers/hid/hid-uclogic-core.c > +++ b/drivers/hid/hid-uclogic-core.c > @@ -164,6 +164,9 @@ static int uclogic_probe(struct hid_device *hdev, > struct uclogic_drvdata *drvdata = NULL; > bool params_initialized = false; > > + if (!hid_is_usb(hdev)) > + return -EINVAL; > + > /* > * libinput requires the pad interface to be on a different node > * than the pen, so use QUIRK_MULTI_INPUT for all tablets. > diff --git a/drivers/hid/wacom_sys.c b/drivers/hid/wacom_sys.c > index 22d73772fbc5..88d9d5930c5f 100644 > --- a/drivers/hid/wacom_sys.c > +++ b/drivers/hid/wacom_sys.c > @@ -2730,16 +2730,22 @@ static void wacom_mode_change_work(struct work_struct *work) > static int wacom_probe(struct hid_device *hdev, > const struct hid_device_id *id) > { > - struct usb_interface *intf = to_usb_interface(hdev->dev.parent); > - struct usb_device *dev = interface_to_usbdev(intf); > + struct usb_interface *intf; > + struct usb_device *dev; > struct wacom *wacom; > struct wacom_wac *wacom_wac; > struct wacom_features *features; > int error; > > + if (!hid_is_usb(hdev)) > + return -EINVAL; Nope. I am pretty sure this is wrong as Wacom also handles Bluetooth and I2C devices, and it can also be emulated relatively safely. There are a few checks for hid_is_using_ll_driver, but as I read once again the driver, it seems that there are some cases where we don't enforce it (the wacom receiver is one immediate case). Thanks for the quick patch. FWIW, I am also working on a test that generates all registered USB vid/pids from the currently running kernel and will try to inject them in the testsuite. This should validate this patch and prevent future mishaps. Cheers, Benjamin > + > if (!id->driver_data) > return -EINVAL; > > + intf = to_usb_interface(hdev->dev.parent); > + dev = interface_to_usbdev(intf); > + > hdev->quirks |= HID_QUIRK_NO_INIT_REPORTS; > > /* hid-core sets this quirk for the boot interface */ > -- > 2.34.1 >
On Wed, Dec 01, 2021 at 03:31:24PM +0100, Benjamin Tissoires wrote: > On Wed, Dec 1, 2021 at 3:15 PM Greg Kroah-Hartman > <gregkh@linuxfoundation.org> wrote: > > > > Many HID drivers assume that the HID device assigned to them is a USB > > device as that was the only way HID devices used to be able to be > > created in Linux. However, with the additional ways that HID devices > > can be created for many different bus types, that is no longer true, so > > properly check that we have a USB device associated with the HID device > > before allowing a driver that makes this assumption to claim it. > > > > Cc: Jiri Kosina <jikos@kernel.org> > > Cc: Benjamin Tissoires <benjamin.tissoires@redhat.com> > > Cc: Michael Zaidman <michael.zaidman@gmail.com> > > Cc: Stefan Achatz <erazor_de@users.sourceforge.net> > > Cc: Maxime Coquelin <mcoquelin.stm32@gmail.com> > > Cc: Alexandre Torgue <alexandre.torgue@foss.st.com> > > Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org> > > --- > > drivers/hid/hid-chicony.c | 3 +++ > > drivers/hid/hid-corsair.c | 7 ++++++- > > drivers/hid/hid-elan.c | 2 +- > > drivers/hid/hid-elo.c | 3 +++ > > drivers/hid/hid-ft260.c | 3 +++ > > drivers/hid/hid-holtek-kbd.c | 3 +++ > > drivers/hid/hid-holtek-mouse.c | 9 +++++++++ > > drivers/hid/hid-lg.c | 10 ++++++++-- > > drivers/hid/hid-prodikeys.c | 10 ++++++++-- > > drivers/hid/hid-roccat-arvo.c | 3 +++ > > drivers/hid/hid-roccat-isku.c | 3 +++ > > drivers/hid/hid-roccat-kone.c | 3 +++ > > drivers/hid/hid-roccat-koneplus.c | 3 +++ > > drivers/hid/hid-roccat-konepure.c | 3 +++ > > drivers/hid/hid-roccat-kovaplus.c | 3 +++ > > drivers/hid/hid-roccat-lua.c | 3 +++ > > drivers/hid/hid-roccat-pyra.c | 3 +++ > > drivers/hid/hid-roccat-ryos.c | 3 +++ > > drivers/hid/hid-roccat-savu.c | 3 +++ > > drivers/hid/hid-samsung.c | 3 +++ > > drivers/hid/hid-thrustmaster.c | 3 +++ > > drivers/hid/hid-uclogic-core.c | 3 +++ > > drivers/hid/wacom_sys.c | 10 ++++++++-- > > 23 files changed, 91 insertions(+), 8 deletions(-) > > > > diff --git a/drivers/hid/hid-chicony.c b/drivers/hid/hid-chicony.c > > index ca556d39da2a..f04d2aa23efe 100644 > > --- a/drivers/hid/hid-chicony.c > > +++ b/drivers/hid/hid-chicony.c > > @@ -114,6 +114,9 @@ static int ch_probe(struct hid_device *hdev, const struct hid_device_id *id) > > { > > int ret; > > > > + if (!hid_is_usb(hdev)) > > + return -EINVAL; > > A few lines above, we have: struct usb_interface *intf = > to_usb_interface(hdev->dev.parent); > > I assume we also want to move that statement below. fixup() runs before probe()? If so, ick, I might have done a number of these patches incorrectly. I thought it came after probe, am I mistaken? > > --- a/drivers/hid/hid-ft260.c > > +++ b/drivers/hid/hid-ft260.c > > @@ -915,6 +915,9 @@ static int ft260_probe(struct hid_device *hdev, const struct hid_device_id *id) > > struct ft260_get_chip_version_report version; > > int ret; > > > > + if (!hid_is_usb(hdev)) > > + return -EINVAL; > > We also want to add here the initialization of `usbif` and `interface` > from a few lines above Oops, missed that, will fix up. > > > + > > dev = devm_kzalloc(&hdev->dev, sizeof(*dev), GFP_KERNEL); > > if (!dev) > > return -ENOMEM; > > diff --git a/drivers/hid/hid-holtek-kbd.c b/drivers/hid/hid-holtek-kbd.c > > index 0a38e8e9bc78..a438d8ac00e2 100644 > > --- a/drivers/hid/hid-holtek-kbd.c > > +++ b/drivers/hid/hid-holtek-kbd.c > > @@ -143,6 +143,9 @@ static int holtek_kbd_probe(struct hid_device *hdev, > > struct usb_interface *intf = to_usb_interface(hdev->dev.parent); > > int ret = hid_parse(hdev); > > > > + if (!hid_is_usb(hdev)) > > + return -EINVAL; > > Same here, intf should likely be initialized here. Will fix up. I'll also move the call to hid_parse() after this check. > > + > > if (!ret) > > ret = hid_hw_start(hdev, HID_CONNECT_DEFAULT); > > > > diff --git a/drivers/hid/hid-holtek-mouse.c b/drivers/hid/hid-holtek-mouse.c > > index 195b735b001d..b7172c48ef9f 100644 > > --- a/drivers/hid/hid-holtek-mouse.c > > +++ b/drivers/hid/hid-holtek-mouse.c > > @@ -62,6 +62,14 @@ static __u8 *holtek_mouse_report_fixup(struct hid_device *hdev, __u8 *rdesc, > > return rdesc; > > } > > > > +static int holtek_mouse_probe(struct hid_device *hdev, > > + const struct hid_device_id *id) > > Do we really need to add a probe here? AFAICT, the only usb calls are > in the report fixup, so the rest is usb safe. Can you "fail" a fixup call to prevent the device from being bound to the driver? > > --- a/drivers/hid/wacom_sys.c > > +++ b/drivers/hid/wacom_sys.c > > @@ -2730,16 +2730,22 @@ static void wacom_mode_change_work(struct work_struct *work) > > static int wacom_probe(struct hid_device *hdev, > > const struct hid_device_id *id) > > { > > - struct usb_interface *intf = to_usb_interface(hdev->dev.parent); > > - struct usb_device *dev = interface_to_usbdev(intf); > > + struct usb_interface *intf; > > + struct usb_device *dev; > > struct wacom *wacom; > > struct wacom_wac *wacom_wac; > > struct wacom_features *features; > > int error; > > > > + if (!hid_is_usb(hdev)) > > + return -EINVAL; > > Nope. I am pretty sure this is wrong as Wacom also handles Bluetooth > and I2C devices, and it can also be emulated relatively safely. If it did handle those types of devices, the above lines would have have caused a mess. But you are right, this is trickier, I will split this out into a separate patch. I can split all of these out into individual ones if you want me to. > FWIW, I am also working on a test that generates all registered USB > vid/pids from the currently running kernel and will try to inject them > in the testsuite. This should validate this patch and prevent future > mishaps. That would be great to see. Let me know if you want these broken up smaller, and I will go fix the above issues. thanks, greg k-h
On Wed, Dec 1, 2021 at 3:43 PM Greg Kroah-Hartman <gregkh@linuxfoundation.org> wrote: > > On Wed, Dec 01, 2021 at 03:31:24PM +0100, Benjamin Tissoires wrote: > > On Wed, Dec 1, 2021 at 3:15 PM Greg Kroah-Hartman > > <gregkh@linuxfoundation.org> wrote: > > > > > > Many HID drivers assume that the HID device assigned to them is a USB > > > device as that was the only way HID devices used to be able to be > > > created in Linux. However, with the additional ways that HID devices > > > can be created for many different bus types, that is no longer true, so > > > properly check that we have a USB device associated with the HID device > > > before allowing a driver that makes this assumption to claim it. > > > > > > Cc: Jiri Kosina <jikos@kernel.org> > > > Cc: Benjamin Tissoires <benjamin.tissoires@redhat.com> > > > Cc: Michael Zaidman <michael.zaidman@gmail.com> > > > Cc: Stefan Achatz <erazor_de@users.sourceforge.net> > > > Cc: Maxime Coquelin <mcoquelin.stm32@gmail.com> > > > Cc: Alexandre Torgue <alexandre.torgue@foss.st.com> > > > Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org> > > > --- > > > drivers/hid/hid-chicony.c | 3 +++ > > > drivers/hid/hid-corsair.c | 7 ++++++- > > > drivers/hid/hid-elan.c | 2 +- > > > drivers/hid/hid-elo.c | 3 +++ > > > drivers/hid/hid-ft260.c | 3 +++ > > > drivers/hid/hid-holtek-kbd.c | 3 +++ > > > drivers/hid/hid-holtek-mouse.c | 9 +++++++++ > > > drivers/hid/hid-lg.c | 10 ++++++++-- > > > drivers/hid/hid-prodikeys.c | 10 ++++++++-- > > > drivers/hid/hid-roccat-arvo.c | 3 +++ > > > drivers/hid/hid-roccat-isku.c | 3 +++ > > > drivers/hid/hid-roccat-kone.c | 3 +++ > > > drivers/hid/hid-roccat-koneplus.c | 3 +++ > > > drivers/hid/hid-roccat-konepure.c | 3 +++ > > > drivers/hid/hid-roccat-kovaplus.c | 3 +++ > > > drivers/hid/hid-roccat-lua.c | 3 +++ > > > drivers/hid/hid-roccat-pyra.c | 3 +++ > > > drivers/hid/hid-roccat-ryos.c | 3 +++ > > > drivers/hid/hid-roccat-savu.c | 3 +++ > > > drivers/hid/hid-samsung.c | 3 +++ > > > drivers/hid/hid-thrustmaster.c | 3 +++ > > > drivers/hid/hid-uclogic-core.c | 3 +++ > > > drivers/hid/wacom_sys.c | 10 ++++++++-- > > > 23 files changed, 91 insertions(+), 8 deletions(-) > > > > > > diff --git a/drivers/hid/hid-chicony.c b/drivers/hid/hid-chicony.c > > > index ca556d39da2a..f04d2aa23efe 100644 > > > --- a/drivers/hid/hid-chicony.c > > > +++ b/drivers/hid/hid-chicony.c > > > @@ -114,6 +114,9 @@ static int ch_probe(struct hid_device *hdev, const struct hid_device_id *id) > > > { > > > int ret; > > > > > > + if (!hid_is_usb(hdev)) > > > + return -EINVAL; > > > > A few lines above, we have: struct usb_interface *intf = > > to_usb_interface(hdev->dev.parent); > > > > I assume we also want to move that statement below. > > fixup() runs before probe()? If so, ick, I might have done a number of > these patches incorrectly. I thought it came after probe, am I > mistaken? > > > > --- a/drivers/hid/hid-ft260.c > > > +++ b/drivers/hid/hid-ft260.c > > > @@ -915,6 +915,9 @@ static int ft260_probe(struct hid_device *hdev, const struct hid_device_id *id) > > > struct ft260_get_chip_version_report version; > > > int ret; > > > > > > + if (!hid_is_usb(hdev)) > > > + return -EINVAL; > > > > We also want to add here the initialization of `usbif` and `interface` > > from a few lines above > > Oops, missed that, will fix up. > > > > > > + > > > dev = devm_kzalloc(&hdev->dev, sizeof(*dev), GFP_KERNEL); > > > if (!dev) > > > return -ENOMEM; > > > diff --git a/drivers/hid/hid-holtek-kbd.c b/drivers/hid/hid-holtek-kbd.c > > > index 0a38e8e9bc78..a438d8ac00e2 100644 > > > --- a/drivers/hid/hid-holtek-kbd.c > > > +++ b/drivers/hid/hid-holtek-kbd.c > > > @@ -143,6 +143,9 @@ static int holtek_kbd_probe(struct hid_device *hdev, > > > struct usb_interface *intf = to_usb_interface(hdev->dev.parent); > > > int ret = hid_parse(hdev); > > > > > > + if (!hid_is_usb(hdev)) > > > + return -EINVAL; > > > > Same here, intf should likely be initialized here. > > Will fix up. > > I'll also move the call to hid_parse() after this check. > > > > + > > > if (!ret) > > > ret = hid_hw_start(hdev, HID_CONNECT_DEFAULT); > > > > > > diff --git a/drivers/hid/hid-holtek-mouse.c b/drivers/hid/hid-holtek-mouse.c > > > index 195b735b001d..b7172c48ef9f 100644 > > > --- a/drivers/hid/hid-holtek-mouse.c > > > +++ b/drivers/hid/hid-holtek-mouse.c > > > @@ -62,6 +62,14 @@ static __u8 *holtek_mouse_report_fixup(struct hid_device *hdev, __u8 *rdesc, > > > return rdesc; > > > } > > > > > > +static int holtek_mouse_probe(struct hid_device *hdev, > > > + const struct hid_device_id *id) > > > > Do we really need to add a probe here? AFAICT, the only usb calls are > > in the report fixup, so the rest is usb safe. > > Can you "fail" a fixup call to prevent the device from being bound to > the driver? Not really, but if you return the original report descriptor it wouldn't be worse than plugging the USB device without any driver. HID might complain in that particular case but the kernel will not crash. > > > > --- a/drivers/hid/wacom_sys.c > > > +++ b/drivers/hid/wacom_sys.c > > > @@ -2730,16 +2730,22 @@ static void wacom_mode_change_work(struct work_struct *work) > > > static int wacom_probe(struct hid_device *hdev, > > > const struct hid_device_id *id) > > > { > > > - struct usb_interface *intf = to_usb_interface(hdev->dev.parent); > > > - struct usb_device *dev = interface_to_usbdev(intf); > > > + struct usb_interface *intf; > > > + struct usb_device *dev; > > > struct wacom *wacom; > > > struct wacom_wac *wacom_wac; > > > struct wacom_features *features; > > > int error; > > > > > > + if (!hid_is_usb(hdev)) > > > + return -EINVAL; > > > > Nope. I am pretty sure this is wrong as Wacom also handles Bluetooth > > and I2C devices, and it can also be emulated relatively safely. > > If it did handle those types of devices, the above lines would have > have caused a mess. But you are right, this is trickier, I will split > this out into a separate patch. I assume nobody ever noticed it because the 2 definitions can be statically computed based on the hid device address... But I agree, this is clearly wrong. > > I can split all of these out into individual ones if you want me to. I initially wanted to have them splitted into individual ones, but OTOH, the bulk of it is really a no-brainer so we can leave them in one big patch. However, the wacom chunk might need to be reviewed with an extra pair of eyes, so probably having it split out from the patch would be nice. > > > FWIW, I am also working on a test that generates all registered USB > > vid/pids from the currently running kernel and will try to inject them > > in the testsuite. This should validate this patch and prevent future > > mishaps. > > That would be great to see. Let me know if you want these broken up > smaller, and I will go fix the above issues. https://gitlab.freedesktop.org/bentiss/hid-tools/-/commit/2b04c6cf46c7405c8a916f324988660cb54dadea This seems sufficient to make an unpatched kernel scream :) Cheers, Benjamin > > thanks, > > greg k-h >
On Wed, Dec 01, 2021 at 03:43:43PM +0100, Greg Kroah-Hartman wrote: > On Wed, Dec 01, 2021 at 03:31:24PM +0100, Benjamin Tissoires wrote: > > On Wed, Dec 1, 2021 at 3:15 PM Greg Kroah-Hartman > > <gregkh@linuxfoundation.org> wrote: > > > > > > Many HID drivers assume that the HID device assigned to them is a USB > > > device as that was the only way HID devices used to be able to be > > > created in Linux. However, with the additional ways that HID devices > > > can be created for many different bus types, that is no longer true, so > > > properly check that we have a USB device associated with the HID device > > > before allowing a driver that makes this assumption to claim it. > > > > > > Cc: Jiri Kosina <jikos@kernel.org> > > > Cc: Benjamin Tissoires <benjamin.tissoires@redhat.com> > > > Cc: Michael Zaidman <michael.zaidman@gmail.com> > > > Cc: Stefan Achatz <erazor_de@users.sourceforge.net> > > > Cc: Maxime Coquelin <mcoquelin.stm32@gmail.com> > > > Cc: Alexandre Torgue <alexandre.torgue@foss.st.com> > > > Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org> > > > --- > > > drivers/hid/hid-chicony.c | 3 +++ > > > drivers/hid/hid-corsair.c | 7 ++++++- > > > drivers/hid/hid-elan.c | 2 +- > > > drivers/hid/hid-elo.c | 3 +++ > > > drivers/hid/hid-ft260.c | 3 +++ > > > drivers/hid/hid-holtek-kbd.c | 3 +++ > > > drivers/hid/hid-holtek-mouse.c | 9 +++++++++ > > > drivers/hid/hid-lg.c | 10 ++++++++-- > > > drivers/hid/hid-prodikeys.c | 10 ++++++++-- > > > drivers/hid/hid-roccat-arvo.c | 3 +++ > > > drivers/hid/hid-roccat-isku.c | 3 +++ > > > drivers/hid/hid-roccat-kone.c | 3 +++ > > > drivers/hid/hid-roccat-koneplus.c | 3 +++ > > > drivers/hid/hid-roccat-konepure.c | 3 +++ > > > drivers/hid/hid-roccat-kovaplus.c | 3 +++ > > > drivers/hid/hid-roccat-lua.c | 3 +++ > > > drivers/hid/hid-roccat-pyra.c | 3 +++ > > > drivers/hid/hid-roccat-ryos.c | 3 +++ > > > drivers/hid/hid-roccat-savu.c | 3 +++ > > > drivers/hid/hid-samsung.c | 3 +++ > > > drivers/hid/hid-thrustmaster.c | 3 +++ > > > drivers/hid/hid-uclogic-core.c | 3 +++ > > > drivers/hid/wacom_sys.c | 10 ++++++++-- > > > 23 files changed, 91 insertions(+), 8 deletions(-) > > > > > > diff --git a/drivers/hid/hid-chicony.c b/drivers/hid/hid-chicony.c > > > index ca556d39da2a..f04d2aa23efe 100644 > > > --- a/drivers/hid/hid-chicony.c > > > +++ b/drivers/hid/hid-chicony.c > > > @@ -114,6 +114,9 @@ static int ch_probe(struct hid_device *hdev, const struct hid_device_id *id) > > > { > > > int ret; > > > > > > + if (!hid_is_usb(hdev)) > > > + return -EINVAL; > > > > A few lines above, we have: struct usb_interface *intf = > > to_usb_interface(hdev->dev.parent); > > > > I assume we also want to move that statement below. > > fixup() runs before probe()? If so, ick, I might have done a number of > these patches incorrectly. I thought it came after probe, am I > mistaken? In looking at the code, no, I think my change is correct. report_fixup() gets called when hid_probe() is called, but this check short-circuts that and prevents any non-USB device from being bound to this driver. Which, for now at least, is what we want. If someone really really wants to emulate this, or some of the other drivers in this patch, then they can work to make the checks more fine-grained. For some, like this one: > > > --- a/drivers/hid/hid-ft260.c > > > +++ b/drivers/hid/hid-ft260.c > > > @@ -915,6 +915,9 @@ static int ft260_probe(struct hid_device *hdev, const struct hid_device_id *id) > > > struct ft260_get_chip_version_report version; > > > int ret; > > > > > > + if (!hid_is_usb(hdev)) > > > + return -EINVAL; > > > > We also want to add here the initialization of `usbif` and `interface` > > from a few lines above > > Oops, missed that, will fix up. We don't want to even attempt to bind a non-USB device to the driver as there is no way to actually tell what type of device it is as the check logic would be confused. So I think it's best to just be safe here and not allow these drivers to bind. > > > + > > > dev = devm_kzalloc(&hdev->dev, sizeof(*dev), GFP_KERNEL); > > > if (!dev) > > > return -ENOMEM; > > > diff --git a/drivers/hid/hid-holtek-kbd.c b/drivers/hid/hid-holtek-kbd.c > > > index 0a38e8e9bc78..a438d8ac00e2 100644 > > > --- a/drivers/hid/hid-holtek-kbd.c > > > +++ b/drivers/hid/hid-holtek-kbd.c > > > @@ -143,6 +143,9 @@ static int holtek_kbd_probe(struct hid_device *hdev, > > > struct usb_interface *intf = to_usb_interface(hdev->dev.parent); > > > int ret = hid_parse(hdev); > > > > > > + if (!hid_is_usb(hdev)) > > > + return -EINVAL; > > > > Same here, intf should likely be initialized here. > > Will fix up. > > I'll also move the call to hid_parse() after this check. > > > > + > > > if (!ret) > > > ret = hid_hw_start(hdev, HID_CONNECT_DEFAULT); > > > > > > diff --git a/drivers/hid/hid-holtek-mouse.c b/drivers/hid/hid-holtek-mouse.c > > > index 195b735b001d..b7172c48ef9f 100644 > > > --- a/drivers/hid/hid-holtek-mouse.c > > > +++ b/drivers/hid/hid-holtek-mouse.c > > > @@ -62,6 +62,14 @@ static __u8 *holtek_mouse_report_fixup(struct hid_device *hdev, __u8 *rdesc, > > > return rdesc; > > > } > > > > > > +static int holtek_mouse_probe(struct hid_device *hdev, > > > + const struct hid_device_id *id) > > > > Do we really need to add a probe here? AFAICT, the only usb calls are > > in the report fixup, so the rest is usb safe. > > Can you "fail" a fixup call to prevent the device from being bound to > the driver? To follow up on this, yes, we need the probe to fail here as the driver "has" to be able to look at the USB device information in order to determine what type it is so that it can "fix up" the broken report it contains. Which is the whole reason for this driver in the first place, so it has to only be allowed for USB devices. thanks, greg k-h
On Wed, Dec 1, 2021 at 4:05 PM Benjamin Tissoires <benjamin.tissoires@redhat.com> wrote: > > > FWIW, I am also working on a test that generates all registered USB > > > vid/pids from the currently running kernel and will try to inject them > > > in the testsuite. This should validate this patch and prevent future > > > mishaps. > > > > That would be great to see. Let me know if you want these broken up > > smaller, and I will go fix the above issues. > > https://gitlab.freedesktop.org/bentiss/hid-tools/-/commit/2b04c6cf46c7405c8a916f324988660cb54dadea > > This seems sufficient to make an unpatched kernel scream :) I have now opened https://gitlab.freedesktop.org/libevdev/hid-tools/-/merge_requests/122 with a quicker solution. This leads apparently to 2 extra crashes: - one in hid-holtek-kbd -> hid_is_usb() is called *after* hid_parse(), so report_fixup is actually called for uhid devices. - another in bigbenff which is fixed by the following patch: --- diff --git a/drivers/hid/hid-bigbenff.c b/drivers/hid/hid-bigbenff.c index db6da21ade06..74ad8bf98bfd 100644 --- a/drivers/hid/hid-bigbenff.c +++ b/drivers/hid/hid-bigbenff.c @@ -191,7 +191,7 @@ static void bigben_worker(struct work_struct *work) struct bigben_device, worker); struct hid_field *report_field = bigben->report->field[0]; - if (bigben->removed) + if (bigben->removed || !report_field) return; if (bigben->work_led) { --- Cheers, Benjamin
On Wed, Dec 01, 2021 at 05:38:49PM +0100, Benjamin Tissoires wrote: > > > On Wed, Dec 1, 2021 at 4:05 PM Benjamin Tissoires <benjamin.tissoires@redhat.com> wrote: > > > > FWIW, I am also working on a test that generates all registered USB > > > > vid/pids from the currently running kernel and will try to inject them > > > > in the testsuite. This should validate this patch and prevent future > > > > mishaps. > > > > > > That would be great to see. Let me know if you want these broken up > > > smaller, and I will go fix the above issues. > > > > https://gitlab.freedesktop.org/bentiss/hid-tools/-/commit/2b04c6cf46c7405c8a916f324988660cb54dadea > > > > This seems sufficient to make an unpatched kernel scream :) > > I have now opened https://gitlab.freedesktop.org/libevdev/hid-tools/-/merge_requests/122 > with a quicker solution. > > This leads apparently to 2 extra crashes: > - one in hid-holtek-kbd -> hid_is_usb() is called *after* hid_parse(), so > report_fixup is actually called for uhid devices. Fixed that one in my v2 patch submission. > - another in bigbenff which is fixed by the following patch: > > --- > diff --git a/drivers/hid/hid-bigbenff.c b/drivers/hid/hid-bigbenff.c > index db6da21ade06..74ad8bf98bfd 100644 > --- a/drivers/hid/hid-bigbenff.c > +++ b/drivers/hid/hid-bigbenff.c > @@ -191,7 +191,7 @@ static void bigben_worker(struct work_struct *work) > struct bigben_device, worker); > struct hid_field *report_field = bigben->report->field[0]; > - if (bigben->removed) > + if (bigben->removed || !report_field) I don't see how this is a "USB device" issue. It's a good "fuzz the driver with invalid data" fix though :) thanks, greg k-h
On Wed, Dec 01, 2021 at 05:47:20PM +0100, Greg Kroah-Hartman wrote: > On Wed, Dec 01, 2021 at 05:38:49PM +0100, Benjamin Tissoires wrote: > > > > > > On Wed, Dec 1, 2021 at 4:05 PM Benjamin Tissoires <benjamin.tissoires@redhat.com> wrote: > > > > > FWIW, I am also working on a test that generates all registered USB > > > > > vid/pids from the currently running kernel and will try to inject them > > > > > in the testsuite. This should validate this patch and prevent future > > > > > mishaps. > > > > > > > > That would be great to see. Let me know if you want these broken up > > > > smaller, and I will go fix the above issues. > > > > > > https://gitlab.freedesktop.org/bentiss/hid-tools/-/commit/2b04c6cf46c7405c8a916f324988660cb54dadea > > > > > > This seems sufficient to make an unpatched kernel scream :) > > > > I have now opened https://gitlab.freedesktop.org/libevdev/hid-tools/-/merge_requests/122 > > with a quicker solution. > > > > This leads apparently to 2 extra crashes: > > - one in hid-holtek-kbd -> hid_is_usb() is called *after* hid_parse(), so > > report_fixup is actually called for uhid devices. > > Fixed that one in my v2 patch submission. > > > - another in bigbenff which is fixed by the following patch: > > > > --- > > diff --git a/drivers/hid/hid-bigbenff.c b/drivers/hid/hid-bigbenff.c > > index db6da21ade06..74ad8bf98bfd 100644 > > --- a/drivers/hid/hid-bigbenff.c > > +++ b/drivers/hid/hid-bigbenff.c > > @@ -191,7 +191,7 @@ static void bigben_worker(struct work_struct *work) > > struct bigben_device, worker); > > struct hid_field *report_field = bigben->report->field[0]; > > - if (bigben->removed) > > + if (bigben->removed || !report_field) > > I don't see how this is a "USB device" issue. It's a good "fuzz the > driver with invalid data" fix though :) And to be fair, you are going to find a _LOT_ of these types of issues in most of the HID drivers once you start fuzzing like this. Which is good, but independent of the USB problem. I have seen people starting to use syzbot to create USB HID devices (works as a loopback, so they are real), and start to fuzz the drivers. You've had some reports already, I imagine once they realize they should use the ids in the list from your test above, lots more issues can be found... thanks, greg k-h
On Fri, Dec 03, 2021 at 07:42:12AM -0800, Ping Cheng wrote: > On Thu, Dec 2, 2021, 7:03 PM Greg Kroah-Hartman <gregkh@linuxfoundation.org> > wrote: > > > On Wed, Dec 01, 2021 at 03:31:24PM +0100, Benjamin Tissoires wrote: > > > On Wed, Dec 1, 2021 at 3:15 PM Greg Kroah-Hartman > > > <gregkh@linuxfoundation.org> wrote: > > > > > > > > Many HID drivers assume that the HID device assigned to them is a USB > > > > device as that was the only way HID devices used to be able to be > > > > created in Linux. However, with the additional ways that HID devices > > > > can be created for many different bus types, that is no longer true, so > > > > properly check that we have a USB device associated with the HID device > > > > before allowing a driver that makes this assumption to claim it. > > > > > > > > Cc: Jiri Kosina <jikos@kernel.org> > > > > Cc: Benjamin Tissoires <benjamin.tissoires@redhat.com> > > > > Cc: Michael Zaidman <michael.zaidman@gmail.com> > > > > Cc: Stefan Achatz <erazor_de@users.sourceforge.net> > > > > Cc: Maxime Coquelin <mcoquelin.stm32@gmail.com> > > > > Cc: Alexandre Torgue <alexandre.torgue@foss.st.com> > > > > Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org> > > > > --- > > > > drivers/hid/hid-chicony.c | 3 +++ > > > > drivers/hid/hid-corsair.c | 7 ++++++- > > > > drivers/hid/hid-elan.c | 2 +- > > > > drivers/hid/hid-elo.c | 3 +++ > > > > drivers/hid/hid-ft260.c | 3 +++ > > > > drivers/hid/hid-holtek-kbd.c | 3 +++ > > > > drivers/hid/hid-holtek-mouse.c | 9 +++++++++ > > > > drivers/hid/hid-lg.c | 10 ++++++++-- > > > > drivers/hid/hid-prodikeys.c | 10 ++++++++-- > > > > drivers/hid/hid-roccat-arvo.c | 3 +++ > > > > drivers/hid/hid-roccat-isku.c | 3 +++ > > > > drivers/hid/hid-roccat-kone.c | 3 +++ > > > > drivers/hid/hid-roccat-koneplus.c | 3 +++ > > > > drivers/hid/hid-roccat-konepure.c | 3 +++ > > > > drivers/hid/hid-roccat-kovaplus.c | 3 +++ > > > > drivers/hid/hid-roccat-lua.c | 3 +++ > > > > drivers/hid/hid-roccat-pyra.c | 3 +++ > > > > drivers/hid/hid-roccat-ryos.c | 3 +++ > > > > drivers/hid/hid-roccat-savu.c | 3 +++ > > > > drivers/hid/hid-samsung.c | 3 +++ > > > > drivers/hid/hid-thrustmaster.c | 3 +++ > > > > drivers/hid/hid-uclogic-core.c | 3 +++ > > > > drivers/hid/wacom_sys.c | 10 ++++++++-- > > > > 23 files changed, 91 insertions(+), 8 deletions(-) > > > > > > > > diff --git a/drivers/hid/hid-chicony.c b/drivers/hid/hid-chicony.c > > > > index ca556d39da2a..f04d2aa23efe 100644 > > > > --- a/drivers/hid/hid-chicony.c > > > > +++ b/drivers/hid/hid-chicony.c > > > > @@ -114,6 +114,9 @@ static int ch_probe(struct hid_device *hdev, const > > struct hid_device_id *id) > > > > { > > > > int ret; > > > > > > > > + if (!hid_is_usb(hdev)) > > > > + return -EINVAL; > > > > > > A few lines above, we have: struct usb_interface *intf = > > > to_usb_interface(hdev->dev.parent); > > > > > > I assume we also want to move that statement below. > > > > fixup() runs before probe()? If so, ick, I might have done a number of > > these patches incorrectly. I thought it came after probe, am I > > mistaken? > > > > > > --- a/drivers/hid/hid-ft260.c > > > > +++ b/drivers/hid/hid-ft260.c > > > > @@ -915,6 +915,9 @@ static int ft260_probe(struct hid_device *hdev, > > const struct hid_device_id *id) > > > > struct ft260_get_chip_version_report version; > > > > int ret; > > > > > > > > + if (!hid_is_usb(hdev)) > > > > + return -EINVAL; > > > > > > We also want to add here the initialization of `usbif` and `interface` > > > from a few lines above > > > > Oops, missed that, will fix up. > > > > > > > > > + > > > > dev = devm_kzalloc(&hdev->dev, sizeof(*dev), GFP_KERNEL); > > > > if (!dev) > > > > return -ENOMEM; > > > > diff --git a/drivers/hid/hid-holtek-kbd.c > > b/drivers/hid/hid-holtek-kbd.c > > > > index 0a38e8e9bc78..a438d8ac00e2 100644 > > > > --- a/drivers/hid/hid-holtek-kbd.c > > > > +++ b/drivers/hid/hid-holtek-kbd.c > > > > @@ -143,6 +143,9 @@ static int holtek_kbd_probe(struct hid_device > > *hdev, > > > > struct usb_interface *intf = > > to_usb_interface(hdev->dev.parent); > > > > int ret = hid_parse(hdev); > > > > > > > > + if (!hid_is_usb(hdev)) > > > > + return -EINVAL; > > > > > > Same here, intf should likely be initialized here. > > > > Will fix up. > > > > I'll also move the call to hid_parse() after this check. > > > > > > + > > > > if (!ret) > > > > ret = hid_hw_start(hdev, HID_CONNECT_DEFAULT); > > > > > > > > diff --git a/drivers/hid/hid-holtek-mouse.c > > b/drivers/hid/hid-holtek-mouse.c > > > > index 195b735b001d..b7172c48ef9f 100644 > > > > --- a/drivers/hid/hid-holtek-mouse.c > > > > +++ b/drivers/hid/hid-holtek-mouse.c > > > > @@ -62,6 +62,14 @@ static __u8 *holtek_mouse_report_fixup(struct > > hid_device *hdev, __u8 *rdesc, > > > > return rdesc; > > > > } > > > > > > > > +static int holtek_mouse_probe(struct hid_device *hdev, > > > > + const struct hid_device_id *id) > > > > > > Do we really need to add a probe here? AFAICT, the only usb calls are > > > in the report fixup, so the rest is usb safe. > > > > Can you "fail" a fixup call to prevent the device from being bound to > > the driver? > > > > > > --- a/drivers/hid/wacom_sys.c > > > > +++ b/drivers/hid/wacom_sys.c > > > > @@ -2730,16 +2730,22 @@ static void wacom_mode_change_work(struct > > work_struct *work) > > > > static int wacom_probe(struct hid_device *hdev, > > > > const struct hid_device_id *id) > > > > { > > > > - struct usb_interface *intf = > > to_usb_interface(hdev->dev.parent); > > > > - struct usb_device *dev = interface_to_usbdev(intf); > > > > + struct usb_interface *intf; > > > > + struct usb_device *dev; > > > > struct wacom *wacom; > > > > struct wacom_wac *wacom_wac; > > > > struct wacom_features *features; > > > > int error; > > > > > > > > + if (!hid_is_usb(hdev)) > > > > + return -EINVAL; > > > > > > Nope. I am pretty sure this is wrong as Wacom also handles Bluetooth > > > and I2C devices, and it can also be emulated relatively safely. > > > > This is to confirm that wacom.ko supports both BT and I2C devices too. BT > works fine so far (I didn't test recent kernel releases with a BT device > though). I found I2C device was not recognized by newer kernels, say 5.14 > or later. The device works fine on 5.11 kernel. Since I'm using a > prototype, I wasn't sure if it's a device or a driver issue. But, the > driver should support all three types of devices. Thanks for the confirmation, my later patch fixed that assumption. greg k-h
diff --git a/drivers/hid/hid-chicony.c b/drivers/hid/hid-chicony.c index ca556d39da2a..f04d2aa23efe 100644 --- a/drivers/hid/hid-chicony.c +++ b/drivers/hid/hid-chicony.c @@ -114,6 +114,9 @@ static int ch_probe(struct hid_device *hdev, const struct hid_device_id *id) { int ret; + if (!hid_is_usb(hdev)) + return -EINVAL; + hdev->quirks |= HID_QUIRK_INPUT_PER_APP; ret = hid_parse(hdev); if (ret) { diff --git a/drivers/hid/hid-corsair.c b/drivers/hid/hid-corsair.c index 902a60e249ed..8c895c820b67 100644 --- a/drivers/hid/hid-corsair.c +++ b/drivers/hid/hid-corsair.c @@ -553,7 +553,12 @@ static int corsair_probe(struct hid_device *dev, const struct hid_device_id *id) int ret; unsigned long quirks = id->driver_data; struct corsair_drvdata *drvdata; - struct usb_interface *usbif = to_usb_interface(dev->dev.parent); + struct usb_interface *usbif; + + if (!hid_is_usb(dev)) + return -EINVAL; + + usbif = to_usb_interface(dev->dev.parent); drvdata = devm_kzalloc(&dev->dev, sizeof(struct corsair_drvdata), GFP_KERNEL); diff --git a/drivers/hid/hid-elan.c b/drivers/hid/hid-elan.c index 021049805bb7..3091355d48df 100644 --- a/drivers/hid/hid-elan.c +++ b/drivers/hid/hid-elan.c @@ -50,7 +50,7 @@ struct elan_drvdata { static int is_not_elan_touchpad(struct hid_device *hdev) { - if (hdev->bus == BUS_USB) { + if (hid_is_usb(hdev)) { struct usb_interface *intf = to_usb_interface(hdev->dev.parent); return (intf->altsetting->desc.bInterfaceNumber != diff --git a/drivers/hid/hid-elo.c b/drivers/hid/hid-elo.c index 383dfda8c12f..8e960d7b233b 100644 --- a/drivers/hid/hid-elo.c +++ b/drivers/hid/hid-elo.c @@ -230,6 +230,9 @@ static int elo_probe(struct hid_device *hdev, const struct hid_device_id *id) int ret; struct usb_device *udev; + if (!hid_is_usb(hdev)) + return -EINVAL; + priv = kzalloc(sizeof(*priv), GFP_KERNEL); if (!priv) return -ENOMEM; diff --git a/drivers/hid/hid-ft260.c b/drivers/hid/hid-ft260.c index 4ef1c3b8094e..183eeb3863b3 100644 --- a/drivers/hid/hid-ft260.c +++ b/drivers/hid/hid-ft260.c @@ -915,6 +915,9 @@ static int ft260_probe(struct hid_device *hdev, const struct hid_device_id *id) struct ft260_get_chip_version_report version; int ret; + if (!hid_is_usb(hdev)) + return -EINVAL; + dev = devm_kzalloc(&hdev->dev, sizeof(*dev), GFP_KERNEL); if (!dev) return -ENOMEM; diff --git a/drivers/hid/hid-holtek-kbd.c b/drivers/hid/hid-holtek-kbd.c index 0a38e8e9bc78..a438d8ac00e2 100644 --- a/drivers/hid/hid-holtek-kbd.c +++ b/drivers/hid/hid-holtek-kbd.c @@ -143,6 +143,9 @@ static int holtek_kbd_probe(struct hid_device *hdev, struct usb_interface *intf = to_usb_interface(hdev->dev.parent); int ret = hid_parse(hdev); + if (!hid_is_usb(hdev)) + return -EINVAL; + if (!ret) ret = hid_hw_start(hdev, HID_CONNECT_DEFAULT); diff --git a/drivers/hid/hid-holtek-mouse.c b/drivers/hid/hid-holtek-mouse.c index 195b735b001d..b7172c48ef9f 100644 --- a/drivers/hid/hid-holtek-mouse.c +++ b/drivers/hid/hid-holtek-mouse.c @@ -62,6 +62,14 @@ static __u8 *holtek_mouse_report_fixup(struct hid_device *hdev, __u8 *rdesc, return rdesc; } +static int holtek_mouse_probe(struct hid_device *hdev, + const struct hid_device_id *id) +{ + if (!hid_is_usb(hdev)) + return -EINVAL; + return 0; +} + static const struct hid_device_id holtek_mouse_devices[] = { { HID_USB_DEVICE(USB_VENDOR_ID_HOLTEK_ALT, USB_DEVICE_ID_HOLTEK_ALT_MOUSE_A067) }, @@ -83,6 +91,7 @@ static struct hid_driver holtek_mouse_driver = { .name = "holtek_mouse", .id_table = holtek_mouse_devices, .report_fixup = holtek_mouse_report_fixup, + .probe = holtek_mouse_probe, }; module_hid_driver(holtek_mouse_driver); diff --git a/drivers/hid/hid-lg.c b/drivers/hid/hid-lg.c index d40af911df63..fb3f7258009c 100644 --- a/drivers/hid/hid-lg.c +++ b/drivers/hid/hid-lg.c @@ -749,12 +749,18 @@ static int lg_raw_event(struct hid_device *hdev, struct hid_report *report, static int lg_probe(struct hid_device *hdev, const struct hid_device_id *id) { - struct usb_interface *iface = to_usb_interface(hdev->dev.parent); - __u8 iface_num = iface->cur_altsetting->desc.bInterfaceNumber; + struct usb_interface *iface; + __u8 iface_num; unsigned int connect_mask = HID_CONNECT_DEFAULT; struct lg_drv_data *drv_data; int ret; + if (!hid_is_usb(hdev)) + return -EINVAL; + + iface = to_usb_interface(hdev->dev.parent); + iface_num = iface->cur_altsetting->desc.bInterfaceNumber; + /* G29 only work with the 1st interface */ if ((hdev->product == USB_DEVICE_ID_LOGITECH_G29_WHEEL) && (iface_num != 0)) { diff --git a/drivers/hid/hid-prodikeys.c b/drivers/hid/hid-prodikeys.c index 2666af02d5c1..e4e9471d0f1e 100644 --- a/drivers/hid/hid-prodikeys.c +++ b/drivers/hid/hid-prodikeys.c @@ -798,12 +798,18 @@ static int pk_raw_event(struct hid_device *hdev, struct hid_report *report, static int pk_probe(struct hid_device *hdev, const struct hid_device_id *id) { int ret; - struct usb_interface *intf = to_usb_interface(hdev->dev.parent); - unsigned short ifnum = intf->cur_altsetting->desc.bInterfaceNumber; + struct usb_interface *intf; + unsigned short ifnum; unsigned long quirks = id->driver_data; struct pk_device *pk; struct pcmidi_snd *pm = NULL; + if (!hid_is_usb(hdev)) + return -EINVAL; + + intf = to_usb_interface(hdev->dev.parent); + ifnum = intf->cur_altsetting->desc.bInterfaceNumber; + pk = kzalloc(sizeof(*pk), GFP_KERNEL); if (pk == NULL) { hid_err(hdev, "can't alloc descriptor\n"); diff --git a/drivers/hid/hid-roccat-arvo.c b/drivers/hid/hid-roccat-arvo.c index 4556d2a50f75..d94ee0539421 100644 --- a/drivers/hid/hid-roccat-arvo.c +++ b/drivers/hid/hid-roccat-arvo.c @@ -344,6 +344,9 @@ static int arvo_probe(struct hid_device *hdev, { int retval; + if (!hid_is_usb(hdev)) + return -EINVAL; + retval = hid_parse(hdev); if (retval) { hid_err(hdev, "parse failed\n"); diff --git a/drivers/hid/hid-roccat-isku.c b/drivers/hid/hid-roccat-isku.c index ce5f22519956..e95d59cd8d07 100644 --- a/drivers/hid/hid-roccat-isku.c +++ b/drivers/hid/hid-roccat-isku.c @@ -324,6 +324,9 @@ static int isku_probe(struct hid_device *hdev, { int retval; + if (!hid_is_usb(hdev)) + return -EINVAL; + retval = hid_parse(hdev); if (retval) { hid_err(hdev, "parse failed\n"); diff --git a/drivers/hid/hid-roccat-kone.c b/drivers/hid/hid-roccat-kone.c index ea17abc7ad52..76da04801ca9 100644 --- a/drivers/hid/hid-roccat-kone.c +++ b/drivers/hid/hid-roccat-kone.c @@ -749,6 +749,9 @@ static int kone_probe(struct hid_device *hdev, const struct hid_device_id *id) { int retval; + if (!hid_is_usb(hdev)) + return -EINVAL; + retval = hid_parse(hdev); if (retval) { hid_err(hdev, "parse failed\n"); diff --git a/drivers/hid/hid-roccat-koneplus.c b/drivers/hid/hid-roccat-koneplus.c index 0316edf8c5bb..1896c69ea512 100644 --- a/drivers/hid/hid-roccat-koneplus.c +++ b/drivers/hid/hid-roccat-koneplus.c @@ -431,6 +431,9 @@ static int koneplus_probe(struct hid_device *hdev, { int retval; + if (!hid_is_usb(hdev)) + return -EINVAL; + retval = hid_parse(hdev); if (retval) { hid_err(hdev, "parse failed\n"); diff --git a/drivers/hid/hid-roccat-konepure.c b/drivers/hid/hid-roccat-konepure.c index 5248b3c7cf78..cf8eeb33a125 100644 --- a/drivers/hid/hid-roccat-konepure.c +++ b/drivers/hid/hid-roccat-konepure.c @@ -133,6 +133,9 @@ static int konepure_probe(struct hid_device *hdev, { int retval; + if (!hid_is_usb(hdev)) + return -EINVAL; + retval = hid_parse(hdev); if (retval) { hid_err(hdev, "parse failed\n"); diff --git a/drivers/hid/hid-roccat-kovaplus.c b/drivers/hid/hid-roccat-kovaplus.c index 960012881570..6fb9b9563769 100644 --- a/drivers/hid/hid-roccat-kovaplus.c +++ b/drivers/hid/hid-roccat-kovaplus.c @@ -501,6 +501,9 @@ static int kovaplus_probe(struct hid_device *hdev, { int retval; + if (!hid_is_usb(hdev)) + return -EINVAL; + retval = hid_parse(hdev); if (retval) { hid_err(hdev, "parse failed\n"); diff --git a/drivers/hid/hid-roccat-lua.c b/drivers/hid/hid-roccat-lua.c index 4a88a76d5c62..d5ddf0d68346 100644 --- a/drivers/hid/hid-roccat-lua.c +++ b/drivers/hid/hid-roccat-lua.c @@ -160,6 +160,9 @@ static int lua_probe(struct hid_device *hdev, { int retval; + if (!hid_is_usb(hdev)) + return -EINVAL; + retval = hid_parse(hdev); if (retval) { hid_err(hdev, "parse failed\n"); diff --git a/drivers/hid/hid-roccat-pyra.c b/drivers/hid/hid-roccat-pyra.c index 989927defe8d..4fcc8e7d276f 100644 --- a/drivers/hid/hid-roccat-pyra.c +++ b/drivers/hid/hid-roccat-pyra.c @@ -449,6 +449,9 @@ static int pyra_probe(struct hid_device *hdev, const struct hid_device_id *id) { int retval; + if (!hid_is_usb(hdev)) + return -EINVAL; + retval = hid_parse(hdev); if (retval) { hid_err(hdev, "parse failed\n"); diff --git a/drivers/hid/hid-roccat-ryos.c b/drivers/hid/hid-roccat-ryos.c index 3956a6c9c521..5bf1971a2b14 100644 --- a/drivers/hid/hid-roccat-ryos.c +++ b/drivers/hid/hid-roccat-ryos.c @@ -141,6 +141,9 @@ static int ryos_probe(struct hid_device *hdev, { int retval; + if (!hid_is_usb(hdev)) + return -EINVAL; + retval = hid_parse(hdev); if (retval) { hid_err(hdev, "parse failed\n"); diff --git a/drivers/hid/hid-roccat-savu.c b/drivers/hid/hid-roccat-savu.c index 818701f7a028..a784bb4ee651 100644 --- a/drivers/hid/hid-roccat-savu.c +++ b/drivers/hid/hid-roccat-savu.c @@ -113,6 +113,9 @@ static int savu_probe(struct hid_device *hdev, { int retval; + if (!hid_is_usb(hdev)) + return -EINVAL; + retval = hid_parse(hdev); if (retval) { hid_err(hdev, "parse failed\n"); diff --git a/drivers/hid/hid-samsung.c b/drivers/hid/hid-samsung.c index 2e1c31156eca..cf5992e97094 100644 --- a/drivers/hid/hid-samsung.c +++ b/drivers/hid/hid-samsung.c @@ -152,6 +152,9 @@ static int samsung_probe(struct hid_device *hdev, int ret; unsigned int cmask = HID_CONNECT_DEFAULT; + if (!hid_is_usb(hdev)) + return -EINVAL; + ret = hid_parse(hdev); if (ret) { hid_err(hdev, "parse failed\n"); diff --git a/drivers/hid/hid-thrustmaster.c b/drivers/hid/hid-thrustmaster.c index d44550aa8805..0c92b7f9b8b8 100644 --- a/drivers/hid/hid-thrustmaster.c +++ b/drivers/hid/hid-thrustmaster.c @@ -274,6 +274,9 @@ static int thrustmaster_probe(struct hid_device *hdev, const struct hid_device_i int ret = 0; struct tm_wheel *tm_wheel = 0; + if (!hid_is_usb(hdev)) + return -EINVAL; + ret = hid_parse(hdev); if (ret) { hid_err(hdev, "parse failed with error %d\n", ret); diff --git a/drivers/hid/hid-uclogic-core.c b/drivers/hid/hid-uclogic-core.c index 6a9865dd703c..d8ab0139e5cd 100644 --- a/drivers/hid/hid-uclogic-core.c +++ b/drivers/hid/hid-uclogic-core.c @@ -164,6 +164,9 @@ static int uclogic_probe(struct hid_device *hdev, struct uclogic_drvdata *drvdata = NULL; bool params_initialized = false; + if (!hid_is_usb(hdev)) + return -EINVAL; + /* * libinput requires the pad interface to be on a different node * than the pen, so use QUIRK_MULTI_INPUT for all tablets. diff --git a/drivers/hid/wacom_sys.c b/drivers/hid/wacom_sys.c index 22d73772fbc5..88d9d5930c5f 100644 --- a/drivers/hid/wacom_sys.c +++ b/drivers/hid/wacom_sys.c @@ -2730,16 +2730,22 @@ static void wacom_mode_change_work(struct work_struct *work) static int wacom_probe(struct hid_device *hdev, const struct hid_device_id *id) { - struct usb_interface *intf = to_usb_interface(hdev->dev.parent); - struct usb_device *dev = interface_to_usbdev(intf); + struct usb_interface *intf; + struct usb_device *dev; struct wacom *wacom; struct wacom_wac *wacom_wac; struct wacom_features *features; int error; + if (!hid_is_usb(hdev)) + return -EINVAL; + if (!id->driver_data) return -EINVAL; + intf = to_usb_interface(hdev->dev.parent); + dev = interface_to_usbdev(intf); + hdev->quirks |= HID_QUIRK_NO_INIT_REPORTS; /* hid-core sets this quirk for the boot interface */
Many HID drivers assume that the HID device assigned to them is a USB device as that was the only way HID devices used to be able to be created in Linux. However, with the additional ways that HID devices can be created for many different bus types, that is no longer true, so properly check that we have a USB device associated with the HID device before allowing a driver that makes this assumption to claim it. Cc: Jiri Kosina <jikos@kernel.org> Cc: Benjamin Tissoires <benjamin.tissoires@redhat.com> Cc: Michael Zaidman <michael.zaidman@gmail.com> Cc: Stefan Achatz <erazor_de@users.sourceforge.net> Cc: Maxime Coquelin <mcoquelin.stm32@gmail.com> Cc: Alexandre Torgue <alexandre.torgue@foss.st.com> Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org> --- drivers/hid/hid-chicony.c | 3 +++ drivers/hid/hid-corsair.c | 7 ++++++- drivers/hid/hid-elan.c | 2 +- drivers/hid/hid-elo.c | 3 +++ drivers/hid/hid-ft260.c | 3 +++ drivers/hid/hid-holtek-kbd.c | 3 +++ drivers/hid/hid-holtek-mouse.c | 9 +++++++++ drivers/hid/hid-lg.c | 10 ++++++++-- drivers/hid/hid-prodikeys.c | 10 ++++++++-- drivers/hid/hid-roccat-arvo.c | 3 +++ drivers/hid/hid-roccat-isku.c | 3 +++ drivers/hid/hid-roccat-kone.c | 3 +++ drivers/hid/hid-roccat-koneplus.c | 3 +++ drivers/hid/hid-roccat-konepure.c | 3 +++ drivers/hid/hid-roccat-kovaplus.c | 3 +++ drivers/hid/hid-roccat-lua.c | 3 +++ drivers/hid/hid-roccat-pyra.c | 3 +++ drivers/hid/hid-roccat-ryos.c | 3 +++ drivers/hid/hid-roccat-savu.c | 3 +++ drivers/hid/hid-samsung.c | 3 +++ drivers/hid/hid-thrustmaster.c | 3 +++ drivers/hid/hid-uclogic-core.c | 3 +++ drivers/hid/wacom_sys.c | 10 ++++++++-- 23 files changed, 91 insertions(+), 8 deletions(-)