Re: [PATCH] NetworkPkg/Ip6Dxe: Fix ASSERT logic in Ip6ProcessRouterAdvertise()


Siyuan, Fu
 

With Laszlo's comments.
Reviewed-by: Siyuan Fu <siyuan.fu@...>

-----Original Message-----
From: Laszlo Ersek <lersek@...>
Sent: 2020年4月1日 20:02
To: Maciej Rabeda <maciej.rabeda@...>; devel@edk2.groups.io
Cc: Wu, Jiaxin <jiaxin.wu@...>; Fu, Siyuan <siyuan.fu@...>
Subject: Re: [PATCH] NetworkPkg/Ip6Dxe: Fix ASSERT logic in
Ip6ProcessRouterAdvertise()

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@...>
Cc: Siyuan Fu <siyuan.fu@...>
Signed-off-by: Maciej Rabeda <maciej.rabeda@...>
Tested-by: Laszlo Ersek <lersek@...>
---
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

Join devel@edk2.groups.io to automatically receive all group messages.