[PATCH v1 0/2] Fix the possible NULL pointer dereference issue.

[PATCH v1 0/2] Fix the possible NULL pointer dereference issue.

[Jiaxin Wu]

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

For the NET_LIST_FOR_EACH & NET_LIST_FOR_EACH_SAFE, "Entry" should be
checked whether it's NULL or not instead of using the pointer directly.

Besides, NET_LIST_FOR_EACH_SAFE is defined to iterate through the double
linked list in delete-safe way. It's unnecessary to use this macro if
list entries won't be deleted.

Cc: Wu Hao A <hao.a.wu@intel.com>
Cc: Gao Liming <liming.gao@intel.com>
Cc: Ye Ting <ting.ye@intel.com>
Cc: Fu Siyuan <siyuan.fu@intel.com>
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Wu Jiaxin <jiaxin.wu@intel.com>
Jiaxin Wu (2):
  MdeModulePkg/NetLib.h: Fix the possible NULL pointer dereference issue.
  MdeModulePkg/Dhcp4Dxe: Use NET_LIST_FOR_EACH instead of NET_LIST_FOR_EACH_SAFE.

 MdeModulePkg/Include/Library/NetLib.h             | 6 +++---
 MdeModulePkg/Universal/Network/Dhcp4Dxe/Dhcp4Io.c | 4 ++--
 2 files changed, 5 insertions(+), 5 deletions(-)

-- 
2.17.1.windows.2

[PATCH v1 1/2] MdeModulePkg/NetLib.h: Fix the possible NULL pointer dereference issue.

[Jiaxin Wu]

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

For the NET_LIST_FOR_EACH & NET_LIST_FOR_EACH_SAFE, "Entry" should be
checked whether it's NULL or not instead of using the pointer directly.

Cc: Wu Hao A <hao.a.wu@intel.com>
Cc: Gao Liming <liming.gao@intel.com>
Cc: Ye Ting <ting.ye@intel.com>
Cc: Fu Siyuan <siyuan.fu@intel.com>
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Wu Jiaxin <jiaxin.wu@intel.com>
---
 MdeModulePkg/Include/Library/NetLib.h | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/MdeModulePkg/Include/Library/NetLib.h b/MdeModulePkg/Include/Library/NetLib.h
