@@ -501,7 +501,7 @@ 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",+printk("BOGUS urb xfer, pipe %x != type %x (hardware misbehaviour?)\n",usb_pipetype(urb->pipe),pipetypes[xfertype]);/* Check against a simple/standard policy */
@@ -501,7 +501,7 @@ 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",+printk("BOGUS urb xfer, pipe %x != type %x (hardware misbehaviour?)\n",usb_pipetype(urb->pipe),pipetypes[xfertype]);/* Check against a simple/standard policy */
You can't fix a bug by changing the line that reports it from dev_WARN
to printk!
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.
Alan Stern
@@ -501,7 +501,7 @@ 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",+printk("BOGUS urb xfer, pipe %x != type %x (hardware misbehaviour?)\n",usb_pipetype(urb->pipe),pipetypes[xfertype]);/* Check against a simple/standard policy */
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.
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?
Helge
From: Alan Stern <stern@rowland.harvard.edu> Date: 2023-05-18 15:06:07
On Thu, May 18, 2023 at 04:16:33PM +0200, Helge Deller wrote:
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
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.
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. But the checks should be added
in any case; without them the drivers are simply wrong.
Alan Stern
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>
@@ -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|
@@ -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");gotoerror;}
From: Alan Stern <stern@rowland.harvard.edu> Date: 2023-05-18 20:36:10
On Thu, May 18, 2023 at 09:06:12PM +0200, Helge Deller wrote:
* Alan Stern [off-list ref]:
quoted
On Thu, May 18, 2023 at 04:16:33PM +0200, Helge Deller wrote:
quoted
On 5/18/23 15:54, Alan Stern wrote:
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).
quoted
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.
Alan Stern
#syz test: https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/usb.git a4422ff22142
Index: usb-devel/drivers/video/fbdev/udlfb.c
===================================================================
@@ -1652,7 +1652,7 @@ static int dlfb_usb_probe(struct usb_intstructfb_info*info;intretval;structusb_device*usbdev=interface_to_usbdev(intf);-structusb_endpoint_descriptor*out;+staticu8out_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_intdlfb->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;gotoerror;}
On Thu, May 18, 2023 at 09:06:12PM +0200, Helge Deller wrote:
quoted
* Alan Stern [off-list ref]:
quoted
On Thu, May 18, 2023 at 04:16:33PM +0200, Helge Deller wrote:
quoted
On 5/18/23 15:54, Alan Stern wrote:
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).
quoted
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
@@ -1652,7 +1652,7 @@ static int dlfb_usb_probe(struct usb_intstructfb_info*info;intretval;structusb_device*usbdev=interface_to_usbdev(intf);-structusb_endpoint_descriptor*out;+staticu8out_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_intdlfb->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;gotoerror;}
From: Alan Stern <stern@rowland.harvard.edu> Date: 2023-05-19 15:42:58
On Fri, May 19, 2023 at 12:38:15PM +0200, Helge Deller wrote:
Patch looks good and survived the test.
Will you send a proper patch to the fbdev mailing list, so that I can
include it?
Will do.
While you're working on this driver, here's a suggestion for another
improvement you can make. The temporary buffer allocations and calls to
usb_control_msg() in dlfb_get_edid() and dlfb_select_std_channel() can
be replaced with calls to usb_control_msg_recv() and
usb_control_msg_send() respectively.
Alan Stern
On Fri, May 19, 2023 at 12:38:15PM +0200, Helge Deller wrote:
quoted
Patch looks good and survived the test.
Will you send a proper patch to the fbdev mailing list, so that I can
include it?
Will do.
Great! Thanks!
While you're working on this driver,
I'm not working on that driver. Just looked into it because of this
sysbot issue. I even don't have that hardware to test.
here's a suggestion for another
improvement you can make. The temporary buffer allocations and calls to
usb_control_msg() in dlfb_get_edid() and dlfb_select_std_channel() can
be replaced with calls to usb_control_msg_recv() and
usb_control_msg_send() respectively.
From: Alan Stern <stern@rowland.harvard.edu> Date: 2023-05-19 19:32:35
The syzbot fuzzer detected a problem in the udlfb driver, caused by an
endpoint not having the expected type:
usb 1-1: Read EDID byte 0 failed: -71
usb 1-1: Unable to get valid EDID from device/display
------------[ cut here ]------------
usb 1-1: BOGUS urb xfer, pipe 3 != type 1
WARNING: CPU: 0 PID: 9 at drivers/usb/core/urb.c:504 usb_submit_urb+0xed6/0x1880
drivers/usb/core/urb.c:504
Modules linked in:
CPU: 0 PID: 9 Comm: kworker/0:1 Not tainted
6.4.0-rc1-syzkaller-00016-ga4422ff22142 #0
Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google
04/28/2023
Workqueue: usb_hub_wq hub_event
RIP: 0010:usb_submit_urb+0xed6/0x1880 drivers/usb/core/urb.c:504
...
Call Trace:
<TASK>
dlfb_submit_urb+0x92/0x180 drivers/video/fbdev/udlfb.c:1980
dlfb_set_video_mode+0x21f0/0x2950 drivers/video/fbdev/udlfb.c:315
dlfb_ops_set_par+0x2a7/0x8d0 drivers/video/fbdev/udlfb.c:1111
dlfb_usb_probe+0x149a/0x2710 drivers/video/fbdev/udlfb.c:1743
The current approach for this issue failed to catch the problem
because it only checks for the existence of a bulk-OUT endpoint; it
doesn't check whether this endpoint is the one that the driver will
actually use.
We can fix the problem by instead checking that the endpoint used by
the driver does exist and is bulk-OUT.
Reported-and-tested-by: syzbot+0e22d63dcebb802b9bc8@syzkaller.appspotmail.com
Signed-off-by: Alan Stern <stern@rowland.harvard.edu>
CC: Pavel Skripkin <redacted>
Fixes: aaf7dbe07385 ("video: fbdev: udlfb: properly check endpoint type")
CC: <redacted>
---
drivers/video/fbdev/udlfb.c | 13 ++++++++-----
1 file changed, 8 insertions(+), 5 deletions(-)
Index: usb-devel/drivers/video/fbdev/udlfb.c
===================================================================
@@ -27,6 +27,8 @@#include<video/udlfb.h>#include"edid.h"+#define OUT_EP_NUM 1 /* The endpoint number we will use */+staticconststructfb_fix_screeninfodlfb_fix={.id="udlfb",.type=FB_TYPE_PACKED_PIXELS,
@@ -1652,7 +1654,7 @@ static int dlfb_usb_probe(struct usb_intstructfb_info*info;intretval;structusb_device*usbdev=interface_to_usbdev(intf);-structusb_endpoint_descriptor*out;+staticu8out_ep[]={OUT_EP_NUM+USB_DIR_OUT,0};/* usb initialization */dlfb=kzalloc(sizeof(*dlfb),GFP_KERNEL);
@@ -1666,9 +1668,9 @@ static int dlfb_usb_probe(struct usb_intdlfb->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;gotoerror;}
@@ -1927,7 +1929,8 @@ retry:}/* urb->transfer_buffer_length set to actual before submit */-usb_fill_bulk_urb(urb,dlfb->udev,usb_sndbulkpipe(dlfb->udev,1),+usb_fill_bulk_urb(urb,dlfb->udev,+usb_sndbulkpipe(dlfb->udev,OUT_EP_NUM),buf,size,dlfb_urb_completion,unode);urb->transfer_flags|=URB_NO_TRANSFER_DMA_MAP;
The syzbot fuzzer detected a problem in the udlfb driver, caused by an
endpoint not having the expected type:
usb 1-1: Read EDID byte 0 failed: -71
usb 1-1: Unable to get valid EDID from device/display
------------[ cut here ]------------
usb 1-1: BOGUS urb xfer, pipe 3 != type 1
WARNING: CPU: 0 PID: 9 at drivers/usb/core/urb.c:504 usb_submit_urb+0xed6/0x1880
drivers/usb/core/urb.c:504
Modules linked in:
CPU: 0 PID: 9 Comm: kworker/0:1 Not tainted
6.4.0-rc1-syzkaller-00016-ga4422ff22142 #0
Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google
04/28/2023
Workqueue: usb_hub_wq hub_event
RIP: 0010:usb_submit_urb+0xed6/0x1880 drivers/usb/core/urb.c:504
...
Call Trace:
<TASK>
dlfb_submit_urb+0x92/0x180 drivers/video/fbdev/udlfb.c:1980
dlfb_set_video_mode+0x21f0/0x2950 drivers/video/fbdev/udlfb.c:315
dlfb_ops_set_par+0x2a7/0x8d0 drivers/video/fbdev/udlfb.c:1111
dlfb_usb_probe+0x149a/0x2710 drivers/video/fbdev/udlfb.c:1743
The current approach for this issue failed to catch the problem
because it only checks for the existence of a bulk-OUT endpoint; it
doesn't check whether this endpoint is the one that the driver will
actually use.
We can fix the problem by instead checking that the endpoint used by
the driver does exist and is bulk-OUT.
Reported-and-tested-by: syzbot+0e22d63dcebb802b9bc8@syzkaller.appspotmail.com
Signed-off-by: Alan Stern <stern@rowland.harvard.edu>
CC: Pavel Skripkin <redacted>
Fixes: aaf7dbe07385 ("video: fbdev: udlfb: properly check endpoint type")
CC: <redacted>
@@ -27,6 +27,8 @@#include<video/udlfb.h>#include"edid.h"+#define OUT_EP_NUM 1 /* The endpoint number we will use */+staticconststructfb_fix_screeninfodlfb_fix={.id="udlfb",.type=FB_TYPE_PACKED_PIXELS,
@@ -1652,7 +1654,7 @@ static int dlfb_usb_probe(struct usb_intstructfb_info*info;intretval;structusb_device*usbdev=interface_to_usbdev(intf);-structusb_endpoint_descriptor*out;+staticu8out_ep[]={OUT_EP_NUM+USB_DIR_OUT,0};/* usb initialization */dlfb=kzalloc(sizeof(*dlfb),GFP_KERNEL);
@@ -1666,9 +1668,9 @@ static int dlfb_usb_probe(struct usb_intdlfb->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;gotoerror;}
@@ -1927,7 +1929,8 @@ retry:}/* urb->transfer_buffer_length set to actual before submit */-usb_fill_bulk_urb(urb,dlfb->udev,usb_sndbulkpipe(dlfb->udev,1),+usb_fill_bulk_urb(urb,dlfb->udev,+usb_sndbulkpipe(dlfb->udev,OUT_EP_NUM),buf,size,dlfb_urb_completion,unode);urb->transfer_flags|=URB_NO_TRANSFER_DMA_MAP;