linux-kernel - Re: [PATCH] usb: gadget: ncm: Avoid dropping datagrams of properly parsed NTBs

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]

Date: Fri, 5 Jan 2024 10:11:53 +0530
From: Krishna Kurapati PSSNV <quic_kriskura@...cinc.com>
To: Jack Pham <quic_jackp@...cinc.com>,
        Maciej Żenczykowski
	<maze@...gle.com>
CC: Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Hardik Gajjar
	<hgajjar@...adit-jv.com>, <linux-usb@...r.kernel.org>,
        <linux-kernel@...r.kernel.org>, <quic_ppratap@...cinc.com>,
        <quic_wcheng@...cinc.com>
Subject: Re: [PATCH] usb: gadget: ncm: Avoid dropping datagrams of properly
 parsed NTBs



On 1/5/2024 2:48 AM, Jack Pham wrote:
> On Tue, Jan 02, 2024 at 05:04:01PM +0530, Krishna Kurapati PSSNV wrote:
>>
>>
>>>> The above might work. But just wanted to check why this 1 byte would
>>>> come actually ? Any reason for this ? ZLP must not give a 1 byte packet
>>>> of 1 byte AFAIK.
>>>
>>> I'm not a USB expert, but... my (possibly wrong) understanding is:
>>> (note I may be using bad terminology... also the 1024/16384 constants
>>> are USB3 specific, USB2 has afaik max 512 not 1024, I think USB1 is
>>> even 64, but it's likely too old to matter, etc.)
>>>
>>> USB3 payloads can be up to 16384 bytes in size,
>>> on the wire they are split up into packets of between 0 and 1024 bytes.
>>> [a Zero Length Packet is a ZLP]
>>> A usb payload is terminated with a usb packet of < 1024 bytes.
>>>
>>> So a 1524 byte payload would be sent as 2 packets 1024 + 500.
>>> While a 2048 byte payload would be sent as 3 packets 1024 + 1024 + 0 (ie. ZLP)
>>>
>>> A 16384 byte payload could be sent as 16 * 1024 + ZLP,
>>> but since 16384 is the max you might be able to get away with just 16
>>> * 1024 and skip the ZLP...
>>>
>>> I think this is why the Linux usb code base has ZLP / NO_ZLP quirks.
>>> [but do note I may be wrong, I haven't gone looking at what exactly
>>> the zlp quirks do,
>>> not even sure if they're receive or transmit side... or both]
>>>
>>> Different hardware/usb chipsets/etc have different behaviour wrt. ZLPs.
>>>
>>> In general it seems like what needs to happen is much clearer if you
>>> just avoid the need for ZLPs entirely.
>>> I think that's what windows is trying to do here: avoid ever sending a
>>> usb payload with a multiple of 1024 bytes,
>>> so it never has to send ZLPs. This seems easy enough to do...
>>> limit max to 16383 (not 16384) and add 1 byte of zero pad if the
>>> payload ends up being a multiple of 1024.
>>>
>>
>> Got it. Thanks for the explanation. Atleast this gives me an insight into
>> what might be the problem.
> 
> Hooray to MS for having open-sourced a reference version of their NCM
> driver on GitHub (under MIT license)--and I think this might explain it:
> 
> https://github.com/microsoft/NCM-Driver-for-Windows/blob/release_21H2/host/device.cpp#L902
> 
> which states in a comment (pasted line-wrapped for mail-friendliness)
> 
>          //NCM spec is not explicit if a ZLP shall be sent when
>          //wBlockLength != 0 and it happens to be
>          //multiple of wMaxPacketSize. Our interpretation is that no ZLP
>          //needed if wBlockLength is non-zero,
>          //because the non-zero wBlockLength has already told the
>          //function side the size of transfer to be expected.
>          //
>          //However, there are in-market NCM devices rely on ZLP as long
>          //as the wBlockLength is multiple of wMaxPacketSize.
>          //To deal with such devices, we pad an extra 0 at end so the
>          //transfer is no longer multiple of wMaxPacketSize
> 
> If so then would be worth calling this out in commit text and/or code
> comment.

Thanks for the inputs Jack. Will make sure to add it in commit text clearly.

Regards,
Krishna,