All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH 1/2] HID: add hid_is_usb() function to make it simpler for USB detection
@ 2021-12-01 14:15 Greg Kroah-Hartman
  2021-12-01 14:15 ` [PATCH 2/2] HID: check for valid USB device for many HID drivers Greg Kroah-Hartman
  0 siblings, 1 reply; 10+ messages in thread
From: Greg Kroah-Hartman @ 2021-12-01 14:15 UTC (permalink / raw)
  To: Jiri Kosina, Benjamin Tissoires
  Cc: linux-input, Greg Kroah-Hartman, linux-kernel

A number of HID drivers already call hid_is_using_ll_driver() but only
for the detection of if this is a USB device or not.  Make this more
obvious by creating hid_is_usb() and calling the function that way.

Also converts the existing hid_is_using_ll_driver() functions to use the
new call.

Cc: Jiri Kosina <jikos@kernel.org>
Cc: Benjamin Tissoires <benjamin.tissoires@redhat.com>
Cc: linux-input@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
---
 drivers/hid/hid-asus.c           | 6 ++----
 drivers/hid/hid-logitech-dj.c    | 2 +-
 drivers/hid/hid-u2fzero.c        | 2 +-
 drivers/hid/hid-uclogic-params.c | 3 +--
 drivers/hid/wacom_sys.c          | 2 +-
 include/linux/hid.h              | 5 +++++
 6 files changed, 11 insertions(+), 9 deletions(-)

diff --git a/drivers/hid/hid-asus.c b/drivers/hid/hid-asus.c
index 5d57214d8dee..c5968fd21d09 100644
--- a/drivers/hid/hid-asus.c
+++ b/drivers/hid/hid-asus.c
@@ -1028,8 +1028,7 @@ static int asus_probe(struct hid_device *hdev, const struct hid_device_id *id)
 	if (drvdata->quirks & QUIRK_IS_MULTITOUCH)
 		drvdata->tp = &asus_i2c_tp;
 
-	if ((drvdata->quirks & QUIRK_T100_KEYBOARD) &&
-	    hid_is_using_ll_driver(hdev, &usb_hid_driver)) {
+	if ((drvdata->quirks & QUIRK_T100_KEYBOARD) && hid_is_usb(hdev)) {
 		struct usb_interface *intf = to_usb_interface(hdev->dev.parent);
 
 		if (intf->altsetting->desc.bInterfaceNumber == T100_TPAD_INTF) {
@@ -1057,8 +1056,7 @@ static int asus_probe(struct hid_device *hdev, const struct hid_device_id *id)
 		drvdata->tp = &asus_t100chi_tp;
 	}
 
-	if ((drvdata->quirks & QUIRK_MEDION_E1239T) &&
-	    hid_is_using_ll_driver(hdev, &usb_hid_driver)) {
+	if ((drvdata->quirks & QUIRK_MEDION_E1239T) && hid_is_usb(hdev)) {
 		struct usb_host_interface *alt =
 			to_usb_interface(hdev->dev.parent)->altsetting;
 
diff --git a/drivers/hid/hid-logitech-dj.c b/drivers/hid/hid-logitech-dj.c
index a0017b010c34..7106b921b53c 100644
--- a/drivers/hid/hid-logitech-dj.c
+++ b/drivers/hid/hid-logitech-dj.c
@@ -1777,7 +1777,7 @@ static int logi_dj_probe(struct hid_device *hdev,
 	case recvr_type_bluetooth:	no_dj_interfaces = 2; break;
 	case recvr_type_dinovo:		no_dj_interfaces = 2; break;
 	}
-	if (hid_is_using_ll_driver(hdev, &usb_hid_driver)) {
+	if (hid_is_usb(hdev)) {
 		intf = to_usb_interface(hdev->dev.parent);
 		if (intf && intf->altsetting->desc.bInterfaceNumber >=
 							no_dj_interfaces) {
diff --git a/drivers/hid/hid-u2fzero.c b/drivers/hid/hid-u2fzero.c
index 31ea7fc69916..ad489caf53ad 100644
--- a/drivers/hid/hid-u2fzero.c
+++ b/drivers/hid/hid-u2fzero.c
@@ -311,7 +311,7 @@ static int u2fzero_probe(struct hid_device *hdev,
 	unsigned int minor;
 	int ret;
 
-	if (!hid_is_using_ll_driver(hdev, &usb_hid_driver))
+	if (!hid_is_usb(hdev))
 		return -EINVAL;
 
 	dev = devm_kzalloc(&hdev->dev, sizeof(*dev), GFP_KERNEL);
diff --git a/drivers/hid/hid-uclogic-params.c b/drivers/hid/hid-uclogic-params.c
index 3d67b748a3b9..adff1bd68d9f 100644
--- a/drivers/hid/hid-uclogic-params.c
+++ b/drivers/hid/hid-uclogic-params.c
@@ -843,8 +843,7 @@ int uclogic_params_init(struct uclogic_params *params,
 	struct uclogic_params p = {0, };
 
 	/* Check arguments */
-	if (params == NULL || hdev == NULL ||
-	    !hid_is_using_ll_driver(hdev, &usb_hid_driver)) {
+	if (params == NULL || hdev == NULL || !hid_is_usb(hdev)) {
 		rc = -EINVAL;
 		goto cleanup;
 	}
diff --git a/drivers/hid/wacom_sys.c b/drivers/hid/wacom_sys.c
index 2717d39600b4..22d73772fbc5 100644
--- a/drivers/hid/wacom_sys.c
+++ b/drivers/hid/wacom_sys.c
@@ -2214,7 +2214,7 @@ static void wacom_update_name(struct wacom *wacom, const char *suffix)
 	if ((features->type == HID_GENERIC) && !strcmp("Wacom HID", features->name)) {
 		char *product_name = wacom->hdev->name;
 
-		if (hid_is_using_ll_driver(wacom->hdev, &usb_hid_driver)) {
+		if (hid_is_usb(wacom->hdev)) {
 			struct usb_interface *intf = to_usb_interface(wacom->hdev->dev.parent);
 			struct usb_device *dev = interface_to_usbdev(intf);
 			product_name = dev->product;
diff --git a/include/linux/hid.h b/include/linux/hid.h
index 9e067f937dbc..f453be385bd4 100644
--- a/include/linux/hid.h
+++ b/include/linux/hid.h
@@ -840,6 +840,11 @@ static inline bool hid_is_using_ll_driver(struct hid_device *hdev,
 	return hdev->ll_driver == driver;
 }
 
+static inline bool hid_is_usb(struct hid_device *hdev)
+{
+	return hid_is_using_ll_driver(hdev, &usb_hid_driver);
+}
+
 #define	PM_HINT_FULLON	1<<5
 #define PM_HINT_NORMAL	1<<1
 
-- 
2.34.1


^ permalink raw reply related	[flat|nested] 10+ messages in thread
* [PATCH 2/2] HID: check for valid USB device for many HID drivers
  2021-12-01 14:15 [PATCH 1/2] HID: add hid_is_usb() function to make it simpler for USB detection Greg Kroah-Hartman
@ 2021-12-01 14:15 ` Greg Kroah-Hartman
  2021-12-01 14:31   ` Benjamin Tissoires
  0 siblings, 1 reply; 10+ messages in thread
From: Greg Kroah-Hartman @ 2021-12-01 14:15 UTC (permalink / raw)
  To: Jiri Kosina, Benjamin Tissoires
  Cc: linux-input, Greg Kroah-Hartman, Michael Zaidman, Stefan Achatz,
	Maxime Coquelin, Alexandre Torgue

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;
+
 	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 */
-- 
2.34.1


^ permalink raw reply related	[flat|nested] 10+ messages in thread
* Re: [PATCH 2/2] HID: check for valid USB device for many HID drivers
  2021-12-01 14:15 ` [PATCH 2/2] HID: check for valid USB device for many HID drivers Greg Kroah-Hartman
