netdev - Re: [PATCH net v3 4/6] usbnet: ipheth: use static NDP16 location in URB

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: <f3657bf6-7980-4c5f-8c82-66c68beb96e4@redhat.com>
Date: Thu, 28 Nov 2024 10:08:17 +0100
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 v3 4/6] usbnet: ipheth: use static NDP16 location in
 URB



On 11/24/24 00:54, Foster Snowhill wrote:
> Original code allowed for the start of NDP16 to be anywhere within the
> URB based on the `wNdpIndex` value in NTH16. Only the start position of
> NDP16 was checked, so it was possible for even the fixed-length part
> of NDP16 to extend past the end of URB, leading to an out-of-bounds
> read.
> 
> On iOS devices, the NDP16 header always directly follows NTH16. Rely on
> and check for this specific format.
> 
> This, along with NCM-specific minimal URB length check that already
> exists, will ensure that the fixed-length part of NDP16 plus a set
> amount of DPEs fit within the URB.

This choice looks fragile. What if the next iOS version moves around
such header?

I think you should add least validate the assumption in the actual URB
payload.

> Note that this commit alone does not fully address the OoB read.
> The limit on the amount of DPEs needs to be enforced separately.
> 
> Fixes: a2d274c62e44 ("usbnet: ipheth: add CDC NCM support")
> Signed-off-by: Foster Snowhill <forst@....gy>
> ---
> v3:
>     Split out from a monolithic patch in v2 as an atomic change.
> v2: https://lore.kernel.org/netdev/20240912211817.1707844-1-forst@pen.gy/
>     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/
> ---
>  drivers/net/usb/ipheth.c | 10 ++++------
>  1 file changed, 4 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/net/usb/ipheth.c b/drivers/net/usb/ipheth.c
> index 48c79e69bb7b..3f9ea6546720 100644
> --- a/drivers/net/usb/ipheth.c
> +++ b/drivers/net/usb/ipheth.c
> @@ -236,16 +236,14 @@ static int ipheth_rcvbulk_callback_ncm(struct urb *urb)
>  	}
>  
>  	ncmh = urb->transfer_buffer;
> -	if (ncmh->dwSignature != cpu_to_le32(USB_CDC_NCM_NTH16_SIGN) ||
> -	    le16_to_cpu(ncmh->wNdpIndex) >= urb->actual_length) {
> +	if (ncmh->dwSignature != cpu_to_le32(USB_CDC_NCM_NTH16_SIGN)) {
>  		dev->net->stats.rx_errors++;
>  		return retval;
>  	}

The URB length is never checked, why it's safe to access (a lot of)
bytes inside the URB without any check?

Thanks,

Paolo