[PATCH v2 0/4] MdeModulePkg: Add check for variables and return value

[PATCH v2 0/4] MdeModulePkg: Add check for variables and return value

[Zhang, Shenglei]

The variables and return value might be NULL.
So add check for them before they are used.

Cc: Jian J Wang <jian.j.wang@intel.com>
Cc: Hao A Wu <hao.a.wu@intel.com>
Cc: Dandan Bi <dandan.bi@intel.com>
Cc: Eric Dong <eric.dong@intel.com>
Cc: Hao A Wu <hao.a.wu@intel.com>
Cc: Liming Gao <liming.gao@intel.com>
Shenglei Zhang (4):

v2: Update the checking method in 02/04.

  MdeModulePkg/EbcDebugger: Add check for Entry and RetEntry
  MdeModulePkg/HiiDatabaseDxe: Add check for StringPtr
  MdeModulePkg/EsrtDxe: Add check for EsrtRepository
  MdeModulePkg/SetupBrowserDxe: Add check for GetBufferForValue()

 .../Universal/EbcDxe/EbcDebugger/EdbCmdSymbol.c        |  2 +-
 MdeModulePkg/Universal/EbcDxe/EbcDebugger/EdbSymbol.c  |  2 +-
 MdeModulePkg/Universal/EsrtDxe/EsrtImpl.c              | 10 ++++++++++
 MdeModulePkg/Universal/HiiDatabaseDxe/ConfigRouting.c  |  1 +
 MdeModulePkg/Universal/SetupBrowserDxe/Expression.c    |  5 +++++
 5 files changed, 18 insertions(+), 2 deletions(-)

-- 
2.18.0.windows.1

[PATCH v2 1/4] MdeModulePkg/EbcDebugger: Add check for Entry and RetEntry

[Zhang, Shenglei]

Entry and RetEntry might be NULL before used.

Cc: Jian J Wang <jian.j.wang@intel.com>
Cc: Hao A Wu <hao.a.wu@intel.com>
Signed-off-by: Shenglei Zhang <shenglei.zhang@intel.com>
Reviewed-by: Hao A Wu <hao.a.wu@intel.com>
---
 MdeModulePkg/Universal/EbcDxe/EbcDebugger/EdbCmdSymbol.c | 2 +-
 MdeModulePkg/Universal/EbcDxe/EbcDebugger/EdbSymbol.c    | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/MdeModulePkg/Universal/EbcDxe/EbcDebugger/EdbCmdSymbol.c b/MdeModulePkg/Universal/EbcDxe/EbcDebugger/EdbCmdSymbol.c
