linux-kernel - Re: [PATCH v2] 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: <CANP3RGf8=3OwCDnE8UT8PQNRwQrtLO97_yR+Vm0h12Q3anbtVA@mail.gmail.com>
Date: Wed, 31 Jan 2024 10:54:31 -0800
From: Maciej Żenczykowski <maze@...gle.com>
To: Krishna Kurapati PSSNV <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 10:08 AM Krishna Kurapati PSSNV
<quic_kriskura@...cinc.com> wrote:
>
>
>
> On 1/31/2024 10:33 PM, Maciej Żenczykowski wrote:
> >>
> >> 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)) {
> >
>
> Hi Maciej,
>
> > 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.
> >
>
> Ahh !!  So you mean, since this comes because the host doesn't want to
> send packets of size wMaxPacketSize on the wire, we need to consider the

of size 'multiple of 512 or 1024', but yes.

> length of data parsed so far to be checked against 512/1024 and not
> block length.

I believe so, yes.

I believe they are trying to avoid having to send ZLPs.
That's determined *purely* by the size of things as they show up on
the usb cable (ie. the size of the usb xfer).
ie. that's where things that are a multiple of 512 (USB2) or 1024
(USB3) need an extra 0 byte sized packet to prevent ZLP.

The actual size of the NTB doesn't matter.

That said... maybe we're overcomplicating this...
Maybe it's enough to just remove this modulo check entirely (I know I
asked for it before).

Ultimately if we just do:

// Windows NCM driver avoids USB ZLPs by adding a 1-byte zero pad as needed
if (to_process == 1 && !*(u8*)(ntb_ptr + block_len)) --to_process;

it'll fix the problem too, and perhaps be easier to understand?

> But either ways, the implementation in the patch also the same thing in
> a different way right ? Process all NTB's present iteratively and while
> doing so, check if there is one byte left. So if there are multiple
> NTB's mixed up to form a packet of size 1024, even then, both the logics
> would converge onto the point where they find that one byte is left.
>
> >> +               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.
> >
>
> Apologies, I didn't understand this comment. By moving the ntb_ptr by
> block length, we are pointing to the (last byte/ start of the next NTB)
> right as we are fixing in [1] ?

I was just pointing out why the above doesn't (afaict) work.

>
> >>                  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...)
> >
>
> Yes they do and we made a fix for that in [1].
>
>
> > 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.
> >
>
> Sure. Will update the commit message accordingly.
>
> [1]:
> https://lore.kernel.org/all/20230927105858.12950-1-quic_kriskura@quicinc.com/
>
> Regards,
> Krishna,

--
Maciej Żenczykowski, Kernel Networking Developer @ Google