netdev - Re: [PATCH net-next v2] usbnet: ipheth: prevent OoB reads of NDP16

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: <fa3b39c4-8509-49ca-91cf-1536059b79d5@redhat.com>
Date: Thu, 19 Sep 2024 10:05:42 +0200
From: Paolo Abeni <pabeni@...hat.com>
To: Foster Snowhill <forst@....gy>, "David S. Miller" <davem@...emloft.net>,
 Eric Dumazet <edumazet@...gle.com>, Jakub Kicinski <kuba@...nel.org>
Cc: Georgi Valkov <gvalkov@...il.com>, Simon Horman <horms@...nel.org>,
 Oliver Neukum <oneukum@...e.com>, netdev@...r.kernel.org,
 linux-usb@...r.kernel.org
Subject: Re: [PATCH net-next v2] usbnet: ipheth: prevent OoB reads of NDP16

On 9/12/24 23:18, Foster Snowhill wrote:
> In "NCM mode", the iOS device encapsulates RX (phone->computer) traffic
> in NCM Transfer Blocks (similarly to CDC NCM). However, unlike reverse
> tethering (handled by the `cdc_ncm` driver), regular tethering is not
> compliant with the CDC NCM spec, as the device is missing the necessary
> descriptors, and TX (computer->phone) traffic is not encapsulated
> at all. Thus `ipheth` implements a very limited subset of the spec with
> the sole purpose of parsing RX URBs.
> 
> In the first iteration of the NCM mode implementation, there were a few
> potential out of bounds reads when processing malformed URBs received
> from a connected device:
> 
> * Only the start of NDP16 (wNdpIndex) was checked to fit in the URB
>    buffer.
> * Datagram length check as part of DPEs could overflow.
> * DPEs could be read past the end of NDP16 and even end of URB buffer
>    if a trailer DPE wasn't encountered.
> 
> The above is not expected to happen in normal device operation.
> 
> To address the above issues for iOS devices in NCM mode, rely on
> and check for a specific fixed format of incoming URBs expected from
> an iOS device:
> 
> * 12-byte NTH16
> * 96-byte NDP16, allowing up to 22 DPEs (up to 21 datagrams + trailer)
> 
> On iOS, NDP16 directly follows NTH16, and its length is constant
> regardless of the DPE count.
> 
> Adapt the driver to use the fixed URB format. Set an upper bound for
> the DPE count based on the expected header size. Always expect a null
> trailer DPE.
> 
> The minimal URB length of 108 bytes (IPHETH_NCM_HEADER_SIZE) in NCM mode
> is already enforced in ipheth since introduction of NCM mode support.
> 
> Signed-off-by: Foster Snowhill <forst@....gy>
> Tested-by: Georgi Valkov <gvalkov@...il.com>
> ---
> v2: No code changes. Update commit message to further clarify that
>      `ipheth` is not and does not aim to be a complete or spec-compliant
>      CDC NCM implementation.
> v1: https://lore.kernel.org/netdev/20240907230108.978355-1-forst@pen.gy/
> 
> This should perhaps go into "net" rather than "net-next"? I submitted
> the previous patch series to "net-next", but it got merged into "net"
> [1]. However it's quite late in the 6.11-rc cycle, so not sure.

This indeed looks like a fix. I suggest to post it for the net tree 
including a suitable fixes tag.

Additionally since it looks like the patch addressed several issues, it 
would be probably better to split it in a small series, each patch 
addressing a single issue - and each patch with it's own fixed tag.

Thanks,

Paolo