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@xxxxxxxxx> > Reported-and-tested-by: syzbot+348331f63b034f89b622@xxxxxxxxxxxxxxxxxxxxxxxxx > --- > 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@xxxxxxxxxx) > */ > > +#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@xxxxxxxxxx/T/ Best regards, Javier Carrasco
