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@xxxxxx>
> ---
> v3:
> Split out from a monolithic patch in v2 as an atomic change.
> v2: https://lore.kernel.org/netdev/20240912211817.1707844-1-forst@xxxxxx/
> 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@xxxxxx/
> ---
> 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
