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

[Greg Kroah-Hartman <gregkh@linuxfoundation.org>]

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