Re: [PATCH 2/2] HID: check for valid USB device for many HID drivers

On Fri, Dec 03, 2021 at 07:42:12AM -0800, Ping Cheng wrote:

On Thu, Dec 2, 2021, 7:03 PM Greg Kroah-Hartman [off-list ref]
wrote:

quoted

On Wed, Dec 01, 2021 at 03:31:24PM +0100, Benjamin Tissoires wrote:

quoted

On Wed, Dec 1, 2021 at 3:15 PM Greg Kroah-Hartman
[off-list ref] wrote:

quoted

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 <redacted>
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)

quoted

quoted

 {
        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?

quoted

quoted

--- 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)

quoted

quoted

        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.

quoted

quoted

+
        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

quoted

quoted

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,

quoted

quoted

        struct usb_interface *intf =

to_usb_interface(hdev->dev.parent);

quoted

quoted

        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.

quoted

quoted

+
        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

quoted

quoted

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,

quoted

quoted

        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?

quoted

quoted

--- 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)

quoted

quoted

 static int wacom_probe(struct hid_device *hdev,
                const struct hid_device_id *id)
 {
-       struct usb_interface *intf =

to_usb_interface(hdev->dev.parent);

quoted

quoted

-       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