Acked-by: Zhichao Gao <zhichao.gao@...>

-----Original Message-----
From: Fu, Siyuan <siyuan.fu@...>
Sent: Tuesday, March 31, 2020 7:54 PM
To: devel@edk2.groups.io; lersek@...; Ni, Ray <ray.ni@...>;
Gao, Zhichao <zhichao.gao@...>
Cc: maciej.rabeda@...
Subject: RE: [edk2-devel] [PATCH v1] ShellPkg: Fix 'ping' command Ip4 receive
flow.

Reviewed-by: Siyuan Fu <siyuan.fu@...>

-----Original Message-----
From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Laszlo
Ersek
Sent: 2020年3月25日 19:34
To: Ni, Ray <ray.ni@...>; Gao, Zhichao <zhichao.gao@...>
Cc: devel@edk2.groups.io; maciej.rabeda@...
Subject: Re: [edk2-devel] [PATCH v1] ShellPkg: Fix 'ping' command Ip4
receive flow.

Ray, Zhichao,

On 02/27/20 12:02, Maciej Rabeda wrote:

REF: https://bugzilla.tianocore.org/show_bug.cgi?id=2032

'ping' command's receive flow utilizes a single Rx token which it
attempts to reuse before recycling the previously received packet.
This causes a situation where under ICMP traffic,
Ping6OnEchoReplyReceived() function will receive an already recycled
packet with EFI_SUCCESS token status and finally dereference invalid
pointers from RxData structure.

Cc: Ray Ni <ray.ni@...>
Cc: Zhichao Gao <zhichao.gao@...>
Signed-off-by: Maciej Rabeda <maciej.rabeda@...>
---
 ShellPkg/Library/UefiShellNetwork1CommandsLib/Ping.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

can you please review this ShellPkg patch? It's been on the list for
almost a month now.

Thanks
Laszlo

diff --git a/ShellPkg/Library/UefiShellNetwork1CommandsLib/Ping.c

b/ShellPkg/Library/UefiShellNetwork1CommandsLib/Ping.c

