From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from us-smtp-delivery-1.mimecast.com (us-smtp-delivery-1.mimecast.com [207.211.31.81]) by mx.groups.io with SMTP id smtpd.web11.1662.1585700871411458806 for ; Tue, 31 Mar 2020 17:27:51 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@redhat.com header.s=mimecast20190719 header.b=AFQY8PQc; spf=pass (domain: redhat.com, ip: 207.211.31.81, mailfrom: lersek@redhat.com) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1585700870; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=kQJt+WaJF08WG0EsgC0UZPSzp87f3RTaX+e9b+SindU=; b=AFQY8PQcARaj9E1LlgjgwpLmun2EkwZkrUYqx09dH73iYgTBd4cfY/VWaX06/idawnH6g3 6iUIq5MQ5WXwF8TPfgMA9kV1xNHxAtFzl4p9X9DG87sFkPLvG3ZL1JSD9oq+n8RnCdEcG/ 1Bc96hlycnBDN3o9Z+LC4qz27bONfHI= Received: from mimecast-mx01.redhat.com (mimecast-mx01.redhat.com [209.132.183.4]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-315-5wl0N1KJP8KBR57cczaQYQ-1; Tue, 31 Mar 2020 20:27:48 -0400 X-MC-Unique: 5wl0N1KJP8KBR57cczaQYQ-1 Received: from smtp.corp.redhat.com (int-mx04.intmail.prod.int.phx2.redhat.com [10.5.11.14]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id 6715E8017CC; Wed, 1 Apr 2020 00:27:47 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-115-131.ams2.redhat.com [10.36.115.131]) by smtp.corp.redhat.com (Postfix) with ESMTP id 503BE5D9C5; Wed, 1 Apr 2020 00:27:46 +0000 (UTC) Subject: Re: [edk2-devel] [PATCH v2] NetworkPkg/Ip6Dxe: Improve Neightbor Discovery message validation. To: devel@edk2.groups.io, maciej.rabeda@linux.intel.com Cc: Jiaxin Wu , Siyuan Fu References: <20200330122351.1229-1-maciej.rabeda@linux.intel.com> <19b35911-8d69-2a32-df59-a9fb05e88457@redhat.com> <1a4f9e2f-c0a3-4102-63a2-7e3e13d7f3b9@linux.intel.com> From: "Laszlo Ersek" Message-ID: <498297ae-be77-92cf-1e25-b8695725f3f6@redhat.com> Date: Wed, 1 Apr 2020 02:27:45 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.9.1 MIME-Version: 1.0 In-Reply-To: <1a4f9e2f-c0a3-4102-63a2-7e3e13d7f3b9@linux.intel.com> X-Scanned-By: MIMEDefang 2.79 on 10.5.11.14 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Language: en-US Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit On 03/31/20 14:22, Maciej Rabeda wrote: > Hi Laszlo, > > Thanks for trying this out! > > The condition in the ASSERTs is reversed, consequently for the ASSERTs > added in this function. > I have added them to fire up when Ip6IsNDOptionValid() fails to properly > react to invalid packet (return with an error and do not proceed with > processing options). > In this case, fire assert when current position within the packet > (Offset) moved past the size of the option (Option.Length) * 8 (it's in > 8-byte units) is outside the packet (further than first byte outside the > packet). > Offset one byte past the packet == last option ended at the end of the > packet (packet size is Head->PayloadLength). I understand. In your testing, both sides were probably equal, so the inverted relops didn't cause problems. > Can you try swapping the >= to <= and rerun your test? > This should apply to lines 2114, 2167 and 2337. > > I will submit a patch for that. The following patch works OK for me: diff --git a/NetworkPkg/Ip6Dxe/Ip6Nd.c b/NetworkPkg/Ip6Dxe/Ip6Nd.c index fd7f60b2f92c..0780a98cb325 100644 --- a/NetworkPkg/Ip6Dxe/Ip6Nd.c +++ b/NetworkPkg/Ip6Dxe/Ip6Nd.c @@ -2111,7 +2111,7 @@ Ip6ProcessRouterAdvertise ( // Option size validity ensured by Ip6IsNDOptionValid(). // ASSERT (LinkLayerOption.Length != 0); - ASSERT (Offset + (UINT32) LinkLayerOption.Length * 8 >= (UINT32) Head->PayloadLength); + ASSERT (Offset + (UINT32) LinkLayerOption.Length * 8 <= (UINT32) Head->PayloadLength); ZeroMem (&LinkLayerAddress, sizeof (EFI_MAC_ADDRESS)); CopyMem (&LinkLayerAddress, LinkLayerOption.EtherAddr, 6); @@ -2164,7 +2164,7 @@ Ip6ProcessRouterAdvertise ( // Option size validity ensured by Ip6IsNDOptionValid(). // ASSERT (PrefixOption.Length == 4); - ASSERT (Offset + (UINT32) PrefixOption.Length * 8 >= (UINT32) Head->PayloadLength); + ASSERT (Offset + (UINT32) PrefixOption.Length * 8 <= (UINT32) Head->PayloadLength); PrefixOption.ValidLifetime = NTOHL (PrefixOption.ValidLifetime); PrefixOption.PreferredLifetime = NTOHL (PrefixOption.PreferredLifetime); @@ -2334,7 +2334,7 @@ Ip6ProcessRouterAdvertise ( // Option size validity ensured by Ip6IsNDOptionValid(). // ASSERT (MTUOption.Length == 1); - ASSERT (Offset + (UINT32) MTUOption.Length * 8 >= (UINT32) Head->PayloadLength); + ASSERT (Offset + (UINT32) MTUOption.Length * 8 <= (UINT32) Head->PayloadLength); // // Use IPv6 minimum link MTU 1280 bytes as the maximum packet size in order If your posted patch will be identical to the above code changes, then you can add at once: Tested-by: Laszlo Ersek Thanks! Laszlo