linux-kernel - Re: [REGRESSION][BISECTED] flood of "hid-generic ... control queue full" since v5.14-rc1

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] [day] [month] [year] [list]

Message-ID: <97d5cdb9-0372-7e8b-dc61-610198f8664f@applied-asynchrony.com>
Date:   Mon, 30 Aug 2021 21:23:22 +0200
From:   Holger Hoffstätte <holger@...lied-asynchrony.com>
To:     Michal Kubecek <mkubecek@...e.cz>,
        Alan Stern <stern@...land.harvard.edu>
Cc:     Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        linux-usb@...r.kernel.org, linux-input@...r.kernel.org,
        linux-kernel@...r.kernel.org, Jiri Kosina <jikos@...nel.org>,
        Benjamin Tissoires <benjamin.tissoires@...hat.com>
Subject: Re: [REGRESSION][BISECTED] flood of "hid-generic ... control queue
 full" since v5.14-rc1

On 2021-08-16 21:12, Michal Kubecek wrote:
> On Mon, Aug 16, 2021 at 10:38:56AM -0400, Alan Stern wrote:
>> On Mon, Aug 16, 2021 at 04:13:47PM +0200, Michal Kubecek wrote:
>>> Looking at the code, the primary problem seems to be that the "else"
>>> branch in hid_submit_ctrl() recalculates transfer_buffer_length to
>>> a rounded up value but assigns the original length to wLength.
>>
>> Looks like you found the bug.  Fixing it might be as simple as setting
>> len = padlen in that "else" branch.  You could then combine the
>> transfer_buffer_length assignment with the one in the "if" branch and
>> hoist them out after the entire "if" statement.
> 
> With the patch below, there are no errors and the UPS communication
> works correctly and so do other HID devices. But I would prefere someone
> familiar with HID code to confirm that this is what we want and what
> would be the right way to handle usb_submit_urb() errors.
> 
> Michal
> 
> diff --git a/drivers/hid/usbhid/hid-core.c b/drivers/hid/usbhid/hid-core.c
> index 06130dc431a0..ef240ef63a66 100644
> --- a/drivers/hid/usbhid/hid-core.c
> +++ b/drivers/hid/usbhid/hid-core.c
> @@ -377,27 +377,26 @@ static int hid_submit_ctrl(struct hid_device *hid)
>   	len = hid_report_len(report);
>   	if (dir == USB_DIR_OUT) {
>   		usbhid->urbctrl->pipe = usb_sndctrlpipe(hid_to_usb_dev(hid), 0);
> -		usbhid->urbctrl->transfer_buffer_length = len;
>   		if (raw_report) {
>   			memcpy(usbhid->ctrlbuf, raw_report, len);
>   			kfree(raw_report);
>   			usbhid->ctrl[usbhid->ctrltail].raw_report = NULL;
>   		}
>   	} else {
> -		int maxpacket, padlen;
> +		int maxpacket;
>   
>   		usbhid->urbctrl->pipe = usb_rcvctrlpipe(hid_to_usb_dev(hid), 0);
>   		maxpacket = usb_maxpacket(hid_to_usb_dev(hid),
>   					  usbhid->urbctrl->pipe, 0);
>   		if (maxpacket > 0) {
> -			padlen = DIV_ROUND_UP(len, maxpacket);
> -			padlen *= maxpacket;
> -			if (padlen > usbhid->bufsize)
> -				padlen = usbhid->bufsize;
> +			len = DIV_ROUND_UP(len, maxpacket);
> +			len *= maxpacket;
> +			if (len > usbhid->bufsize)
> +				len = usbhid->bufsize;
>   		} else
> -			padlen = 0;
> -		usbhid->urbctrl->transfer_buffer_length = padlen;
> +			len = 0;
>   	}
> +	usbhid->urbctrl->transfer_buffer_length = len;
>   	usbhid->urbctrl->dev = hid_to_usb_dev(hid);
>   
>   	usbhid->cr->bRequestType = USB_TYPE_CLASS | USB_RECIP_INTERFACE | dir;
> 

Got the same flood of messages when trying 5.14.0, found this thread, applied
the patch & rebuilt, problem is gone & UPS still working fine. Yay.

cheers
Holger