index 23567fa2c1bb..a3fa32515192 100644
--- a/ShellPkg/Library/UefiShellNetwork1CommandsLib/Ping.c
+++ b/ShellPkg/Library/UefiShellNetwork1CommandsLib/Ping.c
@@ -614,6 +614,11 @@ Ping6OnEchoReplyReceived (

 ON_EXIT:

+  //
+  // Recycle the packet before reusing RxToken  //
+ gBS->SignalEvent (Private->IpChoice ==

PING_IP_CHOICE_IP6?((EFI_IP6_RECEIVE_DATA*)Private-

RxToken.Packet.RxData)->RecycleSignal:((EFI_IP4_RECEIVE_DATA*)Private
- RxToken.Packet.RxData)->RecycleSignal);
+
   if (Private->RxCount < Private->SendNum) {
     //
     // Continue to receive icmp echo reply packets.
@@ -632,10 +637,6 @@ ON_EXIT:
     //
     Private->Status = EFI_SUCCESS;
   }
-  //
-  // Singal to recycle the each rxdata here, not at the end of process.
-  //
-  gBS->SignalEvent (Private->IpChoice ==

PING_IP_CHOICE_IP6?((EFI_IP6_RECEIVE_DATA*)Private-

RxToken.Packet.RxData)->RecycleSignal:((EFI_IP4_RECEIVE_DATA*)Private
- RxToken.Packet.RxData)->RecycleSignal);
 }

 /**

There is a spell error in the comments of VariableServiceGetVariable() in Variable.c.
- @return EFI_BUFFER_TO_SMALL DataSize is too small for the result.
+ @return EFI_BUFFER_TOO_SMALL DataSize is too small for the result.
Need create new bugs for it or fix in this comment directly?

-----Original Message-----
From: devel@edk2.groups.io [mailto:devel@edk2.groups.io] On Behalf Of
Wang, Jian J
Sent: Monday, March 30, 2020 12:16 PM
To: michael.kubacki@...; devel@edk2.groups.io
Cc: Bret Barkelew <Bret.Barkelew@...>; Gao, Liming
<liming.gao@...>; Kinney, Michael D <michael.d.kinney@...>;
Wu, Hao A <hao.a.wu@...>
Subject: Re: [edk2-devel] [PATCH v3 1/3] MdeModulePkg Variable: Return
GetVariable() attr if EFI_BUFFER_TOO_SMALL


Reviewed-by: Jian J Wang <jian.j.wang@...>

Regards,
Jian

-----Original Message-----
From: michael.kubacki@... <michael.kubacki@...>
Sent: Saturday, March 28, 2020 5:56 AM
To: devel@edk2.groups.io
Cc: Bret Barkelew <Bret.Barkelew@...>; Gao, Liming
<liming.gao@...>; Kinney, Michael D
<michael.d.kinney@...>; Wang, Jian J <jian.j.wang@...>;
Wu, Hao A <hao.a.wu@...>
Subject: [PATCH v3 1/3] MdeModulePkg Variable: Return GetVariable()
attr if EFI_BUFFER_TOO_SMALL

From: Michael Kubacki <michael.kubacki@...>

REF:https://bugzilla.tianocore.org/show_bug.cgi?id=2062

The UEFI specification v2.8 Errata A Section 8.2 "GetVariable()"
"Attributes" parameter description states:

"If not NULL, a pointer to the memory location to return the
attributes bitmask for the variable. See 'Related Definitions.'
If not NULL, then Attributes is set on output both when EFI_SUCCESS
and when EFI_BUFFER_TOO_SMALL is returned."

The attributes were previously only returned from the implementation
in Variable.c on EFI_SUCCESS. They are now returned on EFI_SUCCESS or
EFI_BUFFER_TOO_SMALL according to spec.

Cc: Bret Barkelew <Bret.Barkelew@...>
Cc: Liming Gao <liming.gao@...>
Cc: Michael D Kinney <michael.d.kinney@...>
Cc: Jian J Wang <jian.j.wang@...>
Cc: Hao A Wu <hao.a.wu@...>
Signed-off-by: Michael Kubacki <michael.kubacki@...>
---
MdeModulePkg/Universal/Variable/RuntimeDxe/Variable.c | 10
+++++++---

MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmmRuntimeDxe.
c |

10 ++++++----
2 files changed, 13 insertions(+), 7 deletions(-)

diff --git a/MdeModulePkg/Universal/Variable/RuntimeDxe/Variable.c
b/MdeModulePkg/Universal/Variable/RuntimeDxe/Variable.c
index d23aea4bc712..1e71fc642c76 100644
--- a/MdeModulePkg/Universal/Variable/RuntimeDxe/Variable.c
+++ b/MdeModulePkg/Universal/Variable/RuntimeDxe/Variable.c
@@ -18,6 +18,8 @@

Copyright (c) 2006 - 2020, Intel Corporation. All rights
reserved.<BR>
(C) Copyright 2015-2018 Hewlett Packard Enterprise Development LP<BR>
+Copyright (c) Microsoft Corporation.<BR>
+
SPDX-License-Identifier: BSD-2-Clause-Patent

**/
@@ -2379,9 +2381,6 @@ VariableServiceGetVariable (
}

CopyMem (Data, GetVariableDataPtr (Variable.CurrPtr,
mVariableModuleGlobal->VariableGlobal.AuthFormat), VarDataSize);
- if (Attributes != NULL) {
- *Attributes = Variable.CurrPtr->Attributes;
- }

*DataSize = VarDataSize;
UpdateVariableInfo (VariableName, VendorGuid, Variable.Volatile,
TRUE, FALSE, FALSE, FALSE, &gVariableInfo); @@ -2395,6 +2394,11 @@
VariableServiceGetVariable (
}

Done:
+ if (Status == EFI_SUCCESS || Status == EFI_BUFFER_TOO_SMALL) {
+ if (Attributes != NULL && Variable.CurrPtr != NULL) {
+ *Attributes = Variable.CurrPtr->Attributes;
+ }
+ }
ReleaseLockOnlyAtBootTime (&mVariableModuleGlobal-

VariableGlobal.VariableServicesLock);

return Status;
}
diff --git

a/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmmRuntimeDx
e.c

b/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmmRuntimeDx
e.c

index 2cf0ed32ae55..ca833fb0244d 100644
---

a/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmmRuntimeDx
e.c

+++

b/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmmRuntimeDx
e.c

@@ -14,6 +14,7 @@
InitCommunicateBuffer() is really function to check the variable data size.

Copyright (c) 2010 - 2019, Intel Corporation. All rights
reserved.<BR>
+Copyright (c) Microsoft Corporation.<BR>
SPDX-License-Identifier: BSD-2-Clause-Patent

