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

* Alan Stern [off-list ref]:

On Thu, May 18, 2023 at 04:16:33PM +0200, Helge Deller wrote:

quoted

On 5/18/23 15:54, Alan Stern wrote:

quoted

On Thu, May 18, 2023 at 09:34:24AM +0200, Helge Deller wrote:

quoted

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.

quoted

quoted

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.

quoted

quoted

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@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@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;
 	}