linux-kernel - Re: [PATCH v3] 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] [thread-next>] [day] [month] [year] [list]

Date:   Wed, 22 Nov 2023 13:48:56 +0100
From:   Javier Carrasco <javier.carrasco.cruz@...il.com>
To:     John Horan <knasher@...il.com>,
        Henrik Rydberg <rydberg@...math.org>,
        Dmitry Torokhov <dmitry.torokhov@...il.com>
Cc:     linux-input@...r.kernel.org, linux-kernel@...r.kernel.org,
        syzbot+348331f63b034f89b622@...kaller.appspotmail.com
Subject: Re: [PATCH v3] Input: bcm5974 - check endpoint type before starting
 traffic

On 14.10.23 12:20, Javier Carrasco wrote:
> syzbot has found a type mismatch between a USB pipe and the transfer
> endpoint, which is triggered by the bcm5974 driver[1].
> 
> This driver expects the device to provide input interrupt endpoints and
> if that is not the case, the driver registration should terminate.
> 
> Repros are available to reproduce this issue with a certain setup for
> the dummy_hcd, leading to an interrupt/bulk mismatch which is caught in
> the USB core after calling usb_submit_urb() with the following message:
> "BOGUS urb xfer, pipe 1 != type 3"
> 
> Some other device drivers (like the appletouch driver bcm5974 is mainly
> based on) provide some checking mechanism to make sure that an IN
> interrupt endpoint is available. In this particular case the endpoint
> addresses are provided by a config table, so the checking can be
> targeted to the provided endpoints.
> 
> Add some basic checking to guarantee that the endpoints available match
> the expected type for both the trackpad and button endpoints.
> 
> This issue was only found for the trackpad endpoint, but the checking
> has been added to the button endpoint as well for the same reasons.
> 
> Given that there was never a check for the endpoint type, this bug has
> been there since the first implementation of the driver (f89bd95c5c94).
> 
> [1] https://syzkaller.appspot.com/bug?extid=348331f63b034f89b622
> 
> Fixes: f89bd95c5c94 ("Input: bcm5974 - add driver for Macbook Air and Pro Penryn touchpads")
> Signed-off-by: Javier Carrasco <javier.carrasco.cruz@...il.com>
> Reported-and-tested-by: syzbot+348331f63b034f89b622@...kaller.appspotmail.com
> ---
> Changes in v3:
> - Use usb_check_int_endpoints() to validate the endpoints.
> - Link to v2: https://lore.kernel.org/r/20231007-topic-bcm5974_bulk-v2-1-021131c83efb@gmail.com
> 
> Changes in v2:
> - Keep error = -ENOMEM for the rest of the probe and return -ENODEV if
>   the endpoint check fails.
> - Check function returns now bool and was renamed (_is_ for
>   bool-returning functions).
> - Link to v1: https://lore.kernel.org/r/20231007-topic-bcm5974_bulk-v1-1-355be9f8ad80@gmail.com
> ---
>  drivers/input/mouse/bcm5974.c | 20 ++++++++++++++++++++
>  1 file changed, 20 insertions(+)
> 
> diff --git a/drivers/input/mouse/bcm5974.c b/drivers/input/mouse/bcm5974.c
> index ca150618d32f..953992b458e9 100644
> --- a/drivers/input/mouse/bcm5974.c
> +++ b/drivers/input/mouse/bcm5974.c
> @@ -19,6 +19,7 @@
>   * Copyright (C) 2006	   Nicolas Boichat (nicolas@...chat.ch)
>   */
>  
> +#include "linux/usb.h"
>  #include <linux/kernel.h>
>  #include <linux/errno.h>
>  #include <linux/slab.h>
> @@ -193,6 +194,8 @@ 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))
> @@ -891,6 +894,18 @@ 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)
>  {
> @@ -903,6 +918,11 @@ 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: 401644852d0b2a278811de38081be23f74b5bb04
> change-id: 20231007-topic-bcm5974_bulk-c66b743ba7ba
> 
> Best regards,

Gentle ping, syzbot keeps on reporting this bug (last report 7 days ago).

Thanks and best regards,
Javier Carrasco