[Patch 0/4] NetworkPkg/DnsDxe: Fix several issues in DnsDxe driver.

[Patch 0/4] NetworkPkg/DnsDxe: Fix several issues in DnsDxe driver.

[Jiaxin Wu]

Cc: Ye Ting <ting.ye@intel.com>
Cc: Fu Siyuan <siyuan.fu@intel.com>
Cc: Wang Fan <fan.wang@intel.com>
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Wu Jiaxin <jiaxin.wu@intel.com>

Jiaxin Wu (4):
  NetworkPkg/DnsDxe: Remove the unnecessary if condition check in DNS.Config
  NetworkPkg/DnsDxe: Update RetryCount/RetryInterval to comply with UEFI spec.
  NetworkPkg/DnsDxe: Fix the potential memory leak issue.
  NetworkPkg/DnsDxe: Avoid to access the freed memory buffer.

 NetworkPkg/DnsDxe/DnsDriver.h   |   4 +-
 NetworkPkg/DnsDxe/DnsImpl.c     | 135 ++++++++++++++++++++++++++++---
 NetworkPkg/DnsDxe/DnsImpl.h     |   5 +-
 NetworkPkg/DnsDxe/DnsProtocol.c | 175 +++++++++++++++++++++++-----------------
 4 files changed, 229 insertions(+), 90 deletions(-)

-- 
1.9.5.msysgit.1

[Patch 1/4] NetworkPkg/DnsDxe: Remove the unnecessary if condition check in DNS.Config

[Jiaxin Wu]

Cc: Ye Ting <ting.ye@intel.com>
Cc: Fu Siyuan <siyuan.fu@intel.com>
Cc: Wang Fan <fan.wang@intel.com>
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Wu Jiaxin <jiaxin.wu@intel.com>
---
 NetworkPkg/DnsDxe/DnsProtocol.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/NetworkPkg/DnsDxe/DnsProtocol.c b/NetworkPkg/DnsDxe/DnsProtocol.c
