Re: [PATCH] fbdev: udlfb: properly check endpoint type

[Helge Deller <deller@gmx.de>]

On 3/22/22 21:04, Pavel Skripkin wrote:
> syzbot reported warning in usb_submit_urb, which is caused by wrong
> endpoint type.
>
> This driver uses out bulk endpoint for communication, so
> let's check if this endpoint is present and bail out early if not.
>
> Fail log:
>
> usb 1-1: BOGUS urb xfer, pipe 3 != type 1
> WARNING: CPU: 0 PID: 4822 at drivers/usb/core/urb.c:493 usb_submit_urb+0xd27/0x1540 drivers/usb/core/urb.c:493
> Modules linked in:
> CPU: 0 PID: 4822 Comm: kworker/0:3 Tainted: G        W         5.13.0-syzkaller #0
> ...
> Workqueue: usb_hub_wq hub_event
> RIP: 0010:usb_submit_urb+0xd27/0x1540 drivers/usb/core/urb.c:493
> ...
> Call Trace:
>  dlfb_submit_urb+0x89/0x160 drivers/video/fbdev/udlfb.c:1969
>  dlfb_set_video_mode+0x21f0/0x2950 drivers/video/fbdev/udlfb.c:315
>  dlfb_ops_set_par+0x2a3/0x840 drivers/video/fbdev/udlfb.c:1110
>  dlfb_usb_probe.cold+0x113e/0x1f4a drivers/video/fbdev/udlfb.c:1732
>  usb_probe_interface+0x315/0x7f0 drivers/usb/core/driver.c:396
>
> Fixes: 88e58b1a42f8 ("Staging: add udlfb driver")
> Reported-and-tested-by: syzbot+53ce4a4246d0fe0fee34@syzkaller.appspotmail.com
> Signed-off-by: Pavel Skripkin <paskripkin@gmail.com>

applied.

Thanks,
Helge

> ---
>  drivers/video/fbdev/udlfb.c | 14 ++++++++++++--
>  1 file changed, 12 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/video/fbdev/udlfb.c b/drivers/video/fbdev/udlfb.c
> index b9cdd02c1000..2343c7955747 100644
> --- a/drivers/video/fbdev/udlfb.c
> +++ b/drivers/video/fbdev/udlfb.c
> @@ -1649,8 +1649,9 @@ static int dlfb_usb_probe(struct usb_interface *intf,
>  	const struct device_attribute *attr;
>  	struct dlfb_data *dlfb;
>  	struct fb_info *info;
> -	int retval = -ENOMEM;
> +	int retval;
>  	struct usb_device *usbdev = interface_to_usbdev(intf);
> +	struct usb_endpoint_descriptor *out;
>
>  	/* usb initialization */
>  	dlfb = kzalloc(sizeof(*dlfb), GFP_KERNEL);
> @@ -1664,6 +1665,12 @@ static int dlfb_usb_probe(struct usb_interface *intf,
>  	dlfb->udev = usb_get_dev(usbdev);
>  	usb_set_intfdata(intf, dlfb);
>
> +	retval = usb_find_common_endpoints(intf->cur_altsetting, NULL, &out, NULL, NULL);
> +	if (retval) {
> +		dev_err(&intf->dev, "Device should have at lease 1 bulk endpoint!\n");
> +		goto error;
> +	}
> +
>  	dev_dbg(&intf->dev, "console enable=%d\n", console);
>  	dev_dbg(&intf->dev, "fb_defio enable=%d\n", fb_defio);
>  	dev_dbg(&intf->dev, "shadow enable=%d\n", shadow);
> @@ -1673,6 +1680,7 @@ static int dlfb_usb_probe(struct usb_interface *intf,
>  	if (!dlfb_parse_vendor_descriptor(dlfb, intf)) {
>  		dev_err(&intf->dev,
>  			"firmware not recognized, incompatible device?\n");
> +		retval = -ENODEV;
>  		goto error;
>  	}
>
> @@ -1686,8 +1694,10 @@ static int dlfb_usb_probe(struct usb_interface *intf,
>
>  	/* allocates framebuffer driver structure, not framebuffer memory */
>  	info = framebuffer_alloc(0, &dlfb->udev->dev);
> -	if (!info)
> +	if (!info) {
> +		retval = -ENOMEM;
>  		goto error;
> +	}
>
>  	dlfb->info = info;
>  	info->par = dlfb;