| 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
| ||
|
Message-ID: <CANP3RGebjQhjCMVg5h936kp2mcCFcRvwzwi+84vxzFeUnmQwCQ@mail.gmail.com>
Date: Wed, 31 Jan 2024 09:18:35 -0800
From: Maciej Żenczykowski <maze@...gle.com>
To: Krishna Kurapati <quic_kriskura@...cinc.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, quic_jackp@...cinc.com
Subject: Re: [PATCH v2] usb: gadget: ncm: Avoid dropping datagrams of properly
parsed NTBs
On Wed, Jan 31, 2024 at 9:14 AM Maciej Żenczykowski <maze@...gle.com> wrote:
>
> On Wed, Jan 31, 2024 at 9:03 AM Maciej Żenczykowski <maze@...gle.com> wrote:
> >
> > On Wed, Jan 31, 2024 at 7:03 AM Krishna Kurapati
> > <quic_kriskura@...cinc.com> wrote:
> > >
> > > It is observed sometimes when tethering is used over NCM with Windows 11
> > > as host, at some instances, the gadget_giveback has one byte appended at
> > > the end of a proper NTB. When the NTB is parsed, unwrap call looks for
> > > any leftover bytes in SKB provided by u_ether and if there are any pending
> > > bytes, it treats them as a separate NTB and parses it. But in case the
> > > second NTB (as per unwrap call) is faulty/corrupt, all the datagrams that
> > > were parsed properly in the first NTB and saved in rx_list are dropped.
> > >
> > > Adding a few custom traces showed the following:
> > >
> > > [002] d..1 7828.532866: dwc3_gadget_giveback: ep1out:
> > > req 000000003868811a length 1025/16384 zsI ==> 0
> > > [002] d..1 7828.532867: ncm_unwrap_ntb: K: ncm_unwrap_ntb toprocess: 1025
> > > [002] d..1 7828.532867: ncm_unwrap_ntb: K: ncm_unwrap_ntb nth: 1751999342
> > > [002] d..1 7828.532868: ncm_unwrap_ntb: K: ncm_unwrap_ntb seq: 0xce67
> > > [002] d..1 7828.532868: ncm_unwrap_ntb: K: ncm_unwrap_ntb blk_len: 0x400
> > > [002] d..1 7828.532868: ncm_unwrap_ntb: K: ncm_unwrap_ntb ndp_len: 0x10
> > > [002] d..1 7828.532869: ncm_unwrap_ntb: K: Parsed NTB with 1 frames
> > >
> > > In this case, the giveback is of 1025 bytes and block length is 1024.
> > > The rest 1 byte (which is 0x00) won't be parsed resulting in drop of
> > > all datagrams in rx_list.
> > >
> > > Same is case with packets of size 2048:
> > > [002] d..1 7828.557948: dwc3_gadget_giveback: ep1out:
> > > req 0000000011dfd96e length 2049/16384 zsI ==> 0
> > > [002] d..1 7828.557949: ncm_unwrap_ntb: K: ncm_unwrap_ntb nth: 1751999342
> > > [002] d..1 7828.557950: ncm_unwrap_ntb: K: ncm_unwrap_ntb blk_len: 0x800
> > >
> > > Lecroy shows one byte coming in extra confirming that the byte is coming
> > > in from PC:
> > >
> > > Transfer 2959 - Bytes Transferred(1025) Timestamp((18.524 843 590)
> > > - Transaction 8391 - Data(1025 bytes) Timestamp(18.524 843 590)
> > > --- Packet 4063861
> > > Data(1024 bytes)
> > > Duration(2.117us) Idle(14.700ns) Timestamp(18.524 843 590)
> > > --- Packet 4063863
> > > Data(1 byte)
> > > Duration(66.160ns) Time(282.000ns) Timestamp(18.524 845 722)
> > >
> > > According to Windows driver, no ZLP is 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
> > > that rely on ZLP as long as the wBlockLength is multiple of wMaxPacketSize.
> > > To deal with such devices, it pads an extra 0 at end so the transfer is no
> > > longer multiple of wMaxPacketSize.
> > >
> > > Signed-off-by: Krishna Kurapati <quic_kriskura@...cinc.com>
> > > ---
> > > drivers/usb/gadget/function/f_ncm.c | 8 +++++++-
> > > 1 file changed, 7 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/usb/gadget/function/f_ncm.c b/drivers/usb/gadget/function/f_ncm.c
> > > index ca5d5f564998..8c314dc98952 100644
> > > --- a/drivers/usb/gadget/function/f_ncm.c
> > > +++ b/drivers/usb/gadget/function/f_ncm.c
> > > @@ -1338,11 +1338,17 @@ static int ncm_unwrap_ntb(struct gether *port,
> > > "Parsed NTB with %d frames\n", dgram_counter);
> > >
> > > to_process -= block_len;
> > > - if (to_process != 0) {
> > > +
> > > + if (to_process == 1 &&
> > > + (block_len % 512 == 0) &&
> > > + (*(unsigned char *)(ntb_ptr + block_len) == 0x00)) {
> >
> > I'm unconvinced this (block_len % 512) works right...
> > imagine you have 2 ntbs back to back (for example 400 + 624) that
> > together add up to 1024,
> > and then a padding null byte.
> > I think block_len will be 624 then and not 1024.
> >
> > perhaps this actually needs to be:
> > (ntp_ptr + block_len - skb->data) % 512 == 0
> >
> > The point is that AFAICT the multiple of 512/1024 that matters is in
> > the size of the USB transfer,
> > not the NTB block size itself.
> >
> > > + goto done;
> > > + } else if (to_process > 0) {
> > > ntb_ptr = (unsigned char *)(ntb_ptr + block_len);
> >
> > note that ntb_ptr moves forward by block_len here,
> > so it's *not* always the start of the packet, so block_len is not
> > necessarily the actual on the wire size.
> >
> > > goto parse_ntb;
> > > }
> > >
> > > +done:
> > > dev_consume_skb_any(skb);
> > >
> > > return 0;
> > > --
> > > 2.34.1
> > >
> >
> > But it would perhaps be worth confirming in the MS driver source what
> > exactly they're doing...
> > (maybe they never even generate multiple ntbs per usb segment...)
> >
> > You may also want to mention in the commit message that 512 comes from
> > USB2 block size, and also works for USB3 block size of 1024.
>
> Also since this is a fix, it should probably have a Fixes tag
> (possibly on some original sha1 that added the driver, since I think
> it's always been broken) or at least a commit title that more clearly
> flags it as a 'fix' or cc stable...
>
> (something like 'usb: gadget: ncm: fix rare win11 packet discard')
>
> We definitely want this to hit LTS...
One more thing: the 'goto done' and 'done' label are not needed -
that's already the natural code flow...
So the 'goto' could just be replaced with a comment or perhaps with
--to_process.
Powered by blists - more mailing lists