Re: [Patch v2] MdeModulePkg/DxeNetLib: Add array range check in NetIp6IsNetEqual().

["Wu, Jiaxin" <jiaxin.wu@intel.com>]

Reviewed-by: Jiaxin Wu <jiaxin.wu@intel.com>


> -----Original Message-----
> From: Wang, Fan
> Sent: Thursday, January 11, 2018 6:14 PM
> To: edk2-devel@lists.01.org
> Cc: Fu, Siyuan <siyuan.fu@intel.com>; Wu, Jiaxin <jiaxin.wu@intel.com>; Wu,
> Hao A <hao.a.wu@intel.com>
> Subject: [Patch v2] MdeModulePkg/DxeNetLib: Add array range check in
> NetIp6IsNetEqual().
> 
> V2
> * Added an ASSERT check for the case PrefixLength equals to
> IP6_PREFIX_MAX.
> * Synced some code descriptions to head file.
> 
> Cc: Fu Siyuan <siyuan.fu@intel.com>
> Cc: Jiaxin Wu <jiaxin.wu@intel.com>
> Cc: Hao Wu <hao.a.wu@intel.com>
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Wang Fan <fan.wang@intel.com>
> ---
>  MdeModulePkg/Include/Library/NetLib.h      | 50
> +++++++++++++++++++++++++++---
>  MdeModulePkg/Library/DxeNetLib/DxeNetLib.c |  8 +++--
>  2 files changed, 52 insertions(+), 6 deletions(-)
> 
> diff --git a/MdeModulePkg/Include/Library/NetLib.h
> b/MdeModulePkg/Include/Library/NetLib.h
> index 7862df9..b0bbaf2 100644
> --- a/MdeModulePkg/Include/Library/NetLib.h
> +++ b/MdeModulePkg/Include/Library/NetLib.h
> @@ -1,10 +1,10 @@
>  /** @file
>    This library is only intended to be used by UEFI network stack modules.
>    It provides basic functions for the UEFI network stack.
> 
> -Copyright (c) 2005 - 2017, Intel Corporation. All rights reserved.<BR>
> +Copyright (c) 2005 - 2018, Intel Corporation. All rights reserved.<BR>
>  This program and the accompanying materials
>  are licensed and made available under the terms and conditions of the BSD
> License
>  which accompanies this distribution.  The full text of the license may be
> found at<BR>
>  http://opensource.org/licenses/bsd-license.php
> 
> @@ -438,10 +438,12 @@ NetIp4IsUnicast (
>    );
> 
>  /**
>    Check whether the incoming IPv6 address is a valid unicast address.
> 
> +  ASSERT if Ip6 is NULL.
> +
>    If the address is a multicast address has binary 0xFF at the start, it is not
>    a valid unicast address. If the address is unspecified ::, it is not a valid
>    unicast address to be assigned to any node. If the address is loopback
> address
>    ::1, it is also not a valid unicast address to be assigned to any physical
>    interface.
> @@ -459,10 +461,12 @@ NetIp6IsValidUnicast (
> 
> 
>  /**
>    Check whether the incoming Ipv6 address is the unspecified address or not.
> 
> +  ASSERT if Ip6 is NULL.
> +
>    @param[in] Ip6   - Ip6 address, in network order.
> 
>    @retval TRUE     - Yes, incoming Ipv6 address is the unspecified address.
>    @retval FALSE    - The incoming Ipv6 address is not the unspecified address
> 
> @@ -474,10 +478,12 @@ NetIp6IsUnspecifiedAddr (
>    );
> 
>  /**
>    Check whether the incoming Ipv6 address is a link-local address.
> 
> +  ASSERT if Ip6 is NULL.
> +
>    @param[in] Ip6   - Ip6 address, in network order.
> 
>    @retval TRUE  - The incoming Ipv6 address is a link-local address.
>    @retval FALSE - The incoming Ipv6 address is not a link-local address.
> 
> @@ -489,10 +495,13 @@ NetIp6IsLinkLocalAddr (
>    );
> 
>  /**
>    Check whether the Ipv6 address1 and address2 are on the connected
> network.
> 
> +  ASSERT if Ip1 or Ip2 is NULL.
> +  ASSERT if PrefixLength exceeds or equals to IP6_PREFIX_MAX.
> +
>    @param[in] Ip1          - Ip6 address1, in network order.
>    @param[in] Ip2          - Ip6 address2, in network order.
>    @param[in] PrefixLength - The prefix length of the checking net.
> 
>    @retval TRUE            - Yes, the Ipv6 address1 and address2 are connected.
> @@ -508,10 +517,12 @@ NetIp6IsNetEqual (
>    );
> 
>  /**
>    Switches the endianess of an IPv6 address.
> 
> +  ASSERT if Ip6 is NULL.
> +
>    This function swaps the bytes in a 128-bit IPv6 address to switch the value
>    from little endian to big endian or vice versa. The byte swapped value is
>    returned.
> 
>    @param  Ip6 Points to an IPv6 address.
> @@ -542,10 +553,12 @@ extern EFI_IPv4_ADDRESS  mZeroIp4Addr;
>  #define NET_RANDOM(Seed)        ((UINT32) ((UINT32) (Seed) *
> 1103515245UL + 12345) % 4294967295UL)
> 
>  /**
>    Extract a UINT32 from a byte stream.
> 
> +  ASSERT if Buf is NULL.
> +
>    This function copies a UINT32 from a byte stream, and then converts it from
> Network
>    byte order to host byte order. Use this function to avoid alignment error.
> 
>    @param[in]  Buf                 The buffer to extract the UINT32.
> 
> @@ -559,10 +572,12 @@ NetGetUint32 (
>    );
> 
>  /**
>    Puts a UINT32 into the byte stream in network byte order.
> 
> +  ASSERT if Buf is NULL.
> +
>    Converts a UINT32 from host byte order to network byte order, then copies
> it to the
>    byte stream.
> 
>    @param[in, out]  Buf          The buffer in which to put the UINT32.
>    @param[in]       Data         The data to be converted and put into the byte
> stream.
> @@ -675,10 +690,12 @@ NetListRemoveTail (
>    );
> 
>  /**
>    Insert a new node entry after a designated node entry of a doubly linked
> list.
> 
> +  ASSERT if PrevEntry or NewEntry is NULL.
> +
>    Inserts a new node entry designated by NewEntry after the node entry
> designated by PrevEntry
>    of the doubly linked list.
> 
>    @param[in, out]  PrevEntry             The entry after which to insert.
>    @param[in, out]  NewEntry              The new entry to insert.
> @@ -692,10 +709,12 @@ NetListInsertAfter (
>    );
> 
>  /**
>    Insert a new node entry before a designated node entry of a doubly linked
> list.
> 
> +  ASSERT if PostEntry or NewEntry is NULL.
> +
>    Inserts a new node entry designated by NewEntry before the node entry
> designated by PostEntry
>    of the doubly linked list.
> 
>    @param[in, out]  PostEntry             The entry to insert before.
>    @param[in, out]  NewEntry              The new entry to insert.
> @@ -837,11 +856,10 @@ NetMapClean (
> 
>    If the number of the <Key, Value> pairs in the netmap is zero, return TRUE.
> 
>    If Map is NULL, then ASSERT().
> 
> -
>    @param[in]  Map                   The net map to test.
> 
>    @return TRUE if the netmap is empty, otherwise FALSE.
> 
>  **/
> @@ -852,10 +870,12 @@ NetMapIsEmpty (
>    );
> 
>  /**
>    Return the number of the <Key, Value> pairs in the netmap.
> 
> +  If Map is NULL, then ASSERT().
> +
>    @param[in]  Map                   The netmap to get the entry number.
> 
>    @return The entry number in the netmap.
> 
>  **/
> @@ -871,10 +891,11 @@ NetMapGetCount (
>    Allocate an item to save the <Key, Value> pair and add corresponding node
> entry
>    to the beginning of the Used doubly linked list. The number of the <Key,
> Value>
>    pairs in the netmap increase by 1.
> 
>    If Map is NULL, then ASSERT().
> +  If Key is NULL, then ASSERT().
> 
>    @param[in, out]  Map                   The netmap to insert into.
>    @param[in]       Key                   The user's key.
>    @param[in]       Value                 The user's value for the key.
> 
> @@ -896,10 +917,11 @@ NetMapInsertHead (
>    Allocate an item to save the <Key, Value> pair and add corresponding node
> entry
>    to the tail of the Used doubly linked list. The number of the <Key, Value>
>    pairs in the netmap increase by 1.
> 
>    If Map is NULL, then ASSERT().
> +  If Key is NULL, then ASSERT().
> 
>    @param[in, out]  Map                   The netmap to insert into.
>    @param[in]       Key                   The user's key.
>    @param[in]       Value                 The user's value for the key.
> 
> @@ -920,10 +942,11 @@ NetMapInsertTail (
> 
>    Iterate the Used doubly linked list of the netmap to get every item.
> Compare the key of every
>    item with the key to search. It returns the point to the item contains the
> Key if found.
> 
>    If Map is NULL, then ASSERT().
> +  If Key is NULL, then ASSERT().
> 
>    @param[in]  Map                   The netmap to search within.
>    @param[in]  Key                   The key to search.
> 
>    @return The point to the item contains the Key, or NULL if Key isn't in the
> map.
> @@ -1165,10 +1188,13 @@ NetLibGetVlanHandle (
>    );
> 
>  /**
>    Get MAC address associated with the network service handle.
> 
> +  If MacAddress is NULL, then ASSERT().
> +  If AddressSize is NULL, then ASSERT().
> +
>    There should be MNP Service Binding Protocol installed on the input
> ServiceHandle.
>    If SNP is installed on the ServiceHandle or its parent handle, MAC address
> will
>    be retrieved from SNP. If no SNP found, try to get SNP mode data use MNP.
> 
>    @param[in]   ServiceHandle    The handle where network service binding
> protocols are
> @@ -1190,10 +1216,12 @@ NetLibGetMacAddress (
> 
>  /**
>    Convert MAC address of the NIC associated with specified Service Binding
> Handle
>    to a unicode string. Callers are responsible for freeing the string storage.
> 
> +  If MacString is NULL, then ASSERT().
> +
>    Locate simple network protocol associated with the Service Binding Handle
> and
>    get the mac address from SNP. Then convert the mac address into a
> unicode
>    string. It takes 2 unicode characters to represent a 1 byte binary buffer.
>    Plus one unicode character for the null-terminator.
> 
> @@ -1219,10 +1247,12 @@ NetLibGetMacString (
>    );
> 
>  /**
>    Detect media status for specified network device.
> 
> +  If MediaPresent is NULL, then ASSERT().
> +
>    The underlying UNDI driver may or may not support reporting media status
> from
>    GET_STATUS command
> (PXE_STATFLAGS_GET_STATUS_NO_MEDIA_SUPPORTED). This routine
>    will try to invoke Snp->GetStatus() to get the media status. If media is
> already
>    present, it returns directly. If media is not present, it will stop SNP and then
>    restart SNP to get the latest media status. This provides an opportunity to
> get
> @@ -1252,11 +1282,10 @@ NetLibDetectMedia (
>    IN  EFI_HANDLE            ServiceHandle,
>    OUT BOOLEAN               *MediaPresent
>    );
> 
>  /**
> -
>    Detect media state for a network device. This routine will wait for a period
> of time at
>    a specified checking interval when a certain network is under connecting
> until connection
>    process finishes or timeout. If Aip protocol is supported by low layer drivers,
> three kinds
>    of media states can be detected: EFI_SUCCESS, EFI_NOT_READY and
> EFI_NO_MEDIA, represents
>    connected state, connecting state and no media state respectively. When
> function detects
> @@ -1288,10 +1317,12 @@ NetLibDetectMediaWaitTimeout (
> 
> 
>  /**
>    Create an IPv4 device path node.
> 
> +  If Node is NULL, then ASSERT().
> +
>    The header type of IPv4 device path node is MESSAGING_DEVICE_PATH.
>    The header subtype of IPv4 device path node is MSG_IPv4_DP.
>    The length of the IPv4 device path node in bytes is 19.
>    Get other information from parameters to make up the whole IPv4 device
> path node.
> 
> @@ -1319,10 +1350,14 @@ NetLibCreateIPv4DPathNode (
>    );
> 
>  /**
>    Create an IPv6 device path node.
> 
> +  If Node is NULL, then ASSERT().
> +  If LocalIp is NULL, then ASSERT().
> +  If RemoteIp is NULL, then ASSERT().
> +
>    The header type of IPv6 device path node is MESSAGING_DEVICE_PATH.
>    The header subtype of IPv6 device path node is MSG_IPv6_DP.
>    The length of the IPv6 device path node in bytes is 43.
>    Get other information from parameters to make up the whole IPv6 device
> path node.
> 
> @@ -1349,10 +1384,12 @@ NetLibCreateIPv6DPathNode (
> 
> 
>  /**
>    Find the UNDI/SNP handle from controller and protocol GUID.
> 
> +  If ProtocolGuid is NULL, then ASSERT().
> +
>    For example, IP will open an MNP child to transmit/receive
>    packets. When MNP is stopped, IP should also be stopped. IP
>    needs to find its own private data that is related the IP's
>    service binding instance that is installed on the UNDI/SNP handle.
>    The controller is then either an MNP or an ARP child handle. Note that
> @@ -2192,10 +2229,12 @@ NetIpSecNetbufFree (
>    );
> 
>  /**
>    This function obtains the system guid from the smbios table.
> 
> +  If SystemGuid is NULL, then ASSERT().
> +
>    @param[out]  SystemGuid     The pointer of the returned system guid.
> 
>    @retval EFI_SUCCESS         Successfully obtained the system guid.
>    @retval EFI_NOT_FOUND       Did not find the SMBIOS table.
> 
> @@ -2205,11 +2244,14 @@ EFIAPI
>  NetLibGetSystemGuid (
>    OUT EFI_GUID              *SystemGuid
>    );
> 
>  /**
> -  Create Dns QName according the queried domain name.
> +  Create Dns QName according the queried domain name.
> +
> +  If DomainName is NULL, then ASSERT().
> +
>    QName is a domain name represented as a sequence of labels,
>    where each label consists of a length octet followed by that
>    number of octets. The QName terminates with the zero
>    length octet for the null label of the root. Caller should
>    take responsibility to free the buffer in returned pointer.
> diff --git a/MdeModulePkg/Library/DxeNetLib/DxeNetLib.c
> b/MdeModulePkg/Library/DxeNetLib/DxeNetLib.c
> index cbce28f..90d2e3e 100644
> --- a/MdeModulePkg/Library/DxeNetLib/DxeNetLib.c
> +++ b/MdeModulePkg/Library/DxeNetLib/DxeNetLib.c
> @@ -802,11 +802,11 @@ NetIp6IsLinkLocalAddr (
> 
>  /**
>    Check whether the Ipv6 address1 and address2 are on the connected
> network.
> 
>    ASSERT if Ip1 or Ip2 is NULL.
> -  ASSERT if PrefixLength exceeds IP6_PREFIX_MAX.
> +  ASSERT if PrefixLength exceeds or equals to IP6_PREFIX_MAX.
> 
>    @param[in] Ip1          - Ip6 address1, in network order.
>    @param[in] Ip2          - Ip6 address2, in network order.
>    @param[in] PrefixLength - The prefix length of the checking net.
> 
> @@ -824,11 +824,11 @@ NetIp6IsNetEqual (
>  {
>    UINT8 Byte;
>    UINT8 Bit;
>    UINT8 Mask;
> 
> -  ASSERT ((Ip1 != NULL) && (Ip2 != NULL) && (PrefixLength <=
> IP6_PREFIX_MAX));
> +  ASSERT ((Ip1 != NULL) && (Ip2 != NULL) && (PrefixLength <
> IP6_PREFIX_MAX));
> 
>    if (PrefixLength == 0) {
>      return TRUE;
>    }
> 
> @@ -840,10 +840,14 @@ NetIp6IsNetEqual (
>    }
> 
>    if (Bit > 0) {
>      Mask = (UINT8) (0xFF << (8 - Bit));
> 
> +    ASSERT (Byte < 16);
> +    if (Byte >= 16) {
> +      return FALSE;
> +    }
>      if ((Ip1->Addr[Byte] & Mask) != (Ip2->Addr[Byte] & Mask)) {
>        return FALSE;
>      }
>    }
> 
> --
> 1.9.5.msysgit.1