[syzbot] [fbdev?] [usb?] WARNING in dlfb_submit_urb/usb_submit_urb (2)

[Helge Deller]

* Alan Stern <stern at rowland.harvard.edu>:
> On Thu, May 18, 2023 at 04:16:33PM +0200, Helge Deller wrote:
> > On 5/18/23 15:54, Alan Stern wrote:
> > > On Thu, May 18, 2023 at 09:34:24AM +0200, Helge Deller wrote:
> > > > I think this is an informational warning from the USB stack,
> > >
> > > It is not informational.  It is a warning that the caller has a bug.
> >
> > I'm not a USB expert, so I searched for such bug reports, and it seems
> > people sometimes faced this warning with different USB devices.
>
> Yes.
>
> > > You can't fix a bug by changing the line that reports it from dev_WARN
> > > to printk!
> >
> > Of course this patch wasn't intended as "fix".
> > It was intended to see how the udlfb driver behaves in this situation, e.g.
> > if the driver then crashes afterwards.
> >
> > Furthermore, why does usb_submit_urb() prints this WARNING and then continues?
> > If it's a real bug, why doesn't it returns an error instead?
> > So, in principle I still think this warning is kind of informational,
> > which of course points to some kind of problem which should be fixed.
>
> Depending on the situation, the bug may or may not lead to an error.  At
> the time the dev_WARN was added, we were less careful about these sorts
> of checks; I did not want to cause previously working devices to stop
> working by failing the URB submission.

Fair enough.

> > > In this case it looks like dlfb_usb_probe() or one of the routines it
> > > calls is wrong; it assumes that an endpoint has the expected type
> > > without checking.  More precisely, it thinks an endpoint is BULK when
> > > actually it is INTERRUPT.  That's what needs to be fixed.
> >
> > Maybe usb_submit_urb() should return an error so that drivers can
> > react on it, instead of adding the same kind of checks to all drivers?
>
> Feel free to submit a patch doing this.

As you wrote above, this may break other drivers too, so I'd leave that
discussion & decision to the USB maintainers (like you).

> But the checks should be added
> in any case; without them the drivers are simply wrong.

I pushed the hackish patch below through the syz tests which gives this log:
(see https://syzkaller.appspot.com/text?tag=CrashLog&x=160b7509280000)
[   77.559566][    T9] usb 1-1: Unable to get valid EDID from device/display
[   77.587021][    T9] WARNING: BOGUS urb xfer, pipe 3 != type 1 (fix driver to choose correct endpoint)
[   77.596448][    T9] usb 1-1: dlfb_urb_completion - nonzero write bulk status received: -115
[   77.605308][    T9] usb 1-1: submit urb error: -22
[   77.613225][    T9] udlfb: probe of 1-1:0.52 failed with error -22

So, basically there is no urgent fix needed for the dlfb fbdev driver,
as it will gracefully fail as is (which is correct).

What do you suggest we should do with this syzkaller-bug ?
I'd rate it as false-alarm, but it will continue to complain because of
the dev_WARN() in urb.c

Helge
---

From: Helge Deller <deller at gmx.de>
Date: Thu, 18 May 2023 19:03:56 +0200
Subject: [PATCH] fbdev: udlfb: check endpoint type, again

Temporary patch to anaylze syzbot regression:
https://syzkaller.appspot.com/bug?extid=0e22d63dcebb802b9bc8
It's not planned to apply as-is!

Fixes: aaf7dbe07385 ("video: fbdev: udlfb: properly check endpoint type")
Signed-off-by: Helge Deller <deller at gmx.de>

diff --git a/drivers/usb/core/urb.c b/drivers/usb/core/urb.c
index 9f3c54032556..bb889a1da3ef 100644
--- a/drivers/usb/core/urb.c
+++ b/drivers/usb/core/urb.c
@@ -500,9 +500,12 @@ int usb_submit_urb(struct urb *urb, gfp_t mem_flags)
 	 */

 	/* Check that the pipe's type matches the endpoint's type */
-	if (usb_pipe_type_check(urb->dev, urb->pipe))
-		dev_WARN(&dev->dev, "BOGUS urb xfer, pipe %x != type %x\n",
+	if (usb_pipe_type_check(urb->dev, urb->pipe)) {
+		/* temporarily use printk() instead of WARN() to fix bug in udlfb driver */
+		printk("WARNING: BOGUS urb xfer, pipe %x != type %x (fix driver to choose correct endpoint)\n",
 			usb_pipetype(urb->pipe), pipetypes[xfertype]);
+		return -EINVAL;
+	}

 	/* Check against a simple/standard policy */
 	allowed = (URB_NO_TRANSFER_DMA_MAP | URB_NO_INTERRUPT | URB_DIR_MASK |
diff --git a/drivers/video/fbdev/udlfb.c b/drivers/video/fbdev/udlfb.c
index 216d49c9d47e..5e56b2889c8c 100644
--- a/drivers/video/fbdev/udlfb.c
+++ b/drivers/video/fbdev/udlfb.c
@@ -1667,8 +1667,9 @@ static int dlfb_usb_probe(struct usb_interface *intf,
 	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");
+	if (retval || out == NULL) {
+		retval = -ENODEV;
+		dev_err(&intf->dev, "Device should have at least one bulk endpoint!\n");
 		goto error;
 	}