[edk2] [Patch 1/2] NetworkPkg: Code logic optimization for DnsDxe and HttpDxe driver

[Wang Fan]

Revise some errors which may lead NULL pointer be dereferenced:
* DnsDhcp.c: Paralist may be used without any initialized
* DnsHeader and RcvString may be null but be dereferenced
* HttpDriver.c: revise an if judgment
* HttpImpl.c: add a judgment to avoid null dereferenced

Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: fanwang2 <***@intel.com>
---
NetworkPkg/DnsDxe/DnsDhcp.c | 2 ++
NetworkPkg/DnsDxe/DnsImpl.c | 4 +++-
NetworkPkg/HttpDxe/HttpDriver.c | 2 +-
NetworkPkg/HttpDxe/HttpImpl.c | 4 ++++
4 files changed, 10 insertions(+), 2 deletions(-)

diff --git a/NetworkPkg/DnsDxe/DnsDhcp.c b/NetworkPkg/DnsDxe/DnsDhcp.c
index 4607f7b..1cc337f 100644
--- a/NetworkPkg/DnsDxe/DnsDhcp.c
+++ b/NetworkPkg/DnsDxe/DnsDhcp.c
@@ -299,10 +299,12 @@ GetDns4ServerFromDhcp4 (
Ip4Config2 = NULL;
DataSize = 0;
Data = NULL;
InterfaceInfo = NULL;

+ ZeroMem ((UINT8 *) ParaList, sizeof (ParaList));
+
ZeroMem (&MnpConfigData, sizeof (EFI_MANAGED_NETWORK_CONFIG_DATA));

ZeroMem (&DnsServerInfor, sizeof (DNS4_SERVER_INFOR));

ZeroMem (&Token, sizeof (EFI_DHCP4_TRANSMIT_RECEIVE_TOKEN));
diff --git a/NetworkPkg/DnsDxe/DnsImpl.c b/NetworkPkg/DnsDxe/DnsImpl.c
index c227093..b196d18 100644
--- a/NetworkPkg/DnsDxe/DnsImpl.c
+++ b/NetworkPkg/DnsDxe/DnsImpl.c
@@ -1047,10 +1047,11 @@ IsValidDnsResponse (
if (Packet == NULL){

continue;
} else {
TxString = NetbufGetByte (Packet, 0, NULL);
+ ASSERT (TxString != NULL);
DnsHeader = (DNS_HEADER *) TxString;
QueryName = (CHAR8 *) (TxString + sizeof (*DnsHeader));
QuerySection = (DNS_QUERY_SECTION *) (QueryName + AsciiStrLen (QueryName) + 1);

DnsHeader->Identification = NTOHS (DnsHeader->Identification);
@@ -1424,11 +1425,12 @@ DnsOnPacketReceived (
}

ASSERT (Packet != NULL);

RcvString = NetbufGetByte (Packet, 0, NULL);
-
+ ASSERT (RcvString != NULL);
+
//
// Parse Dns Response
//
ParseDnsResponse (Instance, RcvString, &Completed);

diff --git a/NetworkPkg/HttpDxe/HttpDriver.c b/NetworkPkg/HttpDxe/HttpDriver.c
index b65607a..43f42e2 100644
--- a/NetworkPkg/HttpDxe/HttpDriver.c
+++ b/NetworkPkg/HttpDxe/HttpDriver.c
@@ -77,11 +77,11 @@ HttpCreateService (
VOID
HttpCleanService (
IN HTTP_SERVICE *HttpService
)
{
- if (HttpService != NULL) {
+ if (HttpService == NULL) {
return ;
}

if (HttpService->TcpChildHandle != NULL) {
gBS->CloseProtocol (
diff --git a/NetworkPkg/HttpDxe/HttpImpl.c b/NetworkPkg/HttpDxe/HttpImpl.c
index 4bd4ac8..545fe42 100644
--- a/NetworkPkg/HttpDxe/HttpImpl.c
+++ b/NetworkPkg/HttpDxe/HttpImpl.c
@@ -764,10 +764,14 @@ HttpResponseWorker (
EFI_HTTP_TOKEN *Token;
NET_MAP_ITEM *Item;
HTTP_TOKEN_WRAP *ValueInItem;
UINTN HdrLen;

+ if (Wrap == NULL || Wrap->HttpInstance == NULL) {
+ return EFI_INVALID_PARAMETER;
+ }
+
HttpInstance = Wrap->HttpInstance;
Token = Wrap->HttpToken;

HttpMsg = Token->Message;

[Ye, Ting]

Looks good.
Reviewed-by: Ye Ting <***@intel.com>

-----Original Message-----
From: Wang Fan [mailto:***@intel.com]
Sent: Friday, July 10, 2015 4:19 PM
To: Wu, Jiaxin; Fu, Siyuan; edk2-***@lists.sourceforge.net
Subject: [edk2] [Patch 1/2] NetworkPkg: Code logic optimization for DnsDxe and HttpDxe driver

Revise some errors which may lead NULL pointer be dereferenced:
* DnsDhcp.c: Paralist may be used without any initialized
* DnsHeader and RcvString may be null but be dereferenced
* HttpDriver.c: revise an if judgment
* HttpImpl.c: add a judgment to avoid null dereferenced

Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: fanwang2 <***@intel.com>
---
NetworkPkg/DnsDxe/DnsDhcp.c | 2 ++
NetworkPkg/DnsDxe/DnsImpl.c | 4 +++-
NetworkPkg/HttpDxe/HttpDriver.c | 2 +-
NetworkPkg/HttpDxe/HttpImpl.c | 4 ++++
4 files changed, 10 insertions(+), 2 deletions(-)

diff --git a/NetworkPkg/DnsDxe/DnsDhcp.c b/NetworkPkg/DnsDxe/DnsDhcp.c index 4607f7b..1cc337f 100644
--- a/NetworkPkg/DnsDxe/DnsDhcp.c
+++ b/NetworkPkg/DnsDxe/DnsDhcp.c
@@ -299,10 +299,12 @@ GetDns4ServerFromDhcp4 (
Ip4Config2 = NULL;
DataSize = 0;
Data = NULL;
InterfaceInfo = NULL;

+ ZeroMem ((UINT8 *) ParaList, sizeof (ParaList));
+
ZeroMem (&MnpConfigData, sizeof (EFI_MANAGED_NETWORK_CONFIG_DATA));

ZeroMem (&DnsServerInfor, sizeof (DNS4_SERVER_INFOR));

ZeroMem (&Token, sizeof (EFI_DHCP4_TRANSMIT_RECEIVE_TOKEN));
diff --git a/NetworkPkg/DnsDxe/DnsImpl.c b/NetworkPkg/DnsDxe/DnsImpl.c index c227093..b196d18 100644
--- a/NetworkPkg/DnsDxe/DnsImpl.c
+++ b/NetworkPkg/DnsDxe/DnsImpl.c
@@ -1047,10 +1047,11 @@ IsValidDnsResponse (
if (Packet == NULL){

continue;
} else {
TxString = NetbufGetByte (Packet, 0, NULL);
+ ASSERT (TxString != NULL);
DnsHeader = (DNS_HEADER *) TxString;
QueryName = (CHAR8 *) (TxString + sizeof (*DnsHeader));
QuerySection = (DNS_QUERY_SECTION *) (QueryName + AsciiStrLen (QueryName) + 1);

DnsHeader->Identification = NTOHS (DnsHeader->Identification); @@ -1424,11 +1425,12 @@ DnsOnPacketReceived (
}

ASSERT (Packet != NULL);

RcvString = NetbufGetByte (Packet, 0, NULL);
-
+ ASSERT (RcvString != NULL);
+
//
// Parse Dns Response
//
ParseDnsResponse (Instance, RcvString, &Completed);

diff --git a/NetworkPkg/HttpDxe/HttpDriver.c b/NetworkPkg/HttpDxe/HttpDriver.c index b65607a..43f42e2 100644
--- a/NetworkPkg/HttpDxe/HttpDriver.c
+++ b/NetworkPkg/HttpDxe/HttpDriver.c
@@ -77,11 +77,11 @@ HttpCreateService (
VOID
HttpCleanService (
IN HTTP_SERVICE *HttpService
)
{
- if (HttpService != NULL) {
+ if (HttpService == NULL) {
return ;
}

if (HttpService->TcpChildHandle != NULL) {
gBS->CloseProtocol (
diff --git a/NetworkPkg/HttpDxe/HttpImpl.c b/NetworkPkg/HttpDxe/HttpImpl.c index 4bd4ac8..545fe42 100644
--- a/NetworkPkg/HttpDxe/HttpImpl.c
+++ b/NetworkPkg/HttpDxe/HttpImpl.c
@@ -764,10 +764,14 @@ HttpResponseWorker (
EFI_HTTP_TOKEN *Token;
NET_MAP_ITEM *Item;
HTTP_TOKEN_WRAP *ValueInItem;
UINTN HdrLen;

+ if (Wrap == NULL || Wrap->HttpInstance == NULL) {
+ return EFI_INVALID_PARAMETER;
+ }
+
HttpInstance = Wrap->HttpInstance;
Token = Wrap->HttpToken;

HttpMsg = Token->Message;

--
1.9.5.msysgit.1



------------------------------------------------------------------------------
Don't Limit Your Business. Reach for the Cloud.
GigeNET's Cloud Solutions provide you with the tools and support that
you need to offload your IT needs and focus on growing your business.
Configured For All Businesses. Start Your Cloud Today.
https://www.gigenetcloud.com/
_______________________________________________
edk2-devel mailing list
edk2-***@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/edk2-devel

[Wu, Jiaxin]

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

-----Original Message-----
From: Wang, Fan
Sent: Friday, July 10, 2015 4:19 PM
To: Wu, Jiaxin; Fu, Siyuan; edk2-***@lists.sourceforge.net
Cc: Wang, Fan
Subject: [Patch 1/2] NetworkPkg: Code logic optimization for DnsDxe and HttpDxe driver

Revise some errors which may lead NULL pointer be dereferenced:
* DnsDhcp.c: Paralist may be used without any initialized
* DnsHeader and RcvString may be null but be dereferenced
* HttpDriver.c: revise an if judgment
* HttpImpl.c: add a judgment to avoid null dereferenced

Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: fanwang2 <***@intel.com>
---
NetworkPkg/DnsDxe/DnsDhcp.c | 2 ++
NetworkPkg/DnsDxe/DnsImpl.c | 4 +++-
NetworkPkg/HttpDxe/HttpDriver.c | 2 +-
NetworkPkg/HttpDxe/HttpImpl.c | 4 ++++
4 files changed, 10 insertions(+), 2 deletions(-)

diff --git a/NetworkPkg/DnsDxe/DnsDhcp.c b/NetworkPkg/DnsDxe/DnsDhcp.c index 4607f7b..1cc337f 100644
--- a/NetworkPkg/DnsDxe/DnsDhcp.c
+++ b/NetworkPkg/DnsDxe/DnsDhcp.c
@@ -299,10 +299,12 @@ GetDns4ServerFromDhcp4 (
Ip4Config2 = NULL;
DataSize = 0;
Data = NULL;
InterfaceInfo = NULL;

+ ZeroMem ((UINT8 *) ParaList, sizeof (ParaList));
+
ZeroMem (&MnpConfigData, sizeof (EFI_MANAGED_NETWORK_CONFIG_DATA));

ZeroMem (&DnsServerInfor, sizeof (DNS4_SERVER_INFOR));

ZeroMem (&Token, sizeof (EFI_DHCP4_TRANSMIT_RECEIVE_TOKEN));
diff --git a/NetworkPkg/DnsDxe/DnsImpl.c b/NetworkPkg/DnsDxe/DnsImpl.c index c227093..b196d18 100644
--- a/NetworkPkg/DnsDxe/DnsImpl.c
+++ b/NetworkPkg/DnsDxe/DnsImpl.c
@@ -1047,10 +1047,11 @@ IsValidDnsResponse (
if (Packet == NULL){

continue;
} else {
TxString = NetbufGetByte (Packet, 0, NULL);
+ ASSERT (TxString != NULL);
DnsHeader = (DNS_HEADER *) TxString;
QueryName = (CHAR8 *) (TxString + sizeof (*DnsHeader));
QuerySection = (DNS_QUERY_SECTION *) (QueryName + AsciiStrLen (QueryName) + 1);

DnsHeader->Identification = NTOHS (DnsHeader->Identification); @@ -1424,11 +1425,12 @@ DnsOnPacketReceived (
}

ASSERT (Packet != NULL);

RcvString = NetbufGetByte (Packet, 0, NULL);
-
+ ASSERT (RcvString != NULL);
+
//
// Parse Dns Response
//
ParseDnsResponse (Instance, RcvString, &Completed);

diff --git a/NetworkPkg/HttpDxe/HttpDriver.c b/NetworkPkg/HttpDxe/HttpDriver.c index b65607a..43f42e2 100644
--- a/NetworkPkg/HttpDxe/HttpDriver.c
+++ b/NetworkPkg/HttpDxe/HttpDriver.c
@@ -77,11 +77,11 @@ HttpCreateService (
VOID
HttpCleanService (
IN HTTP_SERVICE *HttpService
)
{
- if (HttpService != NULL) {
+ if (HttpService == NULL) {
return ;
}

if (HttpService->TcpChildHandle != NULL) {
gBS->CloseProtocol (
diff --git a/NetworkPkg/HttpDxe/HttpImpl.c b/NetworkPkg/HttpDxe/HttpImpl.c index 4bd4ac8..545fe42 100644
--- a/NetworkPkg/HttpDxe/HttpImpl.c
+++ b/NetworkPkg/HttpDxe/HttpImpl.c
@@ -764,10 +764,14 @@ HttpResponseWorker (
EFI_HTTP_TOKEN *Token;
NET_MAP_ITEM *Item;
HTTP_TOKEN_WRAP *ValueInItem;
UINTN HdrLen;

+ if (Wrap == NULL || Wrap->HttpInstance == NULL) {
+ return EFI_INVALID_PARAMETER;
+ }
+
HttpInstance = Wrap->HttpInstance;
Token = Wrap->HttpToken;

HttpMsg = Token->Message;

--
1.9.5.msysgit.1

[Ard Biesheuvel]

Post by Wang Fan
* DnsDhcp.c: Paralist may be used without any initialized
* DnsHeader and RcvString may be null but be dereferenced
* HttpDriver.c: revise an if judgment
* HttpImpl.c: add a judgment to avoid null dereferenced

Same as for 2/2: the patch subject should indicate that it fixes bugs
that may crash the firmware.
'Code logic optimization' suggests nothing of the sort.

Regards,
Ard.

Post by Wang Fan
Contributed-under: TianoCore Contribution Agreement 1.0
---
NetworkPkg/DnsDxe/DnsDhcp.c | 2 ++
NetworkPkg/DnsDxe/DnsImpl.c | 4 +++-
NetworkPkg/HttpDxe/HttpDriver.c | 2 +-
NetworkPkg/HttpDxe/HttpImpl.c | 4 ++++
4 files changed, 10 insertions(+), 2 deletions(-)
diff --git a/NetworkPkg/DnsDxe/DnsDhcp.c b/NetworkPkg/DnsDxe/DnsDhcp.c
index 4607f7b..1cc337f 100644
--- a/NetworkPkg/DnsDxe/DnsDhcp.c
+++ b/NetworkPkg/DnsDxe/DnsDhcp.c
@@ -299,10 +299,12 @@ GetDns4ServerFromDhcp4 (
Ip4Config2 = NULL;
DataSize = 0;
Data = NULL;
InterfaceInfo = NULL;
+ ZeroMem ((UINT8 *) ParaList, sizeof (ParaList));
+
ZeroMem (&MnpConfigData, sizeof (EFI_MANAGED_NETWORK_CONFIG_DATA));
ZeroMem (&DnsServerInfor, sizeof (DNS4_SERVER_INFOR));
ZeroMem (&Token, sizeof (EFI_DHCP4_TRANSMIT_RECEIVE_TOKEN));
diff --git a/NetworkPkg/DnsDxe/DnsImpl.c b/NetworkPkg/DnsDxe/DnsImpl.c
index c227093..b196d18 100644
--- a/NetworkPkg/DnsDxe/DnsImpl.c
+++ b/NetworkPkg/DnsDxe/DnsImpl.c
@@ -1047,10 +1047,11 @@ IsValidDnsResponse (
if (Packet == NULL){
continue;
} else {
TxString = NetbufGetByte (Packet, 0, NULL);
+ ASSERT (TxString != NULL);
DnsHeader = (DNS_HEADER *) TxString;
QueryName = (CHAR8 *) (TxString + sizeof (*DnsHeader));
QuerySection = (DNS_QUERY_SECTION *) (QueryName + AsciiStrLen (QueryName) + 1);
DnsHeader->Identification = NTOHS (DnsHeader->Identification);
@@ -1424,11 +1425,12 @@ DnsOnPacketReceived (
}
ASSERT (Packet != NULL);
RcvString = NetbufGetByte (Packet, 0, NULL);
-
+ ASSERT (RcvString != NULL);
+
//
// Parse Dns Response
//
ParseDnsResponse (Instance, RcvString, &Completed);
diff --git a/NetworkPkg/HttpDxe/HttpDriver.c b/NetworkPkg/HttpDxe/HttpDriver.c
index b65607a..43f42e2 100644
--- a/NetworkPkg/HttpDxe/HttpDriver.c
+++ b/NetworkPkg/HttpDxe/HttpDriver.c
@@ -77,11 +77,11 @@ HttpCreateService (
VOID
HttpCleanService (
IN HTTP_SERVICE *HttpService
)
{
- if (HttpService != NULL) {
+ if (HttpService == NULL) {
return ;
}
if (HttpService->TcpChildHandle != NULL) {
gBS->CloseProtocol (
diff --git a/NetworkPkg/HttpDxe/HttpImpl.c b/NetworkPkg/HttpDxe/HttpImpl.c
index 4bd4ac8..545fe42 100644
--- a/NetworkPkg/HttpDxe/HttpImpl.c
+++ b/NetworkPkg/HttpDxe/HttpImpl.c
@@ -764,10 +764,14 @@ HttpResponseWorker (
EFI_HTTP_TOKEN *Token;
NET_MAP_ITEM *Item;
HTTP_TOKEN_WRAP *ValueInItem;
UINTN HdrLen;
+ if (Wrap == NULL || Wrap->HttpInstance == NULL) {
+ return EFI_INVALID_PARAMETER;
+ }
+
HttpInstance = Wrap->HttpInstance;
Token = Wrap->HttpToken;
HttpMsg = Token->Message;
--
1.9.5.msysgit.1
------------------------------------------------------------------------------
Don't Limit Your Business. Reach for the Cloud.
GigeNET's Cloud Solutions provide you with the tools and support that
you need to offload your IT needs and focus on growing your business.
Configured For All Businesses. Start Your Cloud Today.
https://www.gigenetcloud.com/
_______________________________________________
edk2-devel mailing list
https://lists.sourceforge.net/lists/listinfo/edk2-devel

[Leif Lindholm]

Post by Wang Fan
* DnsDhcp.c: Paralist may be used without any initialized
* DnsHeader and RcvString may be null but be dereferenced
* HttpDriver.c: revise an if judgment
* HttpImpl.c: add a judgment to avoid null dereferenced
Contributed-under: TianoCore Contribution Agreement 1.0
---
NetworkPkg/DnsDxe/DnsDhcp.c | 2 ++
NetworkPkg/DnsDxe/DnsImpl.c | 4 +++-
NetworkPkg/HttpDxe/HttpDriver.c | 2 +-
NetworkPkg/HttpDxe/HttpImpl.c | 4 ++++
4 files changed, 10 insertions(+), 2 deletions(-)
diff --git a/NetworkPkg/DnsDxe/DnsDhcp.c b/NetworkPkg/DnsDxe/DnsDhcp.c
index 4607f7b..1cc337f 100644
--- a/NetworkPkg/DnsDxe/DnsDhcp.c
+++ b/NetworkPkg/DnsDxe/DnsDhcp.c
@@ -299,10 +299,12 @@ GetDns4ServerFromDhcp4 (
Ip4Config2 = NULL;
DataSize = 0;
Data = NULL;
InterfaceInfo = NULL;
+ ZeroMem ((UINT8 *) ParaList, sizeof (ParaList));
+
ZeroMem (&MnpConfigData, sizeof (EFI_MANAGED_NETWORK_CONFIG_DATA));
ZeroMem (&DnsServerInfor, sizeof (DNS4_SERVER_INFOR));
ZeroMem (&Token, sizeof (EFI_DHCP4_TRANSMIT_RECEIVE_TOKEN));
diff --git a/NetworkPkg/DnsDxe/DnsImpl.c b/NetworkPkg/DnsDxe/DnsImpl.c
index c227093..b196d18 100644
--- a/NetworkPkg/DnsDxe/DnsImpl.c
+++ b/NetworkPkg/DnsDxe/DnsImpl.c
@@ -1047,10 +1047,11 @@ IsValidDnsResponse (
if (Packet == NULL){
continue;
} else {
TxString = NetbufGetByte (Packet, 0, NULL);
+ ASSERT (TxString != NULL);

Surely this should be a runtime test as well, or instead?
If NetbufGetByte ever returns NULL, we will pass a bogus pointer down
the chain...

Post by Wang Fan
DnsHeader = (DNS_HEADER *) TxString;
QueryName = (CHAR8 *) (TxString + sizeof (*DnsHeader));
QuerySection = (DNS_QUERY_SECTION *) (QueryName + AsciiStrLen (QueryName) + 1);

... probably causing a crash in AsciiStrLen above, as it attempts to
access (0x0 + sizeof (*DnsHeader)).

Post by Wang Fan
DnsHeader->Identification = NTOHS (DnsHeader->Identification);
@@ -1424,11 +1425,12 @@ DnsOnPacketReceived (
}
ASSERT (Packet != NULL);

Runtime test?

Post by Wang Fan
RcvString = NetbufGetByte (Packet, 0, NULL);
-
+ ASSERT (RcvString != NULL);

Runtime test?

Post by Wang Fan
+
//
// Parse Dns Response
//
ParseDnsResponse (Instance, RcvString, &Completed);
diff --git a/NetworkPkg/HttpDxe/HttpDriver.c b/NetworkPkg/HttpDxe/HttpDriver.c
index b65607a..43f42e2 100644
--- a/NetworkPkg/HttpDxe/HttpDriver.c
+++ b/NetworkPkg/HttpDxe/HttpDriver.c
@@ -77,11 +77,11 @@ HttpCreateService (
VOID
HttpCleanService (
IN HTTP_SERVICE *HttpService
)
{
- if (HttpService != NULL) {
+ if (HttpService == NULL) {
return ;
}
if (HttpService->TcpChildHandle != NULL) {
gBS->CloseProtocol (
diff --git a/NetworkPkg/HttpDxe/HttpImpl.c b/NetworkPkg/HttpDxe/HttpImpl.c
index 4bd4ac8..545fe42 100644
--- a/NetworkPkg/HttpDxe/HttpImpl.c
+++ b/NetworkPkg/HttpDxe/HttpImpl.c
@@ -764,10 +764,14 @@ HttpResponseWorker (
EFI_HTTP_TOKEN *Token;
NET_MAP_ITEM *Item;
HTTP_TOKEN_WRAP *ValueInItem;
UINTN HdrLen;
+ if (Wrap == NULL || Wrap->HttpInstance == NULL) {
+ return EFI_INVALID_PARAMETER;
+ }
+
HttpInstance = Wrap->HttpInstance;
Token = Wrap->HttpToken;
HttpMsg = Token->Message;
--
1.9.5.msysgit.1
------------------------------------------------------------------------------
Don't Limit Your Business. Reach for the Cloud.
GigeNET's Cloud Solutions provide you with the tools and support that
you need to offload your IT needs and focus on growing your business.
Configured For All Businesses. Start Your Cloud Today.
https://www.gigenetcloud.com/
_______________________________________________
edk2-devel mailing list
https://lists.sourceforge.net/lists/listinfo/edk2-devel