linux-kernel - Re: [syzbot] [fbdev?] [usb?] WARNING in dlfb_submit_urb/usb_submit

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <6e93305a-2d70-d411-3e36-c536449295dd@gmx.de>
Date:   Fri, 19 May 2023 12:38:15 +0200
From:   Helge Deller <deller@....de>
To:     Alan Stern <stern@...land.harvard.edu>
Cc:     linux-kernel@...r.kernel.org, linux-fbdev@...r.kernel.org,
        dri-devel@...ts.freedesktop.org,
        syzbot <syzbot+0e22d63dcebb802b9bc8@...kaller.appspotmail.com>,
        bernie@...gable.com, linux-usb@...r.kernel.org,
        syzkaller-bugs@...glegroups.com
Subject: Re: [syzbot] [fbdev?] [usb?] WARNING in
 dlfb_submit_urb/usb_submit_urb (2)

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@...land.harvard.edu>:
>>> 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;
>   	}
>