From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mga04.intel.com (mga04.intel.com [192.55.52.120]) by mx.groups.io with SMTP id smtpd.web12.6798.1585747840933028111 for ; Wed, 01 Apr 2020 06:30:43 -0700 Authentication-Results: mx.groups.io; dkim=missing; spf=none, err=permanent DNS error (domain: linux.intel.com, ip: 192.55.52.120, mailfrom: maciej.rabeda@linux.intel.com) IronPort-SDR: A5qDMCKh5PX30ORiaw0h+pDtcPDsA4/oLMCBM8F8Qbq7IEme39hNfeHkl6fISOJk7gbvFCJMu4 IPdMbz9CXAlg== X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from orsmga004.jf.intel.com ([10.7.209.38]) by fmsmga104.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 01 Apr 2020 06:30:40 -0700 IronPort-SDR: ug8dYwRzySWqDa+SQGBXrjxxRPj2lvKHU2GmlOAItgtBwKyjqSS9GtQDMcSfH5XYC2LcFxjsLV TjwUZgsoQbcw== X-IronPort-AV: E=Sophos;i="5.72,331,1580803200"; d="scan'208,217";a="395978465" Received: from mrabeda-mobl.ger.corp.intel.com (HELO [10.213.11.210]) ([10.213.11.210]) by orsmga004-auth.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 01 Apr 2020 06:30:38 -0700 Subject: Re: [edk2-devel] [PATCH] NetworkPkg/Ip6Dxe: Fix ASSERT logic in Ip6ProcessRouterAdvertise() To: devel@edk2.groups.io, lersek@redhat.com Cc: Jiaxin Wu , Siyuan Fu References: <20200401095321.1966-1-maciej.rabeda@linux.intel.com> <5ab7e131-0d14-a2ef-ec00-caf055f7a862@redhat.com> From: "Maciej Rabeda" Message-ID: Date: Wed, 1 Apr 2020 15:30:33 +0200 User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:68.0) Gecko/20100101 Thunderbird/68.6.0 MIME-Version: 1.0 In-Reply-To: <5ab7e131-0d14-a2ef-ec00-caf055f7a862@redhat.com> Content-Type: multipart/alternative; boundary="------------0F5973BFDB5E0DE6F721BCFD" Content-Language: pl --------------0F5973BFDB5E0DE6F721BCFD Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 7bit Hey Laszlo, Thanks for your _meta-suggestions_ :) (1a): BZ created, referenced to BZ 2174, marked with 'regression': https://bugzilla.tianocore.org/show_bug.cgi?id=2655 (2): This will occur upon passed review and pull request to edk2/master Siyuan/Jiaxin, Can we get this through? Thanks, Maciej On 01-Apr-20 14:02, Laszlo Ersek wrote: > Hi Maciej, > > On 04/01/20 11:53, Maciej Rabeda wrote: >> This patch fixes reversed logic of recently added ASSERTs which should >> ensure that Ip6IsNDOptionValid() implementation properly reacts to invalid >> packets. >> >> Cc: Jiaxin Wu >> Cc: Siyuan Fu >> Signed-off-by: Maciej Rabeda >> Tested-by: Laszlo Ersek >> --- >> NetworkPkg/Ip6Dxe/Ip6Nd.c | 6 +++--- >> 1 file changed, 3 insertions(+), 3 deletions(-) > Thanks for the patch. Two meta-suggestions: > > (1) we should do one of the following: > > (1a) Create a new BZ for this issue, cross-link it -- via the See Also > field -- with TianoCore#2174, and reference the new BZ in this commit > message. If we file this new BZ, it should get the Regression keyword. > > (1b) Or else we should reopen TianoCore#2174, and reference *that* BZ in > this commit message. > > (2) Independently of (1), the commit message should carry the following tag: > > Fixes: 9c20342eed70ec99ec50cd73cb81804299f05403 > > Regarding this patch, the above updates only affect the commit message, > so I'm not asking for a v2 -- you can implement the commit message > changes right before pushing. > > Thanks! > Laszlo > > > >> 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 >> > > > --------------0F5973BFDB5E0DE6F721BCFD Content-Type: text/html; charset=windows-1252 Content-Transfer-Encoding: 7bit Hey Laszlo,

Thanks for your meta-suggestions :)

(1a): BZ created, referenced to BZ 2174, marked with 'regression': https://bugzilla.tianocore.org/show_bug.cgi?id=2655
(2): This will occur upon passed review and pull request to edk2/master

Siyuan/Jiaxin,

Can we get this through?

Thanks,
Maciej

On 01-Apr-20 14:02, Laszlo Ersek wrote:
Hi Maciej,

On 04/01/20 11:53, Maciej Rabeda wrote:

This patch fixes reversed logic of recently added ASSERTs which should
ensure that Ip6IsNDOptionValid() implementation properly reacts to invalid
packets.

Cc: Jiaxin Wu <jiaxin.wu@intel.com>
Cc: Siyuan Fu <siyuan.fu@intel.com>
Signed-off-by: Maciej Rabeda <maciej.rabeda@linux.intel.com>
Tested-by: Laszlo Ersek <lersek@redhat.com>
---
 NetworkPkg/Ip6Dxe/Ip6Nd.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

Thanks for the patch. Two meta-suggestions:

(1) we should do one of the following:

(1a) Create a new BZ for this issue, cross-link it -- via the See Also
field -- with TianoCore#2174, and reference the new BZ in this commit
message. If we file this new BZ, it should get the Regression keyword.

(1b) Or else we should reopen TianoCore#2174, and reference *that* BZ in
this commit message.

(2) Independently of (1), the commit message should carry the following tag:

Fixes: 9c20342eed70ec99ec50cd73cb81804299f05403

Regarding this patch, the above updates only affect the commit message,
so I'm not asking for a v2 -- you can implement the commit message
changes right before pushing.

Thanks!
Laszlo




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

--------------0F5973BFDB5E0DE6F721BCFD--