diff mbox series

[v3] Input: bcm5974 - check endpoint type before starting traffic

Message ID 20231007-topic-bcm5974_bulk-v3-1-d0f38b9d2935@gmail.com (mailing list archive)
State New
Headers show
Series [v3] Input: bcm5974 - check endpoint type before starting traffic | expand

Commit Message

Javier Carrasco Oct. 14, 2023, 10:20 a.m. UTC 
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@gmail.com>
Reported-and-tested-by: syzbot+348331f63b034f89b622@syzkaller.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(+)


---
base-commit: 401644852d0b2a278811de38081be23f74b5bb04
change-id: 20231007-topic-bcm5974_bulk-c66b743ba7ba

Best regards,

Comments

Javier Carrasco Nov. 22, 2023, 12:48 p.m. UTC | #1 
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@gmail.com>
> Reported-and-tested-by: syzbot+348331f63b034f89b622@syzkaller.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@boichat.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
Javier Carrasco Jan. 30, 2024, 10:11 p.m. UTC | #2 
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://eur04.safelinks.protection.outlook.com/?url=https%3A%2F%2Fsyzkaller.appspot.com%2Fbug%3Fextid%3D348331f63b034f89b622&data=05%7C01%7Cjavier.carrasco%40wolfvision.net%7C1880f48b0ac1493b40ff08dbcc9f2ea8%7Ce94ec9da9183471e83b351baa8eb804f%7C1%7C0%7C638328756279240780%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=IOsiHpWoIkog8HHkYIY8Ljh762bPZiqgm5xd5oAbK3s%3D&reserved=0
> 
> Fixes: f89bd95c5c94 ("Input: bcm5974 - add driver for Macbook Air and Pro Penryn touchpads")
> Signed-off-by: Javier Carrasco <javier.carrasco.cruz@gmail.com>
> Reported-and-tested-by: syzbot+348331f63b034f89b622@syzkaller.appspotmail.com
> ---
> Changes in v3:
> - Use usb_check_int_endpoints() to validate the endpoints.
> - Link to v2: https://eur04.safelinks.protection.outlook.com/?url=https%3A%2F%2Flore.kernel.org%2Fr%2F20231007-topic-bcm5974_bulk-v2-1-021131c83efb%40gmail.com&data=05%7C01%7Cjavier.carrasco%40wolfvision.net%7C1880f48b0ac1493b40ff08dbcc9f2ea8%7Ce94ec9da9183471e83b351baa8eb804f%7C1%7C0%7C638328756279240780%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=vqLE9mUP0ehBIgtI%2F52ONRsF1wOrikc0VVLrp6MMjqQ%3D&reserved=0
> 
> 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://eur04.safelinks.protection.outlook.com/?url=https%3A%2F%2Flore.kernel.org%2Fr%2F20231007-topic-bcm5974_bulk-v1-1-355be9f8ad80%40gmail.com&data=05%7C01%7Cjavier.carrasco%40wolfvision.net%7C1880f48b0ac1493b40ff08dbcc9f2ea8%7Ce94ec9da9183471e83b351baa8eb804f%7C1%7C0%7C638328756279240780%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=Qf6kg4M2AvwSEpkwClGPpVdo1PO96WfUfTsiy6z28UI%3D&reserved=0
> ---
>  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@boichat.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 reminder: this bug keeps on being found by syzbot and it was
included in the last monthly input report (Jan 2024):

https://lore.kernel.org/all/0000000000001df937060f20c585@google.com/T/

Best regards,
Javier Carrasco
diff mbox series

Patch

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@boichat.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();