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

Message-ID: <20240104211858.GB4127689@hu-jackp-lv.qualcomm.com>
Date: Thu, 4 Jan 2024 13:18:58 -0800
From: Jack Pham <quic_jackp@...cinc.com>
To: Krishna Kurapati PSSNV <quic_kriskura@...cinc.com>
CC: Maciej Żenczykowski <maze@...gle.com>,
        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 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.

Jack