@ 2021-12-01 14:31   ` Benjamin Tissoires
  2021-12-01 14:43     ` Greg Kroah-Hartman
  0 siblings, 1 reply; 10+ messages in thread
From: Benjamin Tissoires @ 2021-12-01 14:31 UTC (permalink / raw)
  To: Greg Kroah-Hartman
  Cc: Jiri Kosina, open list:HID CORE LAYER, Michael Zaidman,
	Stefan Achatz, Maxime Coquelin, Alexandre Torgue

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
>


^ permalink raw reply	[flat|nested] 10+ messages in thread
* Re: [PATCH 2/2] HID: check for valid USB device for many HID drivers
  2021-12-01 14:31   ` Benjamin Tissoires
@ 2021-12-01 14:43     ` Greg Kroah-Hartman
  2021-12-01 15:05       ` Benjamin Tissoires
                         ` (2 more replies)
  0 siblings, 3 replies; 10+ messages in thread
From: Greg Kroah-Hartman @ 2021-12-01 14:43 UTC (permalink / raw)
  To: Benjamin Tissoires
  Cc: Jiri Kosina, open list:HID CORE LAYER, Michael Zaidman,
	Stefan Achatz, Maxime Coquelin, Alexandre Torgue

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

^ permalink raw reply	[flat|nested] 10+ messages in thread
* Re: [PATCH 2/2] HID: check for valid USB device for many HID drivers
  2021-12-01 14:43     ` Greg Kroah-Hartman
@ 2021-12-01 15:05       ` Benjamin Tissoires
  2021-12-01 16:38         ` Benjamin Tissoires
  2021-12-01 16:33       ` Greg Kroah-Hartman
       [not found]       ` <CAF8JNhJLB_MiHBcnYDQn6Y4w7OOZvf3O3m30bstMUspW8wn4ag@mail.gmail.com>
  2 siblings, 1 reply; 10+ messages in thread
From: Benjamin Tissoires @ 2021-12-01 15:05 UTC (permalink / raw)
  To: Greg Kroah-Hartman
  Cc: Jiri Kosina, open list:HID CORE LAYER, Michael Zaidman,
	Stefan Achatz, Maxime Coquelin, Alexandre Torgue

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
>


^ permalink raw reply	[flat|nested] 10+ messages in thread
* Re: [PATCH 2/2] HID: check for valid USB device for many HID drivers
  2021-12-01 14:43     ` Greg Kroah-Hartman
  2021-12-01 15:05       ` Benjamin Tissoires
@ 2021-12-01 16:33       ` Greg Kroah-Hartman
       [not found]       ` <CAF8JNhJLB_MiHBcnYDQn6Y4w7OOZvf3O3m30bstMUspW8wn4ag@mail.gmail.com>
  2 siblings, 0 replies; 10+ messages in thread
From: Greg Kroah-Hartman @ 2021-12-01 16:33 UTC (permalink / raw)
  To: Benjamin Tissoires
  Cc: Jiri Kosina, open list:HID CORE LAYER, Michael Zaidman,
	Stefan Achatz, Maxime Coquelin, Alexandre Torgue

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

^ permalink raw reply	[flat|nested] 10+ messages in thread
* Re: [PATCH 2/2] HID: check for valid USB device for many HID drivers
  2021-12-01 15:05       ` Benjamin Tissoires