index bd189ae..7435607 100644
--- a/NetworkPkg/DnsDxe/DnsProtocol.c
+++ b/NetworkPkg/DnsDxe/DnsProtocol.c
@@ -254,11 +254,11 @@ Dns4Configure (
     Status = Dns4CopyConfigure (&Instance->Dns4CfgData, DnsConfigData);
     if (EFI_ERROR (Status)) {
       goto ON_EXIT;
     }
 
-    if (DnsConfigData->DnsServerListCount == 0 || DnsConfigData->DnsServerList == NULL) {
+    if (DnsConfigData->DnsServerListCount == 0) {
       gBS->RestoreTPL (OldTpl); 
       
       //
       // The DNS instance will retrieve DNS server from DHCP Server
       //
@@ -1076,11 +1076,11 @@ Dns6Configure (
     Status = Dns6CopyConfigure (&Instance->Dns6CfgData, DnsConfigData);
     if (EFI_ERROR (Status)) {
       goto ON_EXIT;
     }
 
-    if (DnsConfigData->DnsServerCount == 0 || DnsConfigData->DnsServerList == NULL) {
+    if (DnsConfigData->DnsServerCount == 0) {
       gBS->RestoreTPL (OldTpl);
 
       //
       //The DNS instance will retrieve DNS server from DHCP Server.
       //
-- 
1.9.5.msysgit.1

[Patch 2/4] NetworkPkg/DnsDxe: Update RetryCount/RetryInterval to comply with UEFI spec.

[Jiaxin Wu]

According to UEFI spec:
"Retry number if no response received after RetryInterval. If zero, use
the parameter configured through Dns.Configure() interface."
"Minimum interval of retry is 2 second. If the retry interval is less
than 2 second, then use the 2 second. If zero, use the parameter configured
through Dns.Configure() interface."

For both DNS.HostNameToIp and DNS.GeneralLookUp, the value of RetryCount /
RetryInterval need to be updated to comply with UEFI spec.

Cc: Ye Ting <ting.ye@intel.com>
Cc: Fu Siyuan <siyuan.fu@intel.com>
Cc: Wang Fan <fan.wang@intel.com>
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Wu Jiaxin <jiaxin.wu@intel.com>
---
 NetworkPkg/DnsDxe/DnsDriver.h   |   4 +-
 NetworkPkg/DnsDxe/DnsImpl.c     |   4 +-
 NetworkPkg/DnsDxe/DnsImpl.h     |   5 +-
 NetworkPkg/DnsDxe/DnsProtocol.c | 102 ++++++++++++++++++++++++----------------
 4 files changed, 67 insertions(+), 48 deletions(-)

diff --git a/NetworkPkg/DnsDxe/DnsDriver.h b/NetworkPkg/DnsDxe/DnsDriver.h
index 6632bb2..49f6a1d 100644
--- a/NetworkPkg/DnsDxe/DnsDriver.h
+++ b/NetworkPkg/DnsDxe/DnsDriver.h
@@ -1,9 +1,9 @@
 /** @file
 The header files of the driver binding and service binding protocol for DnsDxe driver.
 
-Copyright (c) 2015 - 2016, Intel Corporation. All rights reserved.<BR>
+Copyright (c) 2015 - 2017, 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
 http://opensource.org/licenses/bsd-license.php
 
@@ -87,12 +87,10 @@ struct _DNS_INSTANCE {
   EFI_IP_ADDRESS                SessionDnsServer;
 
   NET_MAP                       Dns4TxTokens;
   NET_MAP                       Dns6TxTokens;
 
-  UINT32                        MaxRetry;
-
   UDP_IO                        *UdpIo;
 };
 
 typedef struct {
   EFI_SERVICE_BINDING_PROTOCOL  *ServiceBinding;
diff --git a/NetworkPkg/DnsDxe/DnsImpl.c b/NetworkPkg/DnsDxe/DnsImpl.c
index ea3d27d..7c236a0 100644
--- a/NetworkPkg/DnsDxe/DnsImpl.c
+++ b/NetworkPkg/DnsDxe/DnsImpl.c
@@ -1946,11 +1946,11 @@ DnsOnTimerRetransmit (
 
         //
         // Retransmit the packet if haven't reach the maxmium retry count,
         // otherwise exit the transfer.
         //
-        if (++Dns4TokenEntry->Token->RetryCount < Instance->MaxRetry) {
+        if (++Dns4TokenEntry->RetryCounting <= Dns4TokenEntry->Token->RetryCount) {
           DnsRetransmit (Instance, (NET_BUF *)ItemNetMap->Value);
           EntryNetMap = EntryNetMap->ForwardLink;
         } else {
           //
           // Maximum retries reached, clean the Token up.
@@ -1990,11 +1990,11 @@ DnsOnTimerRetransmit (
 
         //
         // Retransmit the packet if haven't reach the maxmium retry count,
         // otherwise exit the transfer.
         //
-        if (++Dns6TokenEntry->Token->RetryCount < Instance->MaxRetry) {
+        if (++Dns6TokenEntry->RetryCounting <= Dns6TokenEntry->Token->RetryCount) {
           DnsRetransmit (Instance, (NET_BUF *) ItemNetMap->Value);
           EntryNetMap = EntryNetMap->ForwardLink;
         } else {
           //
           // Maximum retries reached, clean the Token up.
diff --git a/NetworkPkg/DnsDxe/DnsImpl.h b/NetworkPkg/DnsDxe/DnsImpl.h
index 5fa7f24..3c6296c 100644
--- a/NetworkPkg/DnsDxe/DnsImpl.h
+++ b/NetworkPkg/DnsDxe/DnsImpl.h
@@ -1,9 +1,9 @@
 /** @file
 DnsDxe support functions implementation.
    
-Copyright (c) 2015 - 2016, Intel Corporation. All rights reserved.<BR>
+Copyright (c) 2015 - 2017, 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
 http://opensource.org/licenses/bsd-license.php
 
@@ -84,11 +84,10 @@ extern EFI_DNS6_PROTOCOL             mDns6Protocol;
 #define DNS_STATE_UNCONFIGED     0
 #define DNS_STATE_CONFIGED       1
 #define DNS_STATE_DESTROY        2
 
 #define DNS_DEFAULT_TIMEOUT      2
-#define DNS_DEFAULT_RETRY        3
 
 #define DNS_TIME_TO_GETMAP       5
 
 #pragma pack(1)
 
@@ -113,18 +112,20 @@ typedef struct {
   LIST_ENTRY             AllServerLink;
   EFI_IPv6_ADDRESS       Dns6ServerIp;       
 } DNS6_SERVER_IP;
 
 typedef struct {
+  UINT32                     RetryCounting;
   UINT32                     PacketToLive;
   CHAR16                     *QueryHostName;
   EFI_IPv4_ADDRESS           QueryIpAddress;
   BOOLEAN                    GeneralLookUp;
   EFI_DNS4_COMPLETION_TOKEN  *Token;
 } DNS4_TOKEN_ENTRY;
 
 typedef struct {
+  UINT32                     RetryCounting;
   UINT32                     PacketToLive;
   CHAR16                     *QueryHostName;
   EFI_IPv6_ADDRESS           QueryIpAddress;
   BOOLEAN                    GeneralLookUp;
   EFI_DNS6_COMPLETION_TOKEN  *Token;
diff --git a/NetworkPkg/DnsDxe/DnsProtocol.c b/NetworkPkg/DnsDxe/DnsProtocol.c
index 7435607..df737dc 100644
--- a/NetworkPkg/DnsDxe/DnsProtocol.c
+++ b/NetworkPkg/DnsDxe/DnsProtocol.c
@@ -221,12 +221,10 @@ Dns4Configure (
     //
     if (!NetMapIsEmpty(&Instance->Dns4TxTokens)) {
       Dns4InstanceCancelToken(Instance, NULL);
     }
 
-    Instance->MaxRetry = 0;
-
     if (Instance->UdpIo != NULL){
       UdpIoCleanIo (Instance->UdpIo);
     }
     
     if (Instance->Dns4CfgData.DnsServerList != NULL) {
@@ -375,28 +373,34 @@ Dns4HostNameToIp (
   
   Instance = DNS_INSTANCE_FROM_THIS_PROTOCOL4 (This);
   
   ConfigData = &(Instance->Dns4CfgData);
   
-  Instance->MaxRetry = ConfigData->RetryCount;
-  
-  Token->Status = EFI_NOT_READY;
-  Token->RetryCount = 0;
-  Token->RetryInterval = ConfigData->RetryInterval;
-
   if (Instance->State != DNS_STATE_CONFIGED) {
     Status = EFI_NOT_STARTED;
     goto ON_EXIT;
   }
 
+  Token->Status = EFI_NOT_READY;
+
   //
-  // Check the MaxRetry and RetryInterval values.
+  // If zero, use the parameter configured through Dns.Configure() interface.
   //
-  if (Instance->MaxRetry == 0) {
-    Instance->MaxRetry = DNS_DEFAULT_RETRY;
+  if (Token->RetryCount == 0) {
+    Token->RetryCount = ConfigData->RetryCount;
   }
 
+  //
+  // If zero, use the parameter configured through Dns.Configure() interface.
+  //
+  if (Token->RetryInterval == 0) {
+    Token->RetryInterval = ConfigData->RetryInterval;
+  }
+  
+  //
+  // Minimum interval of retry is 2 second. If the retry interval is less than 2 second, then use the 2 second. 
+  //
   if (Token->RetryInterval < DNS_DEFAULT_TIMEOUT) {
     Token->RetryInterval = DNS_DEFAULT_TIMEOUT;
   }
 
   //
@@ -618,29 +622,35 @@ Dns4GeneralLookUp (
   OldTpl   = gBS->RaiseTPL (TPL_CALLBACK);
   
   Instance = DNS_INSTANCE_FROM_THIS_PROTOCOL4 (This);
   
   ConfigData = &(Instance->Dns4CfgData);
-  
-  Instance->MaxRetry = ConfigData->RetryCount;
-  
-  Token->Status = EFI_NOT_READY;
-  Token->RetryCount = 0;
-  Token->RetryInterval = ConfigData->RetryInterval;
 
   if (Instance->State != DNS_STATE_CONFIGED) {
     Status = EFI_NOT_STARTED;
     goto ON_EXIT;
   }
 
+  Token->Status = EFI_NOT_READY;
+  
+  //
+  // If zero, use the parameter configured through Dns.Configure() interface.
   //
-  // Check the MaxRetry and RetryInterval values.
+  if (Token->RetryCount == 0) {
+    Token->RetryCount = ConfigData->RetryCount;
+  }
+  
+  //
+  // If zero, use the parameter configured through Dns.Configure() interface.
   //
-  if (Instance->MaxRetry == 0) {
-    Instance->MaxRetry = DNS_DEFAULT_RETRY;
+  if (Token->RetryInterval == 0) {
+    Token->RetryInterval = ConfigData->RetryInterval;
   }
 
+  //
+  // Minimum interval of retry is 2 second. If the retry interval is less than 2 second, then use the 2 second. 
+  //
   if (Token->RetryInterval < DNS_DEFAULT_TIMEOUT) {
     Token->RetryInterval = DNS_DEFAULT_TIMEOUT;
   }
 
   //
@@ -1050,12 +1060,10 @@ Dns6Configure (
     //
     if (!NetMapIsEmpty(&Instance->Dns6TxTokens)) {
       Dns6InstanceCancelToken(Instance, NULL);
     }
 
-    Instance->MaxRetry = 0;
-
     if (Instance->UdpIo != NULL){
       UdpIoCleanIo (Instance->UdpIo);
     }
 
     if (Instance->Dns6CfgData.DnsServerList != NULL) {
@@ -1201,32 +1209,38 @@ Dns6HostNameToIp (
   OldTpl   = gBS->RaiseTPL (TPL_CALLBACK);
   
   Instance = DNS_INSTANCE_FROM_THIS_PROTOCOL6 (This);
   
   ConfigData = &(Instance->Dns6CfgData);
-  
-  Instance->MaxRetry = ConfigData->RetryCount;
-
-  Token->Status = EFI_NOT_READY;
-  Token->RetryCount = 0;
-  Token->RetryInterval = ConfigData->RetryInterval;
 
   if (Instance->State != DNS_STATE_CONFIGED) {
     Status = EFI_NOT_STARTED;
     goto ON_EXIT;
   }
 
+  Token->Status = EFI_NOT_READY;
+
   //
-  // Check the MaxRetry and RetryInterval values.
+  // If zero, use the parameter configured through Dns.Configure() interface.
   //
-  if (Instance->MaxRetry == 0) {
-    Instance->MaxRetry = DNS_DEFAULT_RETRY;
+  if (Token->RetryCount == 0) {
+    Token->RetryCount = ConfigData->RetryCount;
   }
 
+  //
+  // If zero, use the parameter configured through Dns.Configure() interface.
+  //
+  if (Token->RetryInterval == 0) {
+    Token->RetryInterval = ConfigData->RetryInterval;
+  }
+  
+  //
+  // Minimum interval of retry is 2 second. If the retry interval is less than 2 second, then use the 2 second. 
+  //
   if (Token->RetryInterval < DNS_DEFAULT_TIMEOUT) {
     Token->RetryInterval = DNS_DEFAULT_TIMEOUT;
-  } 
+  }
 
   //
   // Check cache
   //
   if (ConfigData->EnableDnsCache) {
@@ -1449,29 +1463,35 @@ Dns6GeneralLookUp (
   OldTpl   = gBS->RaiseTPL (TPL_CALLBACK);
   
   Instance = DNS_INSTANCE_FROM_THIS_PROTOCOL6 (This);
   
   ConfigData = &(Instance->Dns6CfgData);
-  
-  Instance->MaxRetry = ConfigData->RetryCount;
-  
-  Token->Status = EFI_NOT_READY;
-  Token->RetryCount = 0;
-  Token->RetryInterval = ConfigData->RetryInterval;
 
   if (Instance->State != DNS_STATE_CONFIGED) {
     Status = EFI_NOT_STARTED;
     goto ON_EXIT;
   }
 
+  Token->Status = EFI_NOT_READY;
+  
   //
-  // Check the MaxRetry and RetryInterval values.
+  // If zero, use the parameter configured through Dns.Configure() interface.
   //
-  if (Instance->MaxRetry == 0) {
-    Instance->MaxRetry = DNS_DEFAULT_RETRY;
+  if (Token->RetryCount == 0) {
+    Token->RetryCount = ConfigData->RetryCount;
+  }
+  
+  //
+  // If zero, use the parameter configured through Dns.Configure() interface.
+  //
+  if (Token->RetryInterval == 0) {
+    Token->RetryInterval = ConfigData->RetryInterval;
   }
 
+  //
+  // Minimum interval of retry is 2 second. If the retry interval is less than 2 second, then use the 2 second. 
+  //
   if (Token->RetryInterval < DNS_DEFAULT_TIMEOUT) {
     Token->RetryInterval = DNS_DEFAULT_TIMEOUT;
   }
 
   //
-- 
1.9.5.msysgit.1

[Patch 3/4] NetworkPkg/DnsDxe: Fix the potential memory leak issue.

[Jiaxin Wu]

Cc: Ye Ting <ting.ye@intel.com>
Cc: Fu Siyuan <siyuan.fu@intel.com>
Cc: Wang Fan <fan.wang@intel.com>
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Wu Jiaxin <jiaxin.wu@intel.com>
---
 NetworkPkg/DnsDxe/DnsImpl.c | 131 ++++++++++++++++++++++++++++++++++++++++----
 1 file changed, 121 insertions(+), 10 deletions(-)

diff --git a/NetworkPkg/DnsDxe/DnsImpl.c b/NetworkPkg/DnsDxe/DnsImpl.c
index 7c236a0..7057bfb 100644
--- a/NetworkPkg/DnsDxe/DnsImpl.c
+++ b/NetworkPkg/DnsDxe/DnsImpl.c
@@ -790,10 +790,14 @@ UpdateDns4Cache (
       if (DeleteFlag) {
         //
         // Delete matching DNS Cache entry
         //
         RemoveEntryList (&Item->AllCacheLink);
+
+        FreePool (Item->DnsCache.HostName);
+        FreePool (Item->DnsCache.IpAddress);
+        FreePool (Item);
         
         return EFI_SUCCESS;
       } else if (Override) {
         //
         // Update this one
@@ -817,17 +821,20 @@ UpdateDns4Cache (
   
   InitializeListHead (&NewDnsCache->AllCacheLink);
    
   NewDnsCache->DnsCache.HostName = AllocatePool (StrSize (DnsCacheEntry.HostName));
   if (NewDnsCache->DnsCache.HostName == NULL) { 
+    FreePool (NewDnsCache);
     return EFI_OUT_OF_RESOURCES;
   }
   
   CopyMem (NewDnsCache->DnsCache.HostName, DnsCacheEntry.HostName, StrSize (DnsCacheEntry.HostName));
 
   NewDnsCache->DnsCache.IpAddress = AllocatePool (sizeof (EFI_IPv4_ADDRESS));
-  if (NewDnsCache->DnsCache.IpAddress == NULL) { 
+  if (NewDnsCache->DnsCache.IpAddress == NULL) {
+    FreePool (NewDnsCache->DnsCache.HostName);
+    FreePool (NewDnsCache);
     return EFI_OUT_OF_RESOURCES;
   }
 
   CopyMem (NewDnsCache->DnsCache.IpAddress, DnsCacheEntry.IpAddress, sizeof (EFI_IPv4_ADDRESS));
 
@@ -882,10 +889,14 @@ UpdateDns6Cache (
         //
         // Delete matching DNS Cache entry
         //
         RemoveEntryList (&Item->AllCacheLink);
         
+        FreePool (Item->DnsCache.HostName);
+        FreePool (Item->DnsCache.IpAddress);
+        FreePool (Item);
+        
         return EFI_SUCCESS;
       } else if (Override) {
         //
         // Update this one
         //
@@ -908,17 +919,20 @@ UpdateDns6Cache (
   
   InitializeListHead (&NewDnsCache->AllCacheLink);
    
   NewDnsCache->DnsCache.HostName = AllocatePool (StrSize (DnsCacheEntry.HostName));
   if (NewDnsCache->DnsCache.HostName == NULL) { 
+    FreePool (NewDnsCache);
     return EFI_OUT_OF_RESOURCES;
   }
   
   CopyMem (NewDnsCache->DnsCache.HostName, DnsCacheEntry.HostName, StrSize (DnsCacheEntry.HostName));
 
   NewDnsCache->DnsCache.IpAddress = AllocatePool (sizeof (EFI_IPv6_ADDRESS));
-  if (NewDnsCache->DnsCache.IpAddress == NULL) { 
+  if (NewDnsCache->DnsCache.IpAddress == NULL) {
+    FreePool (NewDnsCache->DnsCache.HostName);
+    FreePool (NewDnsCache);
     return EFI_OUT_OF_RESOURCES;
   }
   
   CopyMem (NewDnsCache->DnsCache.IpAddress, DnsCacheEntry.IpAddress, sizeof (EFI_IPv6_ADDRESS));
 
@@ -1254,31 +1268,31 @@ ParseDnsResponse (
 
     if (Dns4TokenEntry->GeneralLookUp) {
       //
       // It's the GeneralLookUp querying.
       //
-      Dns4TokenEntry->Token->RspData.GLookupData = AllocatePool (sizeof (DNS_RESOURCE_RECORD));
+      Dns4TokenEntry->Token->RspData.GLookupData = AllocateZeroPool (sizeof (DNS_RESOURCE_RECORD));
       if (Dns4TokenEntry->Token->RspData.GLookupData == NULL) {
         Status = EFI_OUT_OF_RESOURCES;
         goto ON_EXIT;
       }
-      Dns4TokenEntry->Token->RspData.GLookupData->RRList = AllocatePool (DnsHeader->AnswersNum * sizeof (DNS_RESOURCE_RECORD));
+      Dns4TokenEntry->Token->RspData.GLookupData->RRList = AllocateZeroPool (DnsHeader->AnswersNum * sizeof (DNS_RESOURCE_RECORD));
       if (Dns4TokenEntry->Token->RspData.GLookupData->RRList == NULL) {
         Status = EFI_OUT_OF_RESOURCES;
         goto ON_EXIT;
       }
     } else {
       //
       // It's not the GeneralLookUp querying. Check the Query type.
       //
       if (QuerySection->Type == DNS_TYPE_A) {
-        Dns4TokenEntry->Token->RspData.H2AData = AllocatePool (sizeof (DNS_HOST_TO_ADDR_DATA));
+        Dns4TokenEntry->Token->RspData.H2AData = AllocateZeroPool (sizeof (DNS_HOST_TO_ADDR_DATA));
         if (Dns4TokenEntry->Token->RspData.H2AData == NULL) {
           Status = EFI_OUT_OF_RESOURCES;
           goto ON_EXIT;
         }
-        Dns4TokenEntry->Token->RspData.H2AData->IpList = AllocatePool (DnsHeader->AnswersNum * sizeof (EFI_IPv4_ADDRESS));
+        Dns4TokenEntry->Token->RspData.H2AData->IpList = AllocateZeroPool (DnsHeader->AnswersNum * sizeof (EFI_IPv4_ADDRESS));
         if (Dns4TokenEntry->Token->RspData.H2AData->IpList == NULL) {
           Status = EFI_OUT_OF_RESOURCES;
           goto ON_EXIT;
         }
       } else {
@@ -1291,31 +1305,31 @@ ParseDnsResponse (
 
     if (Dns6TokenEntry->GeneralLookUp) {
       //
       // It's the GeneralLookUp querying.
       //
-      Dns6TokenEntry->Token->RspData.GLookupData = AllocatePool (sizeof (DNS_RESOURCE_RECORD));
+      Dns6TokenEntry->Token->RspData.GLookupData = AllocateZeroPool (sizeof (DNS_RESOURCE_RECORD));
       if (Dns6TokenEntry->Token->RspData.GLookupData == NULL) {
         Status = EFI_OUT_OF_RESOURCES;
         goto ON_EXIT;
       }
-      Dns6TokenEntry->Token->RspData.GLookupData->RRList = AllocatePool (DnsHeader->AnswersNum * sizeof (DNS_RESOURCE_RECORD));
+      Dns6TokenEntry->Token->RspData.GLookupData->RRList = AllocateZeroPool (DnsHeader->AnswersNum * sizeof (DNS_RESOURCE_RECORD));
       if (Dns6TokenEntry->Token->RspData.GLookupData->RRList == NULL) {
         Status = EFI_OUT_OF_RESOURCES;
         goto ON_EXIT;
       }
     } else {
       //
       // It's not the GeneralLookUp querying. Check the Query type.
       //
       if (QuerySection->Type == DNS_TYPE_AAAA) {
-        Dns6TokenEntry->Token->RspData.H2AData = AllocatePool (sizeof (DNS6_HOST_TO_ADDR_DATA));
+        Dns6TokenEntry->Token->RspData.H2AData = AllocateZeroPool (sizeof (DNS6_HOST_TO_ADDR_DATA));
         if (Dns6TokenEntry->Token->RspData.H2AData == NULL) {
           Status = EFI_OUT_OF_RESOURCES;
           goto ON_EXIT;
         }
-        Dns6TokenEntry->Token->RspData.H2AData->IpList = AllocatePool (DnsHeader->AnswersNum * sizeof (EFI_IPv6_ADDRESS));
+        Dns6TokenEntry->Token->RspData.H2AData->IpList = AllocateZeroPool (DnsHeader->AnswersNum * sizeof (EFI_IPv6_ADDRESS));
         if (Dns6TokenEntry->Token->RspData.H2AData->IpList == NULL) {
           Status = EFI_OUT_OF_RESOURCES;
           goto ON_EXIT;
         }
       } else {
@@ -1601,10 +1615,101 @@ ON_COMPLETE:
       DispatchDpc ();
     }
   }
 
 ON_EXIT:
+  //
+  // Free the allocated buffer if error happen.
+  //
+  if (EFI_ERROR (Status)) {
+    if (Dns4TokenEntry != NULL) {
+      if (Dns4TokenEntry->GeneralLookUp) {
+        if (Dns4TokenEntry->Token->RspData.GLookupData != NULL) {
+          if (Dns4TokenEntry->Token->RspData.GLookupData->RRList != NULL) {
+            while (RRCount != 0) {
+              RRCount --;
+              if (Dns4TokenEntry->Token->RspData.GLookupData->RRList[RRCount].QName != NULL) {
+                FreePool (Dns4TokenEntry->Token->RspData.GLookupData->RRList[RRCount].QName);
+              }
+
+              if (Dns4TokenEntry->Token->RspData.GLookupData->RRList[RRCount].RData != NULL) {
+                FreePool (Dns4TokenEntry->Token->RspData.GLookupData->RRList[RRCount].RData);
+              }
+            }
+            
+            FreePool (Dns4TokenEntry->Token->RspData.GLookupData->RRList);
+          }
+          
+          FreePool (Dns4TokenEntry->Token->RspData.GLookupData);
+        }
+      } else {
+        if (QuerySection->Type == DNS_TYPE_A && Dns4TokenEntry->Token->RspData.H2AData != NULL) {
+          if (Dns4TokenEntry->Token->RspData.H2AData->IpList != NULL) {
+            FreePool (Dns4TokenEntry->Token->RspData.H2AData->IpList);
+          }
+          
+          FreePool (Dns4TokenEntry->Token->RspData.H2AData);
+        }
+      }
+    }
+
+    if (Dns6TokenEntry != NULL) {
+      if (Dns6TokenEntry->GeneralLookUp) {
+        if (Dns6TokenEntry->Token->RspData.GLookupData != NULL) {
+          if (Dns6TokenEntry->Token->RspData.GLookupData->RRList != NULL) {
+            while (RRCount != 0) {
+              RRCount --;
+              if (Dns6TokenEntry->Token->RspData.GLookupData->RRList[RRCount].QName != NULL) {
+                FreePool (Dns6TokenEntry->Token->RspData.GLookupData->RRList[RRCount].QName);
+              }
+
+              if (Dns6TokenEntry->Token->RspData.GLookupData->RRList[RRCount].RData != NULL) {
+                FreePool (Dns6TokenEntry->Token->RspData.GLookupData->RRList[RRCount].RData);
+              }
+            }
+            
+            FreePool (Dns6TokenEntry->Token->RspData.GLookupData->RRList);
+          }
+          
+          FreePool (Dns6TokenEntry->Token->RspData.GLookupData);
+        }
+      } else {
+        if (QuerySection->Type == DNS_TYPE_AAAA && Dns6TokenEntry->Token->RspData.H2AData != NULL) {
+          if (Dns6TokenEntry->Token->RspData.H2AData->IpList != NULL) {
+            FreePool (Dns6TokenEntry->Token->RspData.H2AData->IpList);
+          }
+          
+          FreePool (Dns6TokenEntry->Token->RspData.H2AData);
+        }
+      }
+    }
+
+    if (Dns4CacheEntry != NULL) {
+      if (Dns4CacheEntry->HostName != NULL) {
+        FreePool (Dns4CacheEntry->HostName);
+      }
+
+      if (Dns4CacheEntry->IpAddress != NULL) {
+        FreePool (Dns4CacheEntry->IpAddress);
+      }
+
+      FreePool (Dns4CacheEntry);
+    }
+
+    if (Dns6CacheEntry != NULL) {
+      if (Dns6CacheEntry->HostName != NULL) {
+        FreePool (Dns6CacheEntry->HostName);
+      }
+
+      if (Dns6CacheEntry->IpAddress != NULL) {
+        FreePool (Dns6CacheEntry->IpAddress);
+      }
+
+      FreePool (Dns6CacheEntry);
+    }    
+  }
+  
   gBS->RestoreTPL (OldTpl);
   return Status;
 }
 
 /**
@@ -2051,10 +2156,13 @@ DnsOnTimerUpdate (
   Entry = mDriverData->Dns4CacheList.ForwardLink;
   while (Entry != &mDriverData->Dns4CacheList) {
     Item4 = NET_LIST_USER_STRUCT (Entry, DNS4_CACHE, AllCacheLink);
     if (Item4->DnsCache.Timeout == 0) {
       RemoveEntryList (&Item4->AllCacheLink);
+      FreePool (Item4->DnsCache.HostName);
+      FreePool (Item4->DnsCache.IpAddress);
+      FreePool (Item4);
       Entry = mDriverData->Dns4CacheList.ForwardLink;
     } else {
       Entry = Entry->ForwardLink;
     }
   }
@@ -2070,10 +2178,13 @@ DnsOnTimerUpdate (
   Entry = mDriverData->Dns6CacheList.ForwardLink;
   while (Entry != &mDriverData->Dns6CacheList) {
     Item6 = NET_LIST_USER_STRUCT (Entry, DNS6_CACHE, AllCacheLink);
     if (Item6->DnsCache.Timeout == 0) {
       RemoveEntryList (&Item6->AllCacheLink);
+      FreePool (Item6->DnsCache.HostName);
+      FreePool (Item6->DnsCache.IpAddress);
+      FreePool (Item6);
       Entry = mDriverData->Dns6CacheList.ForwardLink;
     } else {
       Entry = Entry->ForwardLink;
     }
   }
-- 
1.9.5.msysgit.1

[Patch 4/4] NetworkPkg/DnsDxe: Avoid to access the freed memory buffer.

[Jiaxin Wu]

The HostNameToIp() is a asynchronous function, so the caller
may free the HostName buffer immediately once HostNameToIp()
is returned. Then DNS driver may access the freed memory buffer
later.

This patch is to fix above issue.

Cc: Ye Ting <ting.ye@intel.com>
Cc: Fu Siyuan <siyuan.fu@intel.com>
Cc: Wang Fan <fan.wang@intel.com>
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Wu Jiaxin <jiaxin.wu@intel.com>
---
 NetworkPkg/DnsDxe/DnsProtocol.c | 69 +++++++++++++++++++++++------------------
 1 file changed, 39 insertions(+), 30 deletions(-)

diff --git a/NetworkPkg/DnsDxe/DnsProtocol.c b/NetworkPkg/DnsDxe/DnsProtocol.c
index df737dc..1fcaabd 100644
--- a/NetworkPkg/DnsDxe/DnsProtocol.c
+++ b/NetworkPkg/DnsDxe/DnsProtocol.c
@@ -462,13 +462,19 @@ Dns4HostNameToIp (
     Status = EFI_OUT_OF_RESOURCES;
     goto ON_EXIT;
   }
   
   TokenEntry->PacketToLive = Token->RetryInterval;
-  TokenEntry->QueryHostName = HostName;
   TokenEntry->Token = Token;
-
+  TokenEntry->QueryHostName = AllocateZeroPool (StrSize (HostName));
+  if (TokenEntry->QueryHostName == NULL) {
+    Status = EFI_OUT_OF_RESOURCES;
+    goto ON_EXIT;
+  }
+  
+  CopyMem (TokenEntry->QueryHostName, HostName, StrSize (HostName));
+  
   //
   // Construct QName.
   //
   QueryName = NetLibCreateDnsQName (TokenEntry->QueryHostName);
   if (QueryName == NULL) {
@@ -478,49 +484,48 @@ Dns4HostNameToIp (
   
   //
   // Construct DNS Query Packet.
   //
   Status = ConstructDNSQuery (Instance, QueryName, DNS_TYPE_A, DNS_CLASS_INET, &Packet);
-  if (EFI_ERROR (Status)) {
-    if (TokenEntry != NULL) {
-      FreePool (TokenEntry);
-    }
-    
+  if (EFI_ERROR (Status)) { 
     goto ON_EXIT;
   }
 
   ASSERT (Packet != NULL);
 
   //
   // Save the token into the Dns4TxTokens map.
   //
   Status = NetMapInsertTail (&Instance->Dns4TxTokens, TokenEntry, Packet);
   if (EFI_ERROR (Status)) {
-    if (TokenEntry != NULL) {
-      FreePool (TokenEntry);
-    }
-    
-    NetbufFree (Packet);
-    
     goto ON_EXIT;
   }
   
   //
   // Dns Query Ip
   //
   Status = DoDnsQuery (Instance, Packet);
   if (EFI_ERROR (Status)) {
     Dns4RemoveTokenEntry (&Instance->Dns4TxTokens, TokenEntry);
+  }
+  
+ON_EXIT:
 
+  if (EFI_ERROR (Status)) {
     if (TokenEntry != NULL) {
+      if (TokenEntry->QueryHostName != NULL) {
+        FreePool (TokenEntry->QueryHostName);
+      }
+      
       FreePool (TokenEntry);
     }
     
-    NetbufFree (Packet);
+    if (Packet != NULL) {
+      NetbufFree (Packet);
+    }
   }
   
-ON_EXIT:
   if (QueryName != NULL) {
     FreePool (QueryName);
   }
   
   gBS->RestoreTPL (OldTpl);
@@ -1299,13 +1304,18 @@ Dns6HostNameToIp (
     Status = EFI_OUT_OF_RESOURCES;
     goto ON_EXIT;
   }
   
   TokenEntry->PacketToLive = Token->RetryInterval;
-  TokenEntry->QueryHostName = HostName;
   TokenEntry->Token = Token;
-
+  TokenEntry->QueryHostName = AllocateZeroPool (StrSize (HostName));
+  if (TokenEntry->QueryHostName == NULL) {
+    Status = EFI_OUT_OF_RESOURCES;
+    goto ON_EXIT;
+  }
+  
+  CopyMem (TokenEntry->QueryHostName, HostName, StrSize (HostName));
 
   //
   // Construct QName.
   //
   QueryName = NetLibCreateDnsQName (TokenEntry->QueryHostName);
@@ -1317,48 +1327,47 @@ Dns6HostNameToIp (
   //
   // Construct DNS Query Packet.
   //
   Status = ConstructDNSQuery (Instance, QueryName, DNS_TYPE_AAAA, DNS_CLASS_INET, &Packet);
   if (EFI_ERROR (Status)) {
-    if (TokenEntry != NULL) {
-      FreePool (TokenEntry);
-    }
-    
     goto ON_EXIT;
   }
 
   ASSERT (Packet != NULL);
 
   //
   // Save the token into the Dns6TxTokens map.
   //
   Status = NetMapInsertTail (&Instance->Dns6TxTokens, TokenEntry, Packet);
   if (EFI_ERROR (Status)) {
-    if (TokenEntry != NULL) {
-      FreePool (TokenEntry);
-    }
-    
-    NetbufFree (Packet);
-    
     goto ON_EXIT;
   }
   
   //
   // Dns Query Ip
   //
   Status = DoDnsQuery (Instance, Packet);
   if (EFI_ERROR (Status)) {
     Dns6RemoveTokenEntry (&Instance->Dns6TxTokens, TokenEntry);
-    
+  }
+  
+ON_EXIT:
+
+  if (EFI_ERROR (Status)) {
     if (TokenEntry != NULL) {
+      if (TokenEntry->QueryHostName != NULL) {
+        FreePool (TokenEntry->QueryHostName);
+      }
+      
       FreePool (TokenEntry);
     }
     
-    NetbufFree (Packet);
+    if (Packet != NULL) {
+      NetbufFree (Packet);
+    }
   }
   
-ON_EXIT:
   if (QueryName != NULL) {
     FreePool (QueryName);
   }
   
   gBS->RestoreTPL (OldTpl);
-- 
1.9.5.msysgit.1

Re: [Patch 0/4] NetworkPkg/DnsDxe: Fix several issues in DnsDxe driver.

[Fu, Siyuan]

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



> -----Original Message-----
> From: Wu, Jiaxin
> Sent: Tuesday, December 5, 2017 2:59 PM
> To: edk2-devel@lists.01.org
> Cc: Ye, Ting <ting.ye@intel.com>; Fu, Siyuan <siyuan.fu@intel.com>; Wang,
> Fan <fan.wang@intel.com>; Wu, Jiaxin <jiaxin.wu@intel.com>
> Subject: [Patch 0/4] NetworkPkg/DnsDxe: Fix several issues in DnsDxe
> driver.
> 
> Cc: Ye Ting <ting.ye@intel.com>
> Cc: Fu Siyuan <siyuan.fu@intel.com>
> Cc: Wang Fan <fan.wang@intel.com>
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Wu Jiaxin <jiaxin.wu@intel.com>
> 
> Jiaxin Wu (4):
>   NetworkPkg/DnsDxe: Remove the unnecessary if condition check in
> DNS.Config
>   NetworkPkg/DnsDxe: Update RetryCount/RetryInterval to comply with UEFI
> spec.
>   NetworkPkg/DnsDxe: Fix the potential memory leak issue.
>   NetworkPkg/DnsDxe: Avoid to access the freed memory buffer.
> 
>  NetworkPkg/DnsDxe/DnsDriver.h   |   4 +-
>  NetworkPkg/DnsDxe/DnsImpl.c     | 135 ++++++++++++++++++++++++++++---
>  NetworkPkg/DnsDxe/DnsImpl.h     |   5 +-
>  NetworkPkg/DnsDxe/DnsProtocol.c | 175 +++++++++++++++++++++++------------
> -----
>  4 files changed, 229 insertions(+), 90 deletions(-)
> 
> --
> 1.9.5.msysgit.1