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

[PATCH 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):
  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  |  4 ++++
 MdeModulePkg/Universal/SetupBrowserDxe/Expression.c    |  5 +++++
 5 files changed, 21 insertions(+), 2 deletions(-)

-- 
2.18.0.windows.1

[PATCH 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>
---
 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

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

[Wu, Hao A]

> -----Original Message-----
> From: Zhang, Shenglei
> Sent: Thursday, October 17, 2019 11:31 AM
> To: devel@edk2.groups.io
> Cc: Wang, Jian J; Wu, Hao A
> Subject: [PATCH 1/4] MdeModulePkg/EbcDebugger: Add check for Entry and
> RetEntry
> 
> 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>
> ---
>  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 ;
>    }


Reviewed-by: Hao A Wu <hao.a.wu@intel.com>

Best Regards,
Hao Wu


> 
> --
> 2.18.0.windows.1

[PATCH 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>
---
 MdeModulePkg/Universal/HiiDatabaseDxe/ConfigRouting.c | 4 ++++
 1 file changed, 4 insertions(+)

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

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

[Dandan Bi]

> -----Original Message-----
> From: Zhang, Shenglei
> Sent: Thursday, October 17, 2019 11:31 AM
> To: devel@edk2.groups.io
> Cc: Bi, Dandan <dandan.bi@intel.com>; Dong, Eric <eric.dong@intel.com>
> Subject: [PATCH 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>
> ---
>  MdeModulePkg/Universal/HiiDatabaseDxe/ConfigRouting.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/MdeModulePkg/Universal/HiiDatabaseDxe/ConfigRouting.c
> b/MdeModulePkg/Universal/HiiDatabaseDxe/ConfigRouting.c
> index 71ea25bc19bf..f786da8e370a 100644
> --- a/MdeModulePkg/Universal/HiiDatabaseDxe/ConfigRouting.c
> +++ b/MdeModulePkg/Universal/HiiDatabaseDxe/ConfigRouting.c
> @@ -909,6 +909,10 @@ CompareAndMergeDefaultString (
>    // To find the <AltResp> with AltConfigHdr in AltCfgResp, ignore other
> <AltResp> which follow it.
>    //
>    StringPtr = StrStr (*AltCfgResp, AltConfigHdr);
> +  if (StringPtr == NULL) {
> +    Status = EFI_NOT_FOUND;
> +    goto Exit;
> +  }
Hi Shenglei,
I think we can add  ASSERT (StringPtr != NULL); here.
Since when call this function, the AltCfgResp and DefaultAltCfgResp must have contained the AltConfigHdr.
The caller of the CompareAndMergeDefaultString have done the check.

Thanks,
Dandan
>    StringPtrNext = StrStr (StringPtr + 1, L"&GUID");
>    if (StringPtrNext != NULL) {
>      TempCharA = *StringPtrNext;
> --
> 2.18.0.windows.1

[PATCH 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>
---
 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

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

[Wu, Hao A]

> -----Original Message-----
> From: Zhang, Shenglei
> Sent: Thursday, October 17, 2019 11:31 AM
> To: devel@edk2.groups.io
> Cc: Wu, Hao A; Gao, Liming
> Subject: [PATCH 3/4] MdeModulePkg/EsrtDxe: Add check for EsrtRepository
> 
> 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>
> ---
>  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;
> +  }
> +


Reviewed-by: Hao A Wu <hao.a.wu@intel.com>

Best Regards,
Hao Wu


>    if (!EFI_ERROR(Status)) {
>      //
>      // if exist, update Esrt cache repository
> --
> 2.18.0.windows.1

[PATCH 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: [PATCH 4/4] MdeModulePkg/SetupBrowserDxe: Add check for GetBufferForValue()

[Wu, Hao A]

Hello Eric and Dandan,

Could you help to review this patch?
Thanks in advance.

Best Regards,
Hao Wu


> -----Original Message-----
> From: Zhang, Shenglei
> Sent: Thursday, October 17, 2019 11:31 AM
> To: devel@edk2.groups.io
> Cc: Wang, Jian J; Wu, Hao A
> Subject: [PATCH 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;
> +    }
>      Result->Value.u64 = *(UINT64*) GetBufferForValue (&Value);
> +
>      if (Value.Type == EFI_IFR_TYPE_BUFFER) {
>        FreePool (Value.Buffer);
>      }
> --
> 2.18.0.windows.1