@ 2021-12-01 16:38         ` Benjamin Tissoires
  2021-12-01 16:47           ` Greg Kroah-Hartman
  0 siblings, 1 reply; 10+ messages in thread
From: Benjamin Tissoires @ 2021-12-01 16:38 UTC (permalink / raw)
  To: Greg Kroah-Hartman
  Cc: Jiri Kosina, open list:HID CORE LAYER, Michael Zaidman,
	Stefan Achatz, Maxime Coquelin, Alexandre Torgue



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


^ permalink raw reply related	[flat|nested] 10+ messages in thread
* Re: [PATCH 2/2] HID: check for valid USB device for many HID drivers
  2021-12-01 16:38         ` Benjamin Tissoires
@ 2021-12-01 16:47           ` Greg Kroah-Hartman
  2021-12-01 16:50             ` Greg Kroah-Hartman
  0 siblings, 1 reply; 10+ messages in thread
From: Greg Kroah-Hartman @ 2021-12-01 16:47 UTC (permalink / raw)
  To: Benjamin Tissoires
  Cc: Jiri Kosina, open list:HID CORE LAYER, Michael Zaidman,
	Stefan Achatz, Maxime Coquelin, Alexandre Torgue

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

^ permalink raw reply	[flat|nested] 10+ messages in thread
* Re: [PATCH 2/2] HID: check for valid USB device for many HID drivers
  2021-12-01 16:47           ` Greg Kroah-Hartman
@ 2021-12-01 16:50             ` Greg Kroah-Hartman
  0 siblings, 0 replies; 10+ messages in thread
From: Greg Kroah-Hartman @ 2021-12-01 16:50 UTC (permalink / raw)
  To: Benjamin Tissoires
  Cc: Jiri Kosina, open list:HID CORE LAYER, Michael Zaidman,
	Stefan Achatz, Maxime Coquelin, Alexandre Torgue

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

^ permalink raw reply	[flat|nested] 10+ messages in thread
* Re: [PATCH 2/2] HID: check for valid USB device for many HID drivers
       [not found]       ` <CAF8JNhJLB_MiHBcnYDQn6Y4w7OOZvf3O3m30bstMUspW8wn4ag@mail.gmail.com>
@ 2021-12-03 15:53         ` Greg Kroah-Hartman
  0 siblings, 0 replies; 10+ messages in thread
From: Greg Kroah-Hartman @ 2021-12-03 15:53 UTC (permalink / raw)
  To: Ping Cheng
  Cc: Benjamin Tissoires, Jiri Kosina, open list:HID CORE LAYER,
	Michael Zaidman, Stefan Achatz, Maxime Coquelin,
	Alexandre Torgue, Jason Gerecke

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

^ permalink raw reply	[flat|nested] 10+ messages in thread
end of thread, other threads:[~2021-12-03 15:54 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-12-01 14:15 [PATCH 1/2] HID: add hid_is_usb() function to make it simpler for USB detection Greg Kroah-Hartman
2021-12-01 14:15 ` [PATCH 2/2] HID: check for valid USB device for many HID drivers Greg Kroah-Hartman
2021-12-01 14:31   ` Benjamin Tissoires
2021-12-01 14:43     ` Greg Kroah-Hartman
2021-12-01 15:05       ` Benjamin Tissoires
2021-12-01 16:38         ` Benjamin Tissoires
2021-12-01 16:47           ` Greg Kroah-Hartman
2021-12-01 16:50             ` Greg Kroah-Hartman
2021-12-01 16:33       ` Greg Kroah-Hartman
     [not found]       ` <CAF8JNhJLB_MiHBcnYDQn6Y4w7OOZvf3O3m30bstMUspW8wn4ag@mail.gmail.com>
2021-12-03 15:53         ` Greg Kroah-Hartman

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.