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: <CANP3RGeirg+f8cBbw_3YR5AvuB1ZxJC_9-wcn+Tb-GXf1ESKCQ@mail.gmail.com>
Date: Tue, 2 Jan 2024 02:32:16 -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] usb: gadget: ncm: Avoid dropping datagrams of properly
 parsed NTBs

en

On Mon, Jan 1, 2024 at 9:52 PM 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.

I think this is likely Windows trying to avoid generating 0 length frames.

(usb max single datagram [frame?] size is 1024 bytes).

My guess is this extra byte will only ever happen at the end of a
multiple of 1024 bytes,
and it will always be exactly one byte, and it will likely be a '0' pad byte.

Could you check if a more specific test of this sort would make sense?
(ie. fix the problem)

Something like

if (to_process == 1) && (current_offset & 1023 == 0) && (*payload == 0)
  // extra 1 zero byte pad to prevent multiple of 1024 sized packet
  return
}

It seems a little dangerous to just blindly ignore arbitrary amounts
of trailing garbage...

>
> 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)
>
> Fix this by checking if the leftover bytes before parsing next NTB is of
> size more than the expected header.
>
> Fixes: 427694cfaafa ("usb: gadget: ncm: Handle decoding of multiple NTB's in unwrap call")
> Signed-off-by: Krishna Kurapati <quic_kriskura@...cinc.com>
> ---
> There could probably be cases where the first NTB is proper and the second
> NTB's header is proper but the NDP is corrupt, and in those cases too, all
> the datagrams are dropped. But I haven't seen such case practically.
>
>  drivers/usb/gadget/function/f_ncm.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/usb/gadget/function/f_ncm.c b/drivers/usb/gadget/function/f_ncm.c
> index cc0ed29a4adc..a75b6dc8b0cb 100644
> --- a/drivers/usb/gadget/function/f_ncm.c
> +++ b/drivers/usb/gadget/function/f_ncm.c
> @@ -1325,7 +1325,7 @@ 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 > opts->nth_size) {

shouldn't this check actually be >= not > ?

>                 ntb_ptr = (unsigned char *)(ntb_ptr + block_len);
>                 goto parse_ntb;
>         }
> --
> 2.42.0
>


--
Maciej Żenczykowski, Kernel Networking Developer @ Google