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

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

On Wed, Dec 01, 2021 at 06:40:08PM +0100, Benjamin Tissoires wrote:
> On 12/1/21 17:38, Greg Kroah-Hartman 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>
> > Cc: linux-input@vger.kernel.org
> > Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> > ---
> > v2: holtek_kbd_probe() changes to test for USB device before calling
> >      hid_probe() and dereferenced the usb interface pointer based on
> >      Benjamin's review.
> > 
> >   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      |  9 +++++++--
> >   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 +++
> >   22 files changed, 87 insertions(+), 8 deletions(-)
> 
> Sorry I haven't spotted this in the earlier versions: hid-sony also
> needs a fix:
> 
> ---
> diff --git a/drivers/hid/hid-sony.c b/drivers/hid/hid-sony.c
> index d1b107d547f5..c186af552129 100644
> --- a/drivers/hid/hid-sony.c
> +++ b/drivers/hid/hid-sony.c
> @@ -3000,7 +3000,6 @@ static int sony_probe(struct hid_device *hdev, const struct hid_device_id *id)
>  	sc->quirks = quirks;
>  	hid_set_drvdata(hdev, sc);
>  	sc->hdev = hdev;
> -	usbdev = to_usb_device(sc->hdev->dev.parent->parent);
>  	ret = hid_parse(hdev);
>  	if (ret) {
> @@ -3043,6 +3042,11 @@ static int sony_probe(struct hid_device *hdev, const struct hid_device_id *id)
>  	}
>  	if (sc->quirks & (GHL_GUITAR_PS3WIIU | GHL_GUITAR_PS4)) {
> +		if (!hid_is_usb(hdev))
> +			return -EINVAL;
> +
> +		usbdev = to_usb_device(sc->hdev->dev.parent->parent);
> +
>  		sc->ghl_urb = usb_alloc_urb(0, GFP_ATOMIC);
>  		if (!sc->ghl_urb)
>  			return -ENOMEM;

Ah, good catch, will go roll that into the next version.

> With that patch and the bigbenff which is not USB related, the hid-tools
> test suite manages to inject all the declared USB devices.

Very nice!

I'll submit v3 now.

thanks for the quick review and test,

greg k-h