On 5/18/23 22:35, Alan Stern wrote:
On Thu, May 18, 2023 at 09:06:12PM +0200, Helge Deller wrote:
* Alan Stern <stern@xxxxxxxxxxxxxxxxxxx>:
On Thu, May 18, 2023 at 04:16:33PM +0200, Helge Deller wrote:
On 5/18/23 15:54, Alan Stern wrote:
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
Let's try this patch instead. It might contain a stupid error because I
haven't even tried to compile it, but it ought to fix the real problem.
Patch looks good and survived the test.
Will you send a proper patch to the fbdev mailing list, so that I can
include it?
Helge
#syz test: https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/usb.git a4422ff22142
Index: usb-devel/drivers/video/fbdev/udlfb.c
===================================================================
--- usb-devel.orig/drivers/video/fbdev/udlfb.c
+++ usb-devel/drivers/video/fbdev/udlfb.c
@@ -1652,7 +1652,7 @@ static int dlfb_usb_probe(struct usb_int
struct fb_info *info;
int retval;
struct usb_device *usbdev = interface_to_usbdev(intf);
- struct usb_endpoint_descriptor *out;
+ static u8 out_ep[] = {1 + USB_DIR_OUT, 0};
/* usb initialization */
dlfb = kzalloc(sizeof(*dlfb), GFP_KERNEL);
@@ -1666,9 +1666,9 @@ static int dlfb_usb_probe(struct usb_int
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");
+ if (!usb_check_bulk_endpoints(intf, out_ep)) {
+ dev_err(&intf->dev, "Invalid DisplayLink device!\n");
+ retval = -EINVAL;
goto error;
}