index 8e305e4243a5..7b453fa98c2b 100644
--- a/MdeModulePkg/Universal/EbcDxe/EbcDebugger/EdbCmdSymbol.c
+++ b/MdeModulePkg/Universal/EbcDxe/EbcDebugger/EdbCmdSymbol.c
@@ -143,7 +143,7 @@ DebuggerDisplaySymbolAccrodingToAddress (
   // Find the nearest symbol address
   //
   CandidateAddress = EbdFindSymbolAddress (Address, EdbMatchSymbolTypeNearestAddress, &Object, &Entry);
-  if (CandidateAddress == 0 || CandidateAddress == (UINTN) -1) {
+  if (CandidateAddress == 0 || CandidateAddress == (UINTN) -1 || Entry == NULL) {
     EDBPrint (L"Symbole at Address not found!\n");
     return EFI_DEBUG_CONTINUE;
   } else if (Address != CandidateAddress) {
diff --git a/MdeModulePkg/Universal/EbcDxe/EbcDebugger/EdbSymbol.c b/MdeModulePkg/Universal/EbcDxe/EbcDebugger/EdbSymbol.c
index 85cc275c114b..90a9b9fbd7ee 100644
--- a/MdeModulePkg/Universal/EbcDxe/EbcDebugger/EdbSymbol.c
+++ b/MdeModulePkg/Universal/EbcDxe/EbcDebugger/EdbSymbol.c
@@ -2062,7 +2062,7 @@ EdbPrintSource (
                     &RetObject,
                     &RetEntry
                     );
-  if (SymbolAddress == 0) {
+  if (SymbolAddress == 0 || RetEntry == NULL) {
     return 0 ;
   }
 
-- 
2.18.0.windows.1

[PATCH v2 2/4] MdeModulePkg/HiiDatabaseDxe: Add check for StringPtr

[Zhang, Shenglei]

If the target string doesn't appear in the searched string,
StringPtr will be NULL. So add a check for that.

Cc: Dandan Bi <dandan.bi@intel.com>
Cc: Eric Dong <eric.dong@intel.com>
Signed-off-by: Shenglei Zhang <shenglei.zhang@intel.com>
---
v2: Instead of returning a value, we add ASSERT to ensure
    StringPtr is not NULL.

 MdeModulePkg/Universal/HiiDatabaseDxe/ConfigRouting.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/MdeModulePkg/Universal/HiiDatabaseDxe/ConfigRouting.c b/MdeModulePkg/Universal/HiiDatabaseDxe/ConfigRouting.c
index 71ea25bc19bf..19a23fcc951e 100644
--- a/MdeModulePkg/Universal/HiiDatabaseDxe/ConfigRouting.c
+++ b/MdeModulePkg/Universal/HiiDatabaseDxe/ConfigRouting.c
@@ -909,6 +909,7 @@ CompareAndMergeDefaultString (
   // To find the <AltResp> with AltConfigHdr in AltCfgResp, ignore other <AltResp> which follow it.
   //
   StringPtr = StrStr (*AltCfgResp, AltConfigHdr);
+  ASSERT (StringPtr != NULL); 
   StringPtrNext = StrStr (StringPtr + 1, L"&GUID");
   if (StringPtrNext != NULL) {
     TempCharA = *StringPtrNext;
-- 
2.18.0.windows.1

[PATCH v2 3/4] MdeModulePkg/EsrtDxe: Add check for EsrtRepository

[Zhang, Shenglei]

EsrtRepository might be NULL. So return EFI_OUT_OF_RESOURCES
when it is NULL.

Cc: Hao A Wu <hao.a.wu@intel.com>
Cc: Liming Gao <liming.gao@intel.com>
Signed-off-by: Shenglei Zhang <shenglei.zhang@intel.com>
Reviewed-by: Hao A Wu <hao.a.wu@intel.com>
---
 MdeModulePkg/Universal/EsrtDxe/EsrtImpl.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/MdeModulePkg/Universal/EsrtDxe/EsrtImpl.c b/MdeModulePkg/Universal/EsrtDxe/EsrtImpl.c
index f48125382dbc..fff17b98fa3d 100644
--- a/MdeModulePkg/Universal/EsrtDxe/EsrtImpl.c
+++ b/MdeModulePkg/Universal/EsrtDxe/EsrtImpl.c
@@ -239,6 +239,11 @@ DeleteEsrtEntry(
     goto EXIT;
   }
 
+  if (EsrtRepository == NULL) {
+    Status = EFI_OUT_OF_RESOURCES;
+    goto EXIT;
+  }
+
   if ((RepositorySize % sizeof(EFI_SYSTEM_RESOURCE_ENTRY)) != 0) {
     DEBUG((EFI_D_ERROR, "Repository Corrupt. Need to rebuild Repository.\n"));
     //
@@ -332,6 +337,11 @@ UpdateEsrtEntry(
              &RepositorySize
              );
 
+  if (EsrtRepository == NULL) {
+    Status = EFI_OUT_OF_RESOURCES;
+    goto EXIT;
+  }
+
   if (!EFI_ERROR(Status)) {
     //
     // if exist, update Esrt cache repository
-- 
2.18.0.windows.1

[PATCH v2 4/4] MdeModulePkg/SetupBrowserDxe: Add check for GetBufferForValue()

[Zhang, Shenglei]

The returned value from GetBufferForValue might be NULL, so add a
check for that before it is used.

Cc: Jian J Wang <jian.j.wang@intel.com>
Cc: Hao A Wu <hao.a.wu@intel.com>
Signed-off-by: Shenglei Zhang <shenglei.zhang@intel.com>
---
 MdeModulePkg/Universal/SetupBrowserDxe/Expression.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/MdeModulePkg/Universal/SetupBrowserDxe/Expression.c b/MdeModulePkg/Universal/SetupBrowserDxe/Expression.c
index 7f4929c2fcd9..984c68c6bb7a 100644
--- a/MdeModulePkg/Universal/SetupBrowserDxe/Expression.c
+++ b/MdeModulePkg/Universal/SetupBrowserDxe/Expression.c
@@ -1281,7 +1281,12 @@ IfrToUint (
       Result->Type = EFI_IFR_TYPE_UNDEFINED;
       return EFI_SUCCESS;
     }
+
+    if (GetBufferForValue (&Value) == NULL) {
+      return EFI_NOT_FOUND;
+    } 
     Result->Value.u64 = *(UINT64*) GetBufferForValue (&Value);
+
     if (Value.Type == EFI_IFR_TYPE_BUFFER) {
       FreePool (Value.Buffer);
     }
-- 
2.18.0.windows.1

Re: [edk2-devel] [PATCH v2 4/4] MdeModulePkg/SetupBrowserDxe: Add check for GetBufferForValue()

[Dandan Bi]

-----Original Message-----
From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Zhang, Shenglei
Sent: Wednesday, October 30, 2019 10:27 PM
To: devel@edk2.groups.io
Cc: Wang, Jian J <jian.j.wang@intel.com>; Wu, Hao A <hao.a.wu@intel.com>
Subject: [edk2-devel] [PATCH v2 4/4] MdeModulePkg/SetupBrowserDxe: Add check for GetBufferForValue()

The returned value from GetBufferForValue might be NULL, so add a check for that before it is used.

Cc: Jian J Wang <jian.j.wang@intel.com>
Cc: Hao A Wu <hao.a.wu@intel.com>
Signed-off-by: Shenglei Zhang <shenglei.zhang@intel.com>
---
 MdeModulePkg/Universal/SetupBrowserDxe/Expression.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/MdeModulePkg/Universal/SetupBrowserDxe/Expression.c b/MdeModulePkg/Universal/SetupBrowserDxe/Expression.c
index 7f4929c2fcd9..984c68c6bb7a 100644
--- a/MdeModulePkg/Universal/SetupBrowserDxe/Expression.c
+++ b/MdeModulePkg/Universal/SetupBrowserDxe/Expression.c
@@ -1281,7 +1281,12 @@ IfrToUint (
       Result->Type = EFI_IFR_TYPE_UNDEFINED;
       return EFI_SUCCESS;
     }
+
+    if (GetBufferForValue (&Value) == NULL) {
+      return EFI_NOT_FOUND;
+    }

Hi Shenglei,

Before call GetBufferForValue function, has already called function  IsTypeInBuffer to make sure the value must be buffer type.
So GetBufferForValue can not return NULL. 
I think we also can add ASSERT here.

Thanks,
Dandan

     Result->Value.u64 = *(UINT64*) GetBufferForValue (&Value);
+
     if (Value.Type == EFI_IFR_TYPE_BUFFER) {
       FreePool (Value.Buffer);
     }
--
2.18.0.windows.1

Re: [PATCH v2 2/4] MdeModulePkg/HiiDatabaseDxe: Add check for StringPtr

[Dandan Bi]

Hi Shenglei,

Please update the commit message and subject before commit, since we have added ASSERT code for this case that the StringPtr cannot be NULL instead of adding check.
With the commit message updated, Reviewed-by: Dandan Bi <dandan.bi@intel.com>.


Thanks,
Dandan

> -----Original Message-----
> From: Zhang, Shenglei
> Sent: Wednesday, October 30, 2019 10:27 PM
> To: devel@edk2.groups.io
> Cc: Bi, Dandan <dandan.bi@intel.com>; Dong, Eric <eric.dong@intel.com>
> Subject: [PATCH v2 2/4] MdeModulePkg/HiiDatabaseDxe: Add check for
> StringPtr
> 
> If the target string doesn't appear in the searched string, StringPtr will be
> NULL. So add a check for that.
> 
> Cc: Dandan Bi <dandan.bi@intel.com>
> Cc: Eric Dong <eric.dong@intel.com>
> Signed-off-by: Shenglei Zhang <shenglei.zhang@intel.com>
> ---
> v2: Instead of returning a value, we add ASSERT to ensure
>     StringPtr is not NULL.
> 
>  MdeModulePkg/Universal/HiiDatabaseDxe/ConfigRouting.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/MdeModulePkg/Universal/HiiDatabaseDxe/ConfigRouting.c
> b/MdeModulePkg/Universal/HiiDatabaseDxe/ConfigRouting.c
> index 71ea25bc19bf..19a23fcc951e 100644
> --- a/MdeModulePkg/Universal/HiiDatabaseDxe/ConfigRouting.c
> +++ b/MdeModulePkg/Universal/HiiDatabaseDxe/ConfigRouting.c
> @@ -909,6 +909,7 @@ CompareAndMergeDefaultString (
>    // To find the <AltResp> with AltConfigHdr in AltCfgResp, ignore other
> <AltResp> which follow it.
>    //
>    StringPtr = StrStr (*AltCfgResp, AltConfigHdr);
> +  ASSERT (StringPtr != NULL);
>    StringPtrNext = StrStr (StringPtr + 1, L"&GUID");
>    if (StringPtrNext != NULL) {
>      TempCharA = *StringPtrNext;
> --
> 2.18.0.windows.1