[PATCH v2] Revert "Input: bcm5974 - check endpoint type before starting traffic"

[PATCH v2] Revert "Input: bcm5974 - check endpoint type before starting traffic"

[Javier Carrasco]

This patch intended to fix an well-knonw issue in old drivers where the
endpoint type is taken for granted, which is often triggered by fuzzers.

That was the case for this driver [1], and although the fix seems to be
correct, it uncovered another issue that leads to a regression [2], if
the endpoints of the current interface are checked.

The driver makes use of endpoints that belong to a different interface
rather than the one it binds (it binds to the third interface, but also
accesses an endpoint from a different one). The driver should claim the
interfaces it requires, but that is still not the case.

Given that the regression is more severe than the issue found by
syzkaller, the best approach is reverting the patch that causes the
regression, and trying to fix the underlying problem before checking
the endpoint types again.

Note that reverting this patch will probably trigger the syzkaller bug
at some point.

This reverts commit 2b9c3eb32a699acdd4784d6b93743271b4970899.

Link: https://syzkaller.appspot.com/bug?extid=348331f63b034f89b622 [1]
Link: https://lore.kernel.org/linux-input/87sf161jjc.wl-tiwai@suse.de/ [2]

Fixes: b516b1b0dfcc ("Revert "Input: bcm5974 - check endpoint type before starting traffic"")
Reported-by: Jacopo Radice <jacopo.radice@outlook.com>
Closes: https://bugzilla.suse.com/show_bug.cgi?id=1220030
Signed-off-by: Javier Carrasco <javier.carrasco.cruz@gmail.com>
---
Changes in v2:
- Add "Reported-by", "Closes" and "Link" tags.
- Use shorter lore link.
- Link to v1: https://lore.kernel.org/r/20240305-revert_bcm5974_ep_check-v1-1-db4f0422588f@gmail.com
---
 drivers/input/mouse/bcm5974.c | 20 --------------------
 1 file changed, 20 deletions(-)

diff --git a/drivers/input/mouse/bcm5974.c b/drivers/input/mouse/bcm5974.c
index 953992b458e9..ca150618d32f 100644
--- a/drivers/input/mouse/bcm5974.c
+++ b/drivers/input/mouse/bcm5974.c
@@ -19,7 +19,6 @@
  * Copyright (C) 2006	   Nicolas Boichat (nicolas@boichat.ch)
  */
 
-#include "linux/usb.h"
 #include <linux/kernel.h>
 #include <linux/errno.h>
 #include <linux/slab.h>
@@ -194,8 +193,6 @@ enum tp_type {
 
 /* list of device capability bits */
 #define HAS_INTEGRATED_BUTTON	1
-/* maximum number of supported endpoints (currently trackpad and button) */
-#define MAX_ENDPOINTS	2
 
 /* trackpad finger data block size */
 #define FSIZE_TYPE1		(14 * sizeof(__le16))
@@ -894,18 +891,6 @@ static int bcm5974_resume(struct usb_interface *iface)
 	return error;
 }
 
