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@xxxxxxxxx> 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@xxxxxxx/ [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@xxxxxxxxxxx>
>Closes: https://bugzilla.suse.com/show_bug.cgi?id=1220030
>Signed-off-by: Javier Carrasco <javier.carrasco.cruz@xxxxxxxxx>
>---
>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@xxxxxxxxx
>---
> 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@xxxxxxxxxx)
>  */
> 
>-#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