Re: [edk2-devel] [PATCH v2] NetworkPkg/Ip6Dxe: Improve Neightbor Discovery message validation.

["Laszlo Ersek" <lersek@redhat.com>]

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 <lersek@redhat.com>

Thanks!
Laszlo