-static bool bcm5974_check_endpoints(struct usb_interface *iface,
-				    const struct bcm5974_config *cfg)
-{
-	u8 ep_addr[MAX_ENDPOINTS + 1] = {0};
-
-	ep_addr[0] = cfg->tp_ep;
-	if (cfg->tp_type == TYPE1)
-		ep_addr[1] = cfg->bt_ep;
-
-	return usb_check_int_endpoints(iface, ep_addr);
-}
-
 static int bcm5974_probe(struct usb_interface *iface,
 			 const struct usb_device_id *id)
 {
@@ -918,11 +903,6 @@ static int bcm5974_probe(struct usb_interface *iface,
 	/* find the product index */
 	cfg = bcm5974_get_config(udev);
 
-	if (!bcm5974_check_endpoints(iface, cfg)) {
-		dev_err(&iface->dev, "Unexpected non-int endpoint\n");
-		return -ENODEV;
-	}
-
 	/* allocate memory for our device state and initialize it */
 	dev = kzalloc(sizeof(struct bcm5974), GFP_KERNEL);
 	input_dev = input_allocate_device();

---
base-commit: 90d35da658da8cff0d4ecbb5113f5fac9d00eb72
change-id: 20240305-revert_bcm5974_ep_check-37f2a6ab2714

Best regards,
-- 
Javier Carrasco <javier.carrasco.cruz@gmail.com>

Re: [PATCH v2] Revert "Input: bcm5974 - check endpoint type before starting traffic"

[Dmitry Torokhov]

On March 4, 2024 10:38:33 PM PST, Javier Carrasco <javier.carrasco.cruz@gmail.com> wrote:
>This patch intended to fix an well-knonw issue in old drivers where the
>endpoint type is taken for granted, which is often triggered by fuzzers.
>
>That was the case for this driver [1], and although the fix seems to be
>correct, it uncovered another issue that leads to a regression [2], if
>the endpoints of the current interface are checked.
>
>The driver makes use of endpoints that belong to a different interface
>rather than the one it binds (it binds to the third interface, but also
>accesses an endpoint from a different one). The driver should claim the
>interfaces it requires, but that is still not the case.
>
>Given that the regression is more severe than the issue found by
>syzkaller, the best approach is reverting the patch that causes the
>regression, and trying to fix the underlying problem before checking
>the endpoint types again.
>
>Note that reverting this patch will probably trigger the syzkaller bug
>at some point.
>
>This reverts commit 2b9c3eb32a699acdd4784d6b93743271b4970899.
>
>Link: https://syzkaller.appspot.com/bug?extid=348331f63b034f89b622 [1]
>Link: https://lore.kernel.org/linux-input/87sf161jjc.wl-tiwai@suse.de/ [2]
>
>Fixes: b516b1b0dfcc ("Revert "Input: bcm5974 - check endpoint type before starting traffic"")


This "fixes" tag looks incorrect. The patch fixes itself?

>Reported-by: Jacopo Radice <jacopo.radice@outlook.com>
>Closes: https://bugzilla.suse.com/show_bug.cgi?id=1220030
>Signed-off-by: Javier Carrasco <javier.carrasco.cruz@gmail.com>
>---
>Changes in v2:
>- Add "Reported-by", "Closes" and "Link" tags.
>- Use shorter lore link.
>- Link to v1: https://lore.kernel.org/r/20240305-revert_bcm5974_ep_check-v1-1-db4f0422588f@gmail.com
>---
> drivers/input/mouse/bcm5974.c | 20 --------------------
> 1 file changed, 20 deletions(-)
>
>diff --git a/drivers/input/mouse/bcm5974.c b/drivers/input/mouse/bcm5974.c
>index 953992b458e9..ca150618d32f 100644
>--- a/drivers/input/mouse/bcm5974.c
>+++ b/drivers/input/mouse/bcm5974.c
>@@ -19,7 +19,6 @@
>  * Copyright (C) 2006	   Nicolas Boichat (nicolas@boichat.ch)
>  */
> 
>-#include "linux/usb.h"
> #include <linux/kernel.h>
> #include <linux/errno.h>
> #include <linux/slab.h>
>@@ -194,8 +193,6 @@ enum tp_type {
> 
> /* list of device capability bits */
> #define HAS_INTEGRATED_BUTTON	1
>-/* maximum number of supported endpoints (currently trackpad and button) */
>-#define MAX_ENDPOINTS	2
> 
> /* trackpad finger data block size */
> #define FSIZE_TYPE1		(14 * sizeof(__le16))
>@@ -894,18 +891,6 @@ static int bcm5974_resume(struct usb_interface *iface)
> 	return error;
> }
> 
>-static bool bcm5974_check_endpoints(struct usb_interface *iface,
>-				    const struct bcm5974_config *cfg)
>-{
>-	u8 ep_addr[MAX_ENDPOINTS + 1] = {0};
>-
>-	ep_addr[0] = cfg->tp_ep;
>-	if (cfg->tp_type == TYPE1)
>-		ep_addr[1] = cfg->bt_ep;
>-
>-	return usb_check_int_endpoints(iface, ep_addr);
>-}
>-
> static int bcm5974_probe(struct usb_interface *iface,
> 			 const struct usb_device_id *id)
> {
>@@ -918,11 +903,6 @@ static int bcm5974_probe(struct usb_interface *iface,
> 	/* find the product index */
> 	cfg = bcm5974_get_config(udev);
> 
>-	if (!bcm5974_check_endpoints(iface, cfg)) {
>-		dev_err(&iface->dev, "Unexpected non-int endpoint\n");
>-		return -ENODEV;
>-	}
>-
> 	/* allocate memory for our device state and initialize it */
> 	dev = kzalloc(sizeof(struct bcm5974), GFP_KERNEL);
> 	input_dev = input_allocate_device();
>
>---
>base-commit: 90d35da658da8cff0d4ecbb5113f5fac9d00eb72
>change-id: 20240305-revert_bcm5974_ep_check-37f2a6ab2714
>
>Best regards,


-- 
Dmitry