**/
@@ -642,10 +643,6 @@ FindVariableInRuntimeCache (
}

CopyMem (Data, GetVariableDataPtr (RtPtrTrack.CurrPtr,
mVariableAuthFormat), TempDataSize);
- if (Attributes != NULL) {
- *Attributes = RtPtrTrack.CurrPtr->Attributes;
- }
-
*DataSize = TempDataSize;

UpdateVariableInfo (VariableName, VendorGuid,
RtPtrTrack.Volatile, TRUE, FALSE, FALSE, TRUE, &mVariableInfo); @@
-661,6 +658,11 @@ FindVariableInRuntimeCache (
}

Done:
+ if (Status == EFI_SUCCESS || Status == EFI_BUFFER_TOO_SMALL) {
+ if (Attributes != NULL && RtPtrTrack.CurrPtr != NULL) {
+ *Attributes = RtPtrTrack.CurrPtr->Attributes;
+ }
+ }
mVariableRuntimeCacheReadLock = FALSE;

return Status;
--
2.16.3.windows.1

-----Original Message-----
From: Fu, Siyuan <siyuan.fu@...>
Sent: Tuesday, March 31, 2020 7:54 PM
To: devel@edk2.groups.io; lersek@...; Ni, Ray <ray.ni@...>;
Gao, Zhichao <zhichao.gao@...>
Cc: maciej.rabeda@...
Subject: RE: [edk2-devel] [PATCH v1] ShellPkg: Fix 'ping' command Ip4 receive
flow.

Reviewed-by: Siyuan Fu <siyuan.fu@...>

-----Original Message-----
From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Laszlo
Ersek
Sent: 2020年3月25日 19:34
To: Ni, Ray <ray.ni@...>; Gao, Zhichao <zhichao.gao@...>
Cc: devel@edk2.groups.io; maciej.rabeda@...
Subject: Re: [edk2-devel] [PATCH v1] ShellPkg: Fix 'ping' command Ip4
receive flow.

Ray, Zhichao,

On 02/27/20 12:02, Maciej Rabeda wrote:

REF: https://bugzilla.tianocore.org/show_bug.cgi?id=2032

'ping' command's receive flow utilizes a single Rx token which it
attempts to reuse before recycling the previously received packet.
This causes a situation where under ICMP traffic,
Ping6OnEchoReplyReceived() function will receive an already recycled
packet with EFI_SUCCESS token status and finally dereference invalid
pointers from RxData structure.

Cc: Ray Ni <ray.ni@...>
Cc: Zhichao Gao <zhichao.gao@...>
Signed-off-by: Maciej Rabeda <maciej.rabeda@...>
---
ShellPkg/Library/UefiShellNetwork1CommandsLib/Ping.c | 9 +++++----
1 file changed, 5 insertions(+), 4 deletions(-)

can you please review this ShellPkg patch? It's been on the list for
almost a month now.

Thanks
Laszlo

diff --git a/ShellPkg/Library/UefiShellNetwork1CommandsLib/Ping.c

b/ShellPkg/Library/UefiShellNetwork1CommandsLib/Ping.c

index 23567fa2c1bb..a3fa32515192 100644
--- a/ShellPkg/Library/UefiShellNetwork1CommandsLib/Ping.c
+++ b/ShellPkg/Library/UefiShellNetwork1CommandsLib/Ping.c
@@ -614,6 +614,11 @@ Ping6OnEchoReplyReceived (

ON_EXIT:

+ //
+ // Recycle the packet before reusing RxToken //
+ gBS->SignalEvent (Private->IpChoice ==

PING_IP_CHOICE_IP6?((EFI_IP6_RECEIVE_DATA*)Private-

RxToken.Packet.RxData)->RecycleSignal:((EFI_IP4_RECEIVE_DATA*)Private
- RxToken.Packet.RxData)->RecycleSignal);
+
if (Private->RxCount < Private->SendNum) {
//
// Continue to receive icmp echo reply packets.
@@ -632,10 +637,6 @@ ON_EXIT:
//
Private->Status = EFI_SUCCESS;
}
- //
- // Singal to recycle the each rxdata here, not at the end of process.
- //
- gBS->SignalEvent (Private->IpChoice ==

PING_IP_CHOICE_IP6?((EFI_IP6_RECEIVE_DATA*)Private-

RxToken.Packet.RxData)->RecycleSignal:((EFI_IP4_RECEIVE_DATA*)Private
- RxToken.Packet.RxData)->RecycleSignal);
}

/**

-----Original Message-----
From: devel@edk2.groups.io <devel@edk2.groups.io> On
Behalf Of Laszlo Ersek
Sent: Tuesday, March 31, 2020 5:42 AM
To: Ard Biesheuvel <ard.biesheuvel@...>; edk2-
devel-groups-io <devel@edk2.groups.io>;
macarl@...
Subject: Re: [edk2-devel] [PATCH] CryptoPkg/FltUsedLib:
Add FltUsedLib for float.

On 03/30/20 23:41, Ard Biesheuvel wrote:

On Mon, 30 Mar 2020 at 23:29, Matthew Carlson via

Groups.Io

<macarl@...> wrote:

So it's not required by OpenSSL, it's required by the

compiler whenever floating point is used, which can be in
multiple places. For example, this is used in mu_plus
(the Microsoft UEFI value add to EDK2) by the
OnScreenKeyboard driver as well as the UiToolKit driver.

OK, so this is the part that was not obvious to me. This
is basically
code that exists on top of edk2 (consumes edk2) *AND*
uses floating
point *internally*.

If you happen to use BaseCryptLib or the intrinsic in a

driver that happens to need crypto as well, it can break
due to multiple.

I do agree, this only applies to MSVC and requiring

every platform to add a line in their DSC would be a
situation I would prefer to avoid if possible. Is there a
way to specify a library dependency only if a toolchain
is being used?

Yes, so this either belongs in one of the

IntrinsicsLibs we have, or

in the various Entrypoint libraries we have for PEIMs,

DXEs, etc.

However, given that we are talking about static

libraries here, adding

a source file that *only* defines __fltused (and

nothing else) to any

library should also work, as the resulting object file

will only be

incorporated by the linker if it is needed to satisfy a

symbol

dependency, and so it can never cause a conflict if it

is the only

symbol in the object.

IMO: if edk2 intends to support out-of-tree modules that
themselves
utilize floating point, *with or without* a dependency on
OpensslLib,
then we should add Ard's suggestion:

[Sources]
+ FloatUsed.c | MSFT

to some of the "most core" edk2 library instances, i.e.
those that *all*
modules of the given module type inevitably depend on.
The entry point
libs look like a great idea to me.

That should link the _fltused external definition exactly
once into all
modules built with MSVC, regardless of whether each such
module uses
floating point or not.

Thanks
Laszlo

Leif,
Thanks for introducing such an interesting tool.
I see this too is very useful for code refactoring.
It's a game changing tool😊

To help me understand, do you suggest MAYBE when incompatible changes like this
happen, the change owners propose the semantic patches for all platforms?


Thanks,
Ray

-----Original Message-----
From: Leif Lindholm <leif@...>
Sent: Tuesday, March 31, 2020 5:22 PM
To: Ni, Ray <ray.ni@...>
Cc: Ard Biesheuvel <ard.biesheuvel@...>; Jiang, Guomin <guomin.jiang@...>; devel@edk2.groups.io; Andrew
Fish <afish@...>; Laszlo Ersek <lersek@...>; Kinney, Michael D <michael.d.kinney@...>
Subject: Re: [edk2-devel] API breakages and their implications. Was: [PATCH 1/1] MdeModulePkg: UART Dynamic clock freq
Support

On Tue, Mar 31, 2020 at 01:53:21 +0000, Ni, Ray wrote:

Leif,
Please understand that the concern of this change is all the platforms that uses
this serial port lib must be changed otherwise build breaks.

Yes. This is the nature of collaborative development.
This is something we on the ARM side have lived with since we got
involved with EDK2, being the less-deployed user of the codebase, and
that is fine.

TianoCore specifically produced the UDK to make life easier for those
who are unable to track upstream, and we have followed that up with
stable tags. I would highly recommend that any team that feels that
this change would be too much effort to move to edk2-stable202002. Of
course, they would then need to manually cherry-pick any improvements
and security fixes from master. That is their choice.

Nevertheless, breaking existing platforms is a side effect, not a
goal. So if an alternative solution can be found (which is not a hack
that is going to affect the codebase negatively over time and simply
need to be fixed properly at a later date), then clearly that is
preferable.

"This breaks many platforms" is a good argument for seeing if a
solution can be found that does not break (as) many platforms. It is
not an argument for duplicating drivers when the change needed for
those platforms is trivial.

These days, Linux tends to deal with API breakages (and other things)
using a semantic patch tool called Coccinelle[1]. It would not be
unreasonable, and indeed it would be helpful to us on the non-x86 side
if something similar was adopted (and semantic patches mandated) for
API breakages in EDK2.

[1] http://coccinelle.lip6.fr/sp.php

Regards,

Leif

Ard,
Using Guided HOB sounds a good idea to me: )
The benefits of using HOB is:
Length field in the HOB header can be used for extension if more parameters are needed.
DXE can have the HOB access as well.

EFI_SEC_HOB_DATA_PPI can be used to return the new Guided HOB from SEC phase if needed.

Thanks,
Ray

-----Original Message-----
From: Ard Biesheuvel <ard.biesheuvel@...>
Sent: Monday, March 30, 2020 3:45 PM
To: Leif Lindholm <leif@...>
Cc: Jiang, Guomin <guomin.jiang@...>; devel@edk2.groups.io; pankaj.bansal@...; Ni, Ray

<ray.ni@...>;

Wang, Jian J <jian.j.wang@...>; Wu, Hao A <hao.a.wu@...>; Ma, Maurice <maurice.ma@...>; Dong,
Guo <guo.dong@...>; You, Benjamin <benjamin.you@...>; Meenakshi Aggarwal
<meenakshi.aggarwal@...>; Varun Sethi <V.Sethi@...>; Samer El-Haj-Mahmoud <Samer.El-Haj-
Mahmoud@...>
Subject: Re: [edk2-devel] [PATCH 1/1] MdeModulePkg: UART Dynamic clock freq Support

On Mon, 30 Mar 2020 at 09:35, Leif Lindholm <leif@...> wrote:

Hi Jiang,

It is not a question of effort of copying a driver, it is a question
that copying drivers is something that should be avoided wherever
practically possible. I did not think this topic was still under
debate.

If the existing 16550 SerialPortLib is overspecialised to the point
where it only works on a subset of 16550 implementations, then it
should change. There are going to be more non-PC systems turning up
with 16550 UARTs - should they each copy/modify their drivers?

If there are better ways of solving that problem, please suggest.
But more duplicated drivers is not the answer.

Could we use a GUIDed HOB? If it exists, we use its contents, and if
it doesn't, we use the default set by the FixedPCD.

-----Original Message-----
From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Laszlo
Ersek
Sent: 2020年3月25日 19:34
To: Ni, Ray <ray.ni@...>; Gao, Zhichao <zhichao.gao@...>
Cc: devel@edk2.groups.io; maciej.rabeda@...
Subject: Re: [edk2-devel] [PATCH v1] ShellPkg: Fix 'ping' command Ip4 receive
flow.

Ray, Zhichao,

On 02/27/20 12:02, Maciej Rabeda wrote:

REF: https://bugzilla.tianocore.org/show_bug.cgi?id=2032

'ping' command's receive flow utilizes a single Rx token which it
attempts to reuse before recycling the previously received packet.
This causes a situation where under ICMP traffic,
Ping6OnEchoReplyReceived() function will receive an already
recycled packet with EFI_SUCCESS token status and finally
dereference invalid pointers from RxData structure.

Cc: Ray Ni <ray.ni@...>
Cc: Zhichao Gao <zhichao.gao@...>
Signed-off-by: Maciej Rabeda <maciej.rabeda@...>
---
ShellPkg/Library/UefiShellNetwork1CommandsLib/Ping.c | 9 +++++----
1 file changed, 5 insertions(+), 4 deletions(-)

can you please review this ShellPkg patch? It's been on the list for
almost a month now.

Thanks
Laszlo

diff --git a/ShellPkg/Library/UefiShellNetwork1CommandsLib/Ping.c

b/ShellPkg/Library/UefiShellNetwork1CommandsLib/Ping.c

index 23567fa2c1bb..a3fa32515192 100644
--- a/ShellPkg/Library/UefiShellNetwork1CommandsLib/Ping.c
+++ b/ShellPkg/Library/UefiShellNetwork1CommandsLib/Ping.c
@@ -614,6 +614,11 @@ Ping6OnEchoReplyReceived (

ON_EXIT:

+ //
+ // Recycle the packet before reusing RxToken
+ //
+ gBS->SignalEvent (Private->IpChoice ==

PING_IP_CHOICE_IP6?((EFI_IP6_RECEIVE_DATA*)Private-

RxToken.Packet.RxData)->RecycleSignal:((EFI_IP4_RECEIVE_DATA*)Private-
RxToken.Packet.RxData)->RecycleSignal);
+
if (Private->RxCount < Private->SendNum) {
//
// Continue to receive icmp echo reply packets.
@@ -632,10 +637,6 @@ ON_EXIT:
//
Private->Status = EFI_SUCCESS;
}
- //
- // Singal to recycle the each rxdata here, not at the end of process.
- //
- gBS->SignalEvent (Private->IpChoice ==

PING_IP_CHOICE_IP6?((EFI_IP6_RECEIVE_DATA*)Private-

RxToken.Packet.RxData)->RecycleSignal:((EFI_IP4_RECEIVE_DATA*)Private-
RxToken.Packet.RxData)->RecycleSignal);
}

/**

Hi Maciej,

On 03/30/20 14:23, Maciej Rabeda wrote:

REF: https://bugzilla.tianocore.org/show_bug.cgi?id=2174

Problem has been identified with Ip6ProcessRouterAdvertise() when
Router Advertise packet contains options with malicious/invalid
'Length' field. This can lead to platform entering infinite loop
when processing options from that packet.

Cc: Jiaxin Wu <jiaxin.wu@...>
Cc: Siyuan Fu <siyuan.fu@...>
Signed-off-by: Maciej Rabeda <maciej.rabeda@...>
Reviewed-by: Siyuan Fu <siyuan.fu@...>
---
NetworkPkg/Ip6Dxe/Ip6Nd.c | 44 +++++++++------
NetworkPkg/Ip6Dxe/Ip6Nd.h | 13 +++++
NetworkPkg/Ip6Dxe/Ip6Option.c | 57 +++++++++++++++-----
3 files changed, 83 insertions(+), 31 deletions(-)

diff --git a/NetworkPkg/Ip6Dxe/Ip6Nd.c b/NetworkPkg/Ip6Dxe/Ip6Nd.c
index 4288ef02dd46..fd7f60b2f92c 100644
--- a/NetworkPkg/Ip6Dxe/Ip6Nd.c
+++ b/NetworkPkg/Ip6Dxe/Ip6Nd.c
@@ -1927,7 +1927,7 @@ Ip6ProcessRouterAdvertise (
UINT32 ReachableTime;
UINT32 RetransTimer;
UINT16 RouterLifetime;
- UINT16 Offset;
+ UINT32 Offset;
UINT8 Type;
UINT8 Length;
IP6_ETHER_ADDR_OPTION LinkLayerOption;
@@ -2094,10 +2094,11 @@ Ip6ProcessRouterAdvertise (
//
// The only defined options that may appear are the Source
// Link-Layer Address, Prefix information and MTU options.
- // All included options have a length that is greater than zero.
+ // All included options have a length that is greater than zero and
+ // fit within the input packet.
//
Offset = 16;
- while (Offset < Head->PayloadLength) {
+ while (Offset < (UINT32) Head->PayloadLength) {
NetbufCopy (Packet, Offset, sizeof (UINT8), &Type);
switch (Type) {
case Ip6OptionEtherSource:
@@ -2105,9 +2106,12 @@ Ip6ProcessRouterAdvertise (
// Update the neighbor cache
//
NetbufCopy (Packet, Offset, sizeof (IP6_ETHER_ADDR_OPTION), (UINT8 *) &LinkLayerOption);
- if (LinkLayerOption.Length <= 0) {
- goto Exit;
- }
+
+ //
+ // Option size validity ensured by Ip6IsNDOptionValid().
+ //
+ ASSERT (LinkLayerOption.Length != 0);
+ ASSERT (Offset + (UINT32) LinkLayerOption.Length * 8 >= (UINT32) Head->PayloadLength);
ZeroMem (&LinkLayerAddress, sizeof (EFI_MAC_ADDRESS));
CopyMem (&LinkLayerAddress, LinkLayerOption.EtherAddr, 6);
@@ -2151,13 +2155,17 @@ Ip6ProcessRouterAdvertise (
}
}
- Offset = (UINT16) (Offset + (UINT16) LinkLayerOption.Length * 8);
+ Offset += (UINT32) LinkLayerOption.Length * 8;
break;
case Ip6OptionPrefixInfo:
NetbufCopy (Packet, Offset, sizeof (IP6_PREFIX_INFO_OPTION), (UINT8 *) &PrefixOption);
- if (PrefixOption.Length != 4) {
- goto Exit;
- }
+
+ //
+ // Option size validity ensured by Ip6IsNDOptionValid().
+ //
+ ASSERT (PrefixOption.Length == 4);
+ ASSERT (Offset + (UINT32) PrefixOption.Length * 8 >= (UINT32) Head->PayloadLength);

I've hit this ASSERT():

ASSERT NetworkPkg/Ip6Dxe/Ip6Nd.c(2167): Offset + (UINT32) PrefixOption.Length * 8 >= (UINT32) Head->PayloadLength

Here's the packet that triggers it:

02:30:25.350096 52:54:00:18:64:e7 > 33:33:00:00:00:01, ethertype IPv6 (0x86dd), length 142: (class 0xc0, hlim 255, next-header ICMPv6 (58) payload length: 88) fe80::5054:ff:fe18:64e7 > ff02::1: [icmp6 sum ok] ICMP6, router advertisement, length 88
hop limit 64, Flags [none], pref medium, router lifetime 1800s, reachable time 0ms, retrans time 0ms
prefix info option (3), length 32 (4): fd33:eb1b:9b36::/64, Flags [onlink, auto], valid time 3600s, pref. time 3600s
mtu option (5), length 8 (1): 1500
source link-address option (1), length 8 (1): 52:54:00:18:64:e7
rdnss option (25), length 24 (3): lifetime 3600s, addr: fe80::5054:ff:fe18:64e7
0x0000: 6c00 0000 0058 3aff fe80 0000 0000 0000 l....X:.........
0x0010: 5054 00ff fe18 64e7 ff02 0000 0000 0000 PT....d.........
0x0020: 0000 0000 0000 0001 8600 0008 4000 0708 ............@...
0x0030: 0000 0000 0000 0000 0304 40c0 0000 0e10 ..........@.....
0x0040: 0000 0e10 0000 0000 fd33 eb1b 9b36 0000 .........3...6..
0x0050: 0000 0000 0000 0000 0501 0000 0000 05dc ................
0x0060: 0101 5254 0018 64e7 1903 0000 0000 0e10 ..RT..d.........
0x0070: fe80 0000 0000 0000 5054 00ff fe18 64e7 ........PT....d.

Thanks,
Laszlo

+
PrefixOption.ValidLifetime = NTOHL (PrefixOption.ValidLifetime);
PrefixOption.PreferredLifetime = NTOHL (PrefixOption.PreferredLifetime);
@@ -2321,9 +2329,12 @@ Ip6ProcessRouterAdvertise (
break;
case Ip6OptionMtu:
NetbufCopy (Packet, Offset, sizeof (IP6_MTU_OPTION), (UINT8 *) &MTUOption);
- if (MTUOption.Length != 1) {
- goto Exit;
- }
+
+ //
+ // Option size validity ensured by Ip6IsNDOptionValid().
+ //
+ ASSERT (MTUOption.Length == 1);
+ ASSERT (Offset + (UINT32) MTUOption.Length * 8 >= (UINT32) Head->PayloadLength);
//
// Use IPv6 minimum link MTU 1280 bytes as the maximum packet size in order
@@ -2338,11 +2349,10 @@ Ip6ProcessRouterAdvertise (
// Silently ignore unrecognized options
//
NetbufCopy (Packet, Offset + sizeof (UINT8), sizeof (UINT8), &Length);
- if (Length <= 0) {
- goto Exit;
- }
- Offset = (UINT16) (Offset + (UINT16) Length * 8);
+ ASSERT (Length != 0);
+
+ Offset += (UINT32) Length * 8;
break;
}
}
diff --git a/NetworkPkg/Ip6Dxe/Ip6Nd.h b/NetworkPkg/Ip6Dxe/Ip6Nd.h
index 560dfa343782..5f1bd6fb922a 100644
--- a/NetworkPkg/Ip6Dxe/Ip6Nd.h
+++ b/NetworkPkg/Ip6Dxe/Ip6Nd.h
@@ -56,12 +56,21 @@ VOID
VOID *Context
);
+typedef struct _IP6_OPTION_HEADER {
+ UINT8 Type;
+ UINT8 Length;
+} IP6_OPTION_HEADER;
+
+STATIC_ASSERT (sizeof (IP6_OPTION_HEADER) == 2, "IP6_OPTION_HEADER is expected to be exactly 2 bytes long.");
+
typedef struct _IP6_ETHE_ADDR_OPTION {
UINT8 Type;
UINT8 Length;
UINT8 EtherAddr[6];
} IP6_ETHER_ADDR_OPTION;
+STATIC_ASSERT (sizeof (IP6_ETHER_ADDR_OPTION) == 8, "IP6_ETHER_ADDR_OPTION is expected to be exactly 8 bytes long.");
+
typedef struct _IP6_MTU_OPTION {
UINT8 Type;
UINT8 Length;
@@ -69,6 +78,8 @@ typedef struct _IP6_MTU_OPTION {
UINT32 Mtu;
} IP6_MTU_OPTION;
+STATIC_ASSERT (sizeof (IP6_MTU_OPTION) == 8, "IP6_MTU_OPTION is expected to be exactly 8 bytes long.");
+
typedef struct _IP6_PREFIX_INFO_OPTION {
UINT8 Type;
UINT8 Length;
@@ -80,6 +91,8 @@ typedef struct _IP6_PREFIX_INFO_OPTION {
EFI_IPv6_ADDRESS Prefix;
} IP6_PREFIX_INFO_OPTION;
+STATIC_ASSERT (sizeof (IP6_PREFIX_INFO_OPTION) == 32, "IP6_PREFIX_INFO_OPTION is expected to be exactly 32 bytes long.");
+
typedef
VOID
(*IP6_DAD_CALLBACK) (
diff --git a/NetworkPkg/Ip6Dxe/Ip6Option.c b/NetworkPkg/Ip6Dxe/Ip6Option.c
index 4d92a852dc86..6b4b029d1479 100644
--- a/NetworkPkg/Ip6Dxe/Ip6Option.c
+++ b/NetworkPkg/Ip6Dxe/Ip6Option.c
@@ -129,45 +129,74 @@ Ip6IsNDOptionValid (
IN UINT16 OptionLen
)
{
- UINT16 Offset;
- UINT8 OptionType;
+ UINT32 Offset;
UINT16 Length;
+ IP6_OPTION_HEADER *OptionHeader;
+
+ if (Option == NULL) {
+ ASSERT (Option != NULL);
+ return FALSE;
+ }
Offset = 0;
- while (Offset < OptionLen) {
- OptionType = *(Option + Offset);
- Length = (UINT16) (*(Option + Offset + 1) * 8);
+ //
+ // RFC 4861 states that Neighbor Discovery packet can contain zero or more
+ // options. Start processing the options if at least Type + Length fields
+ // fit within the input buffer.
+ //
+ while (Offset + sizeof (IP6_OPTION_HEADER) - 1 < OptionLen) {
+ OptionHeader = (IP6_OPTION_HEADER*) (Option + Offset);
+ Length = (UINT16) OptionHeader->Length * 8;
- switch (OptionType) {
+ switch (OptionHeader->Type) {
case Ip6OptionPrefixInfo:
if (Length != 32) {
return FALSE;
}
-
break;
case Ip6OptionMtu:
if (Length != 8) {
return FALSE;
}
-
break;
default:
- //
- // Check the length of Ip6OptionEtherSource, Ip6OptionEtherTarget, and
- // Ip6OptionRedirected here. For unrecognized options, silently ignore
- // and continue processing the message.
- //
+ // RFC 4861 states that Length field cannot be 0.
if (Length == 0) {
return FALSE;
}
+ break;
+ }
+
+ //
+ // Check whether recognized options are within the input buffer's scope.
+ //
+ switch (OptionHeader->Type) {
+ case Ip6OptionEtherSource:
+ case Ip6OptionEtherTarget:
+ case Ip6OptionPrefixInfo:
+ case Ip6OptionRedirected:
+ case Ip6OptionMtu:
+ if (Offset + Length > (UINT32) OptionLen) {
+ return FALSE;
+ }
+ break;
+ default:
+ //
+ // Unrecognized options can be either valid (but unused) or invalid
+ // (garbage in between or right after valid options). Silently ignore.
+ //
break;
}
- Offset = (UINT16) (Offset + Length);
+ //
+ // Advance to the next option.
+ // Length already considers option header's Type + Length.
+ //
+ Offset += Length;
}
return TRUE;