[PATCH] MdeModulePkg/Core: fix an issue of potential NULL pointer access

[PATCH] MdeModulePkg/Core: fix an issue of potential NULL pointer access

[Jian J Wang]

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

This issue is introduced by bb685071c2602cf786ea84c69bbebf2158194a38.

The *MemorySpaceMap assigned with NULL (line 1710) value might be
accessed (line 1726/1730) without any sanity check. Although it won't
happen in practice because of line 1722, we still need to add check
against NULL to make static code analyzer happy.

1710  *MemorySpaceMap       = NULL;
....  ...
1722  if (DescriptorCount == *NumberOfDescriptors) {
....  ...
1726    Descriptor = *MemorySpaceMap;
....  ...
1730        BuildMemoryDescriptor (Descriptor, Entry);

Tests:
  Pass build and boot to shell.

Cc: Hao Wu <hao.a.wu@intel.com>
Cc: Star Zeng <star.zeng@intel.com>
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Jian J Wang <jian.j.wang@intel.com>
---
 MdeModulePkg/Core/Dxe/Gcd/Gcd.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/MdeModulePkg/Core/Dxe/Gcd/Gcd.c b/MdeModulePkg/Core/Dxe/Gcd/Gcd.c
index 8bbdf7129f..a76d2db73c 100644
--- a/MdeModulePkg/Core/Dxe/Gcd/Gcd.c
+++ b/MdeModulePkg/Core/Dxe/Gcd/Gcd.c
@@ -1719,7 +1719,7 @@ CoreGetMemorySpaceMap (
     // AllocatePool() called below has to be running outside the GCD lock.
     //
     DescriptorCount = CoreCountGcdMapEntry (&mGcdMemorySpaceMap);
-    if (DescriptorCount == *NumberOfDescriptors) {
+    if (DescriptorCount == *NumberOfDescriptors && *MemorySpaceMap != NULL) {
       //
       // Fill in the MemorySpaceMap if no memory space map change.
       //
-- 
2.16.2.windows.1

Re: [PATCH] MdeModulePkg/Core: fix an issue of potential NULL pointer access

[Wu, Hao A]

> -----Original Message-----
> From: Wang, Jian J
> Sent: Monday, October 29, 2018 4:22 PM
> To: edk2-devel@lists.01.org
> Cc: Wu, Hao A; Zeng, Star
> Subject: [PATCH] MdeModulePkg/Core: fix an issue of potential NULL
> pointer access
> 
> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=1286
> 
> This issue is introduced by bb685071c2602cf786ea84c69bbebf2158194a38.
> 
> The *MemorySpaceMap assigned with NULL (line 1710) value might be
> accessed (line 1726/1730) without any sanity check. Although it won't
> happen in practice because of line 1722, we still need to add check
> against NULL to make static code analyzer happy.
> 
> 1710  *MemorySpaceMap       = NULL;
> ....  ...
> 1722  if (DescriptorCount == *NumberOfDescriptors) {
> ....  ...
> 1726    Descriptor = *MemorySpaceMap;
> ....  ...
> 1730        BuildMemoryDescriptor (Descriptor, Entry);
> 
> Tests:
>   Pass build and boot to shell.
> 
> Cc: Hao Wu <hao.a.wu@intel.com>
> Cc: Star Zeng <star.zeng@intel.com>
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Jian J Wang <jian.j.wang@intel.com>
> ---
>  MdeModulePkg/Core/Dxe/Gcd/Gcd.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/MdeModulePkg/Core/Dxe/Gcd/Gcd.c
> b/MdeModulePkg/Core/Dxe/Gcd/Gcd.c
> index 8bbdf7129f..a76d2db73c 100644
> --- a/MdeModulePkg/Core/Dxe/Gcd/Gcd.c
> +++ b/MdeModulePkg/Core/Dxe/Gcd/Gcd.c
> @@ -1719,7 +1719,7 @@ CoreGetMemorySpaceMap (
>      // AllocatePool() called below has to be running outside the GCD lock.
>      //
>      DescriptorCount = CoreCountGcdMapEntry (&mGcdMemorySpaceMap);
> -    if (DescriptorCount == *NumberOfDescriptors) {
> +    if (DescriptorCount == *NumberOfDescriptors && *MemorySpaceMap !=
> NULL) {

Seems good to me.
Reviewed-by: Hao Wu <hao.a.wu@intel.com>

Best Regards,
Hao Wu

>        //
>        // Fill in the MemorySpaceMap if no memory space map change.
>        //
> --
> 2.16.2.windows.1

Re: [PATCH] MdeModulePkg/Core: fix an issue of potential NULL pointer access

[Zeng, Star]

Reviewed-by: Star Zeng <star.zeng@intel.com>

-----Original Message-----
From: Wang, Jian J 
Sent: Monday, October 29, 2018 4:22 PM
To: edk2-devel@lists.01.org
Cc: Wu, Hao A <hao.a.wu@intel.com>; Zeng, Star <star.zeng@intel.com>
Subject: [PATCH] MdeModulePkg/Core: fix an issue of potential NULL pointer access

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

This issue is introduced by bb685071c2602cf786ea84c69bbebf2158194a38.

The *MemorySpaceMap assigned with NULL (line 1710) value might be accessed (line 1726/1730) without any sanity check. Although it won't happen in practice because of line 1722, we still need to add check against NULL to make static code analyzer happy.

1710  *MemorySpaceMap       = NULL;
....  ...
1722  if (DescriptorCount == *NumberOfDescriptors) { ....  ...
1726    Descriptor = *MemorySpaceMap;
....  ...
1730        BuildMemoryDescriptor (Descriptor, Entry);

Tests:
  Pass build and boot to shell.

Cc: Hao Wu <hao.a.wu@intel.com>
Cc: Star Zeng <star.zeng@intel.com>
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Jian J Wang <jian.j.wang@intel.com>
---
 MdeModulePkg/Core/Dxe/Gcd/Gcd.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/MdeModulePkg/Core/Dxe/Gcd/Gcd.c b/MdeModulePkg/Core/Dxe/Gcd/Gcd.c index 8bbdf7129f..a76d2db73c 100644
--- a/MdeModulePkg/Core/Dxe/Gcd/Gcd.c
+++ b/MdeModulePkg/Core/Dxe/Gcd/Gcd.c
@@ -1719,7 +1719,7 @@ CoreGetMemorySpaceMap (
     // AllocatePool() called below has to be running outside the GCD lock.
     //
     DescriptorCount = CoreCountGcdMapEntry (&mGcdMemorySpaceMap);
-    if (DescriptorCount == *NumberOfDescriptors) {
+    if (DescriptorCount == *NumberOfDescriptors && *MemorySpaceMap != 
+ NULL) {
       //
       // Fill in the MemorySpaceMap if no memory space map change.
       //
--
2.16.2.windows.1