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@xxxxxxxxxxxxxxxxxxx> 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@xxxxxxxxxx>
> > Cc: Benjamin Tissoires <benjamin.tissoires@xxxxxxxxxx>
> > Cc: Michael Zaidman <michael.zaidman@xxxxxxxxx>
> > Cc: Stefan Achatz <erazor_de@xxxxxxxxxxxxxxxxxxxxx>
> > Cc: Maxime Coquelin <mcoquelin.stm32@xxxxxxxxx>
> > Cc: Alexandre Torgue <alexandre.torgue@xxxxxxxxxxx>
> > Signed-off-by: Greg Kroah-Hartman <gregkh@xxxxxxxxxxxxxxxxxxx>
> > ---
> > 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