index 0977973921..5b1307553a 100644
--- a/MdeModulePkg/Include/Library/NetLib.h
+++ b/MdeModulePkg/Include/Library/NetLib.h
@@ -616,21 +616,21 @@ NetRandomInitSeed (
 
 //
 // Iterate through the double linked list. It is NOT delete safe
 //
 #define NET_LIST_FOR_EACH(Entry, ListHead) \
-  for(Entry = (ListHead)->ForwardLink; Entry != (ListHead); Entry = Entry->ForwardLink)
+  for(Entry = (ListHead)->ForwardLink; Entry != (ListHead), Entry != NULL; Entry = Entry->ForwardLink)
 
 //
 // Iterate through the double linked list. This is delete-safe.
 // Don't touch NextEntry. Also, don't use this macro if list
 // entries other than the Entry may be deleted when processing
 // the current Entry.
 //
 #define NET_LIST_FOR_EACH_SAFE(Entry, NextEntry, ListHead) \
-  for(Entry = (ListHead)->ForwardLink, NextEntry = Entry->ForwardLink; \
-      Entry != (ListHead); \
+  for(Entry = (ListHead)->ForwardLink, (Entry != NULL) ? (NextEntry = Entry->ForwardLink) : (Entry = NULL); \
+      Entry != (ListHead), Entry != NULL; \
       Entry = NextEntry, NextEntry = Entry->ForwardLink \
      )
 
 //
 // Make sure the list isn't empty before getting the first/last record.
-- 
2.17.1.windows.2

Re: [PATCH v1 1/2] MdeModulePkg/NetLib.h: Fix the possible NULL pointer dereference issue.

[Laszlo Ersek]

Hi Jiaxin,

On 01/15/19 02:26, Jiaxin Wu wrote:
> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=1456
> 
> For the NET_LIST_FOR_EACH & NET_LIST_FOR_EACH_SAFE, "Entry" should be
> checked whether it's NULL or not instead of using the pointer directly.
> 
> Cc: Wu Hao A <hao.a.wu@intel.com>
> Cc: Gao Liming <liming.gao@intel.com>
> Cc: Ye Ting <ting.ye@intel.com>
> Cc: Fu Siyuan <siyuan.fu@intel.com>
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Wu Jiaxin <jiaxin.wu@intel.com>
> ---
>  MdeModulePkg/Include/Library/NetLib.h | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/MdeModulePkg/Include/Library/NetLib.h b/MdeModulePkg/Include/Library/NetLib.h
> index 0977973921..5b1307553a 100644
> --- a/MdeModulePkg/Include/Library/NetLib.h
> +++ b/MdeModulePkg/Include/Library/NetLib.h
> @@ -616,21 +616,21 @@ NetRandomInitSeed (
>  
>  //
>  // Iterate through the double linked list. It is NOT delete safe
>  //
>  #define NET_LIST_FOR_EACH(Entry, ListHead) \
> -  for(Entry = (ListHead)->ForwardLink; Entry != (ListHead); Entry = Entry->ForwardLink)
> +  for(Entry = (ListHead)->ForwardLink; Entry != (ListHead), Entry != NULL; Entry = Entry->ForwardLink)

(1) The linked list from BaseLib (LIST_ENTRY) always has at least one
element (the head element), and the list is empty if the head element
points back to itself. In other words, ForwardLink may never be NULL.

So why is it necessary to check against that case here?

(2) If the NULL check is indeed necessary for some reason, then we
should write

  Entry != (ListHead) && Entry != NULL

in the controlling expression. Because, with the comma operator, the
(Entry != (ListHead)) expression would be evaluated, but its result
would be ignored.

Thanks,
Laszlo

>  
>  //
>  // Iterate through the double linked list. This is delete-safe.
>  // Don't touch NextEntry. Also, don't use this macro if list
>  // entries other than the Entry may be deleted when processing
>  // the current Entry.
>  //
>  #define NET_LIST_FOR_EACH_SAFE(Entry, NextEntry, ListHead) \
> -  for(Entry = (ListHead)->ForwardLink, NextEntry = Entry->ForwardLink; \
> -      Entry != (ListHead); \
> +  for(Entry = (ListHead)->ForwardLink, (Entry != NULL) ? (NextEntry = Entry->ForwardLink) : (Entry = NULL); \
> +      Entry != (ListHead), Entry != NULL; \
>        Entry = NextEntry, NextEntry = Entry->ForwardLink \
>       )
>  
>  //
>  // Make sure the list isn't empty before getting the first/last record.
> 

Re: [PATCH v1 1/2] MdeModulePkg/NetLib.h: Fix the possible NULL pointer dereference issue.

[Wu, Jiaxin]

Hi Laszlo,

Thanks review the patch.

> 
> (1) The linked list from BaseLib (LIST_ENTRY) always has at least one
> element (the head element), and the list is empty if the head element
> points back to itself. In other words, ForwardLink may never be NULL.
> 
> So why is it necessary to check against that case here?
> 

I agree the ForwardLink in valid LIST_ENTRY can't be NULL. Here, I added the more condition check was considering the possible broken case of the LIST_ENTRY. But I also checked the other usage of *_FOR_EACH_SAFE /*_FOR_EACH, looks never or unnecessary to consider that case.

If so,  please drop the series patches and I will create another series patches to remove the unnecessary check after retrieving the value from list Entry.


> (2) If the NULL check is indeed necessary for some reason, then we
> should write
> 
>   Entry != (ListHead) && Entry != NULL
> 
> in the controlling expression. Because, with the comma operator, the
> (Entry != (ListHead)) expression would be evaluated, but its result
> would be ignored.
> 

Yes, I was also awared that, so I created patch v2 to fix that: 

[edk2] [PATCH v2 1/2] MdeModulePkg/NetLib.h: Fix the possible NULL pointer dereference issue.
Fix the wrong condition in for cycle.

Thanks,
Jiaxin

[PATCH v1 2/2] MdeModulePkg/Dhcp4Dxe: Use NET_LIST_FOR_EACH instead of NET_LIST_FOR_EACH_SAFE.

[Jiaxin Wu]

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

NET_LIST_FOR_EACH_SAFE is defined to iterate through the double linked list
in delete-safe way. It's unnecessary to use this macro if list entries won't
be deleted.

Cc: Ye Ting <ting.ye@intel.com>
Cc: Fu Siyuan <siyuan.fu@intel.com>
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Wu Jiaxin <jiaxin.wu@intel.com>
---
 MdeModulePkg/Universal/Network/Dhcp4Dxe/Dhcp4Io.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/MdeModulePkg/Universal/Network/Dhcp4Dxe/Dhcp4Io.c b/MdeModulePkg/Universal/Network/Dhcp4Dxe/Dhcp4Io.c
index 98a22a77b4..47a9db6489 100644
--- a/MdeModulePkg/Universal/Network/Dhcp4Dxe/Dhcp4Io.c
+++ b/MdeModulePkg/Universal/Network/Dhcp4Dxe/Dhcp4Io.c
@@ -1493,15 +1493,15 @@ DhcpOnTimerTick (
   IN EFI_EVENT              Event,
   IN VOID                   *Context
   )
 {
   LIST_ENTRY                *Entry;
-  LIST_ENTRY                *Next;
   DHCP_SERVICE              *DhcpSb;
   DHCP_PROTOCOL             *Instance;
   EFI_STATUS                Status;
 
+  Entry    = NULL;
   DhcpSb   = (DHCP_SERVICE *) Context;
   Instance = DhcpSb->ActiveChild;
 
   //
   // 0xffff is the maximum supported value for elapsed time according to RFC.
@@ -1644,11 +1644,11 @@ DhcpOnTimerTick (
 
 ON_EXIT:
   //
   // Iterate through all the DhcpSb Children.
   //
-  NET_LIST_FOR_EACH_SAFE (Entry, Next, &DhcpSb->Children) {
+  NET_LIST_FOR_EACH (Entry, &DhcpSb->Children) {
     Instance = NET_LIST_USER_STRUCT (Entry, DHCP_PROTOCOL, Link);
 
     if ((Instance != NULL) && (Instance->Token != NULL)) {
       Instance->Timeout--;
       if (Instance->Timeout == 0) {
-- 
2.17.1.windows.2