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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <04E6C7D8-395C-4DB3-B331-B79215DE6B40@gmail.com>
Date: Mon, 04 Mar 2024 23:41:25 -0800
From: Dmitry Torokhov <dmitry.torokhov@...il.com>
To: Javier Carrasco <javier.carrasco.cruz@...il.com>,
 Henrik Rydberg <rydberg@...math.org>
CC: linux-input@...r.kernel.org, linux-kernel@...r.kernel.org,
 regressions@...ts.linux.dev, Jacopo Radice <jacopo.radice@...look.com>
Subject: Re: [PATCH v2] Revert "Input: bcm5974 - check endpoint type before starting traffic"

On March 4, 2024 10:38:33 PM PST, Javier Carrasco <javier.carrasco.cruz@...il.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@...look.com>
>Closes: https://bugzilla.suse.com/show_bug.cgi?id=1220030
>Signed-off-by: Javier Carrasco <javier.carrasco.cruz@...il.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@...chat.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