Sriram,
If PrefixLength is allowed to be 128 in Ip6GetPrefix, then the Addr
array is overrun on line 489:
Value = Prefix->Addr[Byte];
Where Byte = PrefixLength / 8.
If 128 should be allowed, then the function should have a separate
condition for that (like when PrefixLength == 0) where SetMem is used to 0xFF
the entire prefix.
Thomas
-----Original Message-----
From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of
Subramanian, Sriram (EG Servers Platform SW)
Sent: Tuesday, April 5, 2016 8:53 AM
To: Zhang Lubo <lubo.zh...@intel.com>; edk2-devel@lists.01.org
Cc: Ye Ting <ting...@intel.com>; Fu Siyuan <siyuan...@intel.com>; Wu Jiaxin
<jiaxin...@intel.com>
Subject: Re: [edk2] [patch 1/2] NetworkPkg: Add new macros and refine codes
> diff --git a/NetworkPkg/Ip6Dxe/Ip6Common.c
> b/NetworkPkg/Ip6Dxe/Ip6Common.c index 4f71d05..9e6d368 100644
> --- a/NetworkPkg/Ip6Dxe/Ip6Common.c
> +++ b/NetworkPkg/Ip6Dxe/Ip6Common.c
> @@ -426,11 +426,11 @@ Ip6RemoveAddr (
> LIST_ENTRY *Entry;
> LIST_ENTRY *Next;
> IP6_ADDRESS_INFO *AddrInfo;
> EFI_IPv6_ADDRESS SnMCastAddr;
>
> - if (IsListEmpty (AddressList) || *AddressCount < 1 || PrefixLength
> > IP6_PREFIX_NUM) {
> + if (IsListEmpty (AddressList) || *AddressCount < 1 || PrefixLength
> + >= (IP6_PREFIX_MAX + 1)) {
> return EFI_INVALID_PARAMETER;
> }
>
> Status = EFI_NOT_FOUND;
The above if() must be PrefixLength > IP6_PREFIX_MAX. The original logic itself
was incorrect (it should have been PrefixLength >= IP6_PREFIX_NUM).
> diff --git a/NetworkPkg/Ip6Dxe/Ip6Icmp.c b/NetworkPkg/Ip6Dxe/Ip6Icmp.c
> index f6a9bb4..f60930e 100644
> --- a/NetworkPkg/Ip6Dxe/Ip6Icmp.c
> +++ b/NetworkPkg/Ip6Dxe/Ip6Icmp.c
> @@ -471,18 +471,18 @@ Ip6GetPrefix (
> UINT8 Byte;
> UINT8 Bit;
> UINT8 Mask;
> UINT8 Value;
>
> - ASSERT ((Prefix != NULL) && (PrefixLength < IP6_PREFIX_NUM));
> + ASSERT ((Prefix != NULL) && (PrefixLength <= IP6_PREFIX_MAX));
This must be PrefixLength < IP6_PREFIX_MAX (again, the original logic was
incorrect, should have been PrefixLength < IP6_PREFIX_NUM - 1).
Also, a question:
Should the original code in Ip6GetPrefix() have allowed PrefixLength == 128? We
had this code:
if (PrefixLength >= IP6_PREFIX_NUM - 1) {
return ;
}
So this means the code rejected the value of 128, however to be consistent with
the RFC should it allow the Prefix of 128?
Thanks,
Sriram.
-----Original Message-----
From: Zhang Lubo [mailto:lubo.zh...@intel.com]
Sent: Tuesday, April 5, 2016 2:18 PM
To: edk2-devel@lists.01.org
Cc: Subramanian, Sriram (EG Servers Platform SW) <srira...@hpe.com>; Fu Siyuan
<siyuan...@intel.com>; Ye Ting <ting...@intel.com>; Wu Jiaxin
<jiaxin...@intel.com>
Subject: [patch 1/2] NetworkPkg: Add new macros and refine codes
Add 2 macros in NetLib.h
#define IP4_MASK_MAX 32
#define IP6_PREFIX_MAX 128
we will use these two macros to check the max mask/prefix length, instead of
#define IP4_MASK_NUM 33
#define IP6_PREFIX_NUM 129
which means a valid number
This will make the code readability and maintainability.
Cc: Subramanian Sriram <srira...@hpe.com>
Cc: Fu Siyuan <siyuan...@intel.com>
Cc: Ye Ting <ting...@intel.com>
Cc: Wu Jiaxin <jiaxin...@intel.com>
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Zhang Lubo <lubo.zh...@intel.com>
---
NetworkPkg/Ip6Dxe/Ip6Common.c | 4 ++--
NetworkPkg/Ip6Dxe/Ip6Icmp.c | 4 ++--
NetworkPkg/Ip6Dxe/Ip6Impl.c | 2 +-
NetworkPkg/Ip6Dxe/Ip6Nd.c | 2 +-
NetworkPkg/Ip6Dxe/Ip6Route.c | 8 ++++----
5 files changed, 10 insertions(+), 10 deletions(-)
diff --git a/NetworkPkg/Ip6Dxe/Ip6Common.c b/NetworkPkg/Ip6Dxe/Ip6Common.c
index 4f71d05..9e6d368 100644
--- a/NetworkPkg/Ip6Dxe/Ip6Common.c
+++ b/NetworkPkg/Ip6Dxe/Ip6Common.c
@@ -426,11 +426,11 @@ Ip6RemoveAddr (
LIST_ENTRY *Entry;
LIST_ENTRY *Next;
IP6_ADDRESS_INFO *AddrInfo;
EFI_IPv6_ADDRESS SnMCastAddr;
- if (IsListEmpty (AddressList) || *AddressCount < 1 || PrefixLength >
IP6_PREFIX_NUM) {
+ if (IsListEmpty (AddressList) || *AddressCount < 1 || PrefixLength >=
+ (IP6_PREFIX_MAX + 1)) {
return EFI_INVALID_PARAMETER;
}
Status = EFI_NOT_FOUND;
@@ -604,11 +604,11 @@ Ip6CopyAddressByPrefix (
UINT8 Byte;
UINT8 Bit;
UINT8 Mask;
ASSERT (Dest != NULL && Src != NULL);
- ASSERT (PrefixLength < IP6_PREFIX_NUM);
+ ASSERT (PrefixLength <= IP6_PREFIX_MAX);
Byte = (UINT8) (PrefixLength / 8);
Bit = (UINT8) (PrefixLength % 8);
ZeroMem (Dest, sizeof (EFI_IPv6_ADDRESS)); diff --git
a/NetworkPkg/Ip6Dxe/Ip6Icmp.c b/NetworkPkg/Ip6Dxe/Ip6Icmp.c index
f6a9bb4..f60930e 100644
--- a/NetworkPkg/Ip6Dxe/Ip6Icmp.c
+++ b/NetworkPkg/Ip6Dxe/Ip6Icmp.c
@@ -471,18 +471,18 @@ Ip6GetPrefix (
UINT8 Byte;
UINT8 Bit;
UINT8 Mask;
UINT8 Value;
- ASSERT ((Prefix != NULL) && (PrefixLength < IP6_PREFIX_NUM));
+ ASSERT ((Prefix != NULL) && (PrefixLength <= IP6_PREFIX_MAX));
if (PrefixLength == 0) {
ZeroMem (Prefix, sizeof (EFI_IPv6_ADDRESS));
return ;
}
- if (PrefixLength >= IP6_PREFIX_NUM - 1) {
+ if (PrefixLength >= IP6_PREFIX_MAX) {
return ;
}
Byte = (UINT8) (PrefixLength / 8);
Bit = (UINT8) (PrefixLength % 8);
diff --git a/NetworkPkg/Ip6Dxe/Ip6Impl.c b/NetworkPkg/Ip6Dxe/Ip6Impl.c index
b186c0a..4e827a9 100644
--- a/NetworkPkg/Ip6Dxe/Ip6Impl.c
+++ b/NetworkPkg/Ip6Dxe/Ip6Impl.c
@@ -840,11 +840,11 @@ EfiIp6Routes (
IP6_PROTOCOL *IpInstance;
EFI_STATUS Status;
EFI_TPL OldTpl;
IP6_SERVICE *IpSb;
- if ((This == NULL) || (PrefixLength >= IP6_PREFIX_NUM)) {
+ if ((This == NULL) || (PrefixLength > IP6_PREFIX_MAX)) {
return EFI_INVALID_PARAMETER;
}
IpInstance = IP6_INSTANCE_FROM_PROTOCOL (This);
IpSb = IpInstance->Service;
diff --git a/NetworkPkg/Ip6Dxe/Ip6Nd.c b/NetworkPkg/Ip6Dxe/Ip6Nd.c index
2c8be42..2cd382c 100644
--- a/NetworkPkg/Ip6Dxe/Ip6Nd.c
+++ b/NetworkPkg/Ip6Dxe/Ip6Nd.c
@@ -196,11 +196,11 @@ Ip6CreatePrefixListEntry (
IP6_ROUTE_ENTRY *RtEntry;
LIST_ENTRY *ListHead;
LIST_ENTRY *Entry;
IP6_PREFIX_LIST_ENTRY *TmpPrefixEntry;
- if (Prefix == NULL || PreferredLifetime > ValidLifetime || PrefixLength >=
IP6_PREFIX_NUM) {
+ if (Prefix == NULL || PreferredLifetime > ValidLifetime ||
+ PrefixLength > IP6_PREFIX_MAX) {
return NULL;
}
NET_CHECK_SIGNATURE (IpSb, IP6_SERVICE_SIGNATURE);
diff --git a/NetworkPkg/Ip6Dxe/Ip6Route.c b/NetworkPkg/Ip6Dxe/Ip6Route.c index
bba365c..af417ad 100644
--- a/NetworkPkg/Ip6Dxe/Ip6Route.c
+++ b/NetworkPkg/Ip6Dxe/Ip6Route.c
@@ -133,11 +133,11 @@ Ip6FindRouteEntry (
ASSERT (Destination != NULL || NextHop != NULL);
RtEntry = NULL;
- for (Index = IP6_PREFIX_NUM - 1; Index >= 0; Index--) {
+ for (Index = IP6_PREFIX_MAX; Index >= 0; Index--) {
NET_LIST_FOR_EACH (Entry, &RtTable->RouteArea[Index]) {
RtEntry = NET_LIST_USER_STRUCT (Entry, IP6_ROUTE_ENTRY, Link);
if (Destination != NULL) {
if (NetIp6IsNetEqual (Destination, &RtEntry->Destination,
RtEntry->PrefixLength)) { @@ -298,11 +298,11 @@ Ip6BuildEfiRouteTable (
//
// Copy the route entry to EFI route table.
//
Count = 0;
- for (Index = IP6_PREFIX_NUM - 1; Index >= 0; Index--) {
+ for (Index = IP6_PREFIX_MAX; Index >= 0; Index--) {
NET_LIST_FOR_EACH (Entry, &(RouteTable->RouteArea[Index])) {
RtEntry = NET_LIST_USER_STRUCT (Entry, IP6_ROUTE_ENTRY, Link);
Ip6CopyAddressByPrefix (
@@ -344,11 +344,11 @@ Ip6CreateRouteTable (
}
RtTable->RefCnt = 1;
RtTable->TotalNum = 0;
- for (Index = 0; Index < IP6_PREFIX_NUM; Index++) {
+ for (Index = 0; Index <= IP6_PREFIX_MAX; Index++) {
InitializeListHead (&RtTable->RouteArea[Index]);
}
for (Index = 0; Index < IP6_ROUTE_CACHE_HASH_SIZE; Index++) {
InitializeListHead (&RtTable->Cache.CacheBucket[Index]);
@@ -383,11 +383,11 @@ Ip6CleanRouteTable (
}
//
// Free all the route table entry and its route cache.
//
- for (Index = 0; Index < IP6_PREFIX_NUM; Index++) {
+ for (Index = 0; Index <= IP6_PREFIX_MAX; Index++) {
NET_LIST_FOR_EACH_SAFE (Entry, Next, &RtTable->RouteArea[Index]) {
RtEntry = NET_LIST_USER_STRUCT (Entry, IP6_ROUTE_ENTRY, Link);
RemoveEntryList (Entry);
Ip6FreeRouteEntry (RtEntry);
}
--
1.9.5.msysgit.1
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
