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@xxxxxxxxx> > Reported-and-tested-by: syzbot+348331f63b034f89b622@xxxxxxxxxxxxxxxxxxxxxxxxx > --- > 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@xxxxxxxxx > > 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@xxxxxxxxx > --- > 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 ping, syzbot keeps on reporting this bug (last report 7 days ago). Thanks and best regards, Javier Carrasco
