[PATCH] MinPlatformPkg/SecFspWrapperPlatformSecLib: Change TopOfTemporaryRam type

[PATCH] MinPlatformPkg/SecFspWrapperPlatformSecLib: Change TopOfTemporaryRam type

[Zhang, Shenglei]

Cast TopOfTemporaryRam's from UINT32 to UINTN in the expression.
The original code (TopOfTemporaryRam - sizeof (UINT32)) may cause
overflow. As a result the operation under 64-bit OS environment, (UINT)(...),
may cast a overflowed 4-byte result to 8-byte one.

Cc: Michael Kubacki <michael.a.kubacki@intel.com>
Cc: Chasel Chiu <chasel.chiu@intel.com>
Cc: Nate DeSimone <nathaniel.l.desimone@intel.com>
Cc: Liming Gao <liming.gao@intel.com>
Signed-off-by: Shenglei Zhang <shenglei.zhang@intel.com>
---
 .../Library/SecFspWrapperPlatformSecLib/SecGetPerformance.c     | 2 +-
 .../SecFspWrapperPlatformSecLib/SecPlatformInformation.c        | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/Platform/Intel/MinPlatformPkg/FspWrapper/Library/SecFspWrapperPlatformSecLib/SecGetPerformance.c b/Platform/Intel/MinPlatformPkg/FspWrapper/Library/SecFspWrapperPlatformSecLib/SecGetPerformance.c
index c4eeb2b1..0cc42f96 100644
--- a/Platform/Intel/MinPlatformPkg/FspWrapper/Library/SecFspWrapperPlatformSecLib/SecGetPerformance.c
+++ b/Platform/Intel/MinPlatformPkg/FspWrapper/Library/SecFspWrapperPlatformSecLib/SecGetPerformance.c
@@ -79,7 +79,7 @@ SecGetPerformance (
   //
   TopOfTemporaryRam = (UINT32)(UINTN)TopOfTemporaryRamPpi - sizeof(UINT32);
   TopOfTemporaryRam -= sizeof(UINT32) * 2;
-  Count             = *(UINT32 *) (UINTN) (TopOfTemporaryRam - sizeof (UINT32));
+  Count             = *(UINT32 *)((UINTN)TopOfTemporaryRam - sizeof (UINT32));
   Size              = Count * sizeof (UINT32);
 
   Ticker = *(UINT64 *) (UINTN) (TopOfTemporaryRam - sizeof (UINT32) - Size - sizeof (UINT32) * 2);
diff --git a/Platform/Intel/MinPlatformPkg/FspWrapper/Library/SecFspWrapperPlatformSecLib/SecPlatformInformation.c b/Platform/Intel/MinPlatformPkg/FspWrapper/Library/SecFspWrapperPlatformSecLib/SecPlatformInformation.c
index 5b94ed2b..1bcee5f4 100644
--- a/Platform/Intel/MinPlatformPkg/FspWrapper/Library/SecFspWrapperPlatformSecLib/SecPlatformInformation.c
+++ b/Platform/Intel/MinPlatformPkg/FspWrapper/Library/SecFspWrapperPlatformSecLib/SecPlatformInformation.c
@@ -61,7 +61,7 @@ SecPlatformInformation (
   //
   TopOfTemporaryRam = (UINT32)(UINTN)TopOfTemporaryRamPpi - sizeof (UINT32);
   TopOfTemporaryRam -= sizeof(UINT32) * 2;
-  Count             = *((UINT32 *)(UINTN) (TopOfTemporaryRam - sizeof (UINT32)));
+  Count             = *((UINT32 *)((UINTN)TopOfTemporaryRam - sizeof (UINT32)));
   Size              = Count * sizeof (IA32_HANDOFF_STATUS);
 
   if ((*StructureSize) < (UINT64) Size) {
-- 
2.18.0.windows.1

[PATCH] MinPlatformPkg/TestPointCheckLib: Add return value when OutTable is NULL

[Zhang, Shenglei]

Currently there is no check for the parameter OutTable.
So add the logic that return value EFI_INVALID_PARAMETER when the
OutTable is NULL.

Cc: Michael Kubacki <michael.a.kubacki@intel.com>
Cc: Chasel Chiu <chasel.chiu@intel.com>
Cc: Nate DeSimone <nathaniel.l.desimone@intel.com>
Cc: Liming Gao <liming.gao@intel.com>
Signed-off-by: Shenglei Zhang <shenglei.zhang@intel.com>
---
 .../Test/Library/TestPointCheckLib/DxeCheckAcpi.c           | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/Platform/Intel/MinPlatformPkg/Test/Library/TestPointCheckLib/DxeCheckAcpi.c b/Platform/Intel/MinPlatformPkg/Test/Library/TestPointCheckLib/DxeCheckAcpi.c
index 263781a2..3828ab72 100644
--- a/Platform/Intel/MinPlatformPkg/Test/Library/TestPointCheckLib/DxeCheckAcpi.c
+++ b/Platform/Intel/MinPlatformPkg/Test/Library/TestPointCheckLib/DxeCheckAcpi.c
@@ -611,6 +611,9 @@ DumpAcpiRsdt (
   if (OutTable != NULL) {
     *OutTable = NULL;
   }
+  else{
+    return EFI_INVALID_PARAMETER;
+  } 
 
   ReturnStatus = EFI_SUCCESS;
   EntryCount = (Rsdt->Length - sizeof (EFI_ACPI_DESCRIPTION_HEADER)) / sizeof(UINT32);
@@ -664,6 +667,9 @@ DumpAcpiXsdt (
   if (OutTable != NULL) {
     *OutTable = NULL;
   }
+  else{
+    return EFI_INVALID_PARAMETER;
+  } 
 
   ReturnStatus = EFI_SUCCESS;
   EntryCount = (Xsdt->Length - sizeof (EFI_ACPI_DESCRIPTION_HEADER)) / sizeof(UINT64);
-- 
2.18.0.windows.1

[PATCH] MinPlatformPkg/TestPointCheckLib: Add check for pointers

[Zhang, Shenglei]

In DxeCheckBootVariable.c, add check for BootOrder and Variable
that return EFI_NOT_FOUND when they are NULL.
In DxeCheckGcd.c, add check for GcdIoMap to ensure it not NULL
when allocating memory to what it points to.

Cc: Michael Kubacki <michael.a.kubacki@intel.com>
Cc: Chasel Chiu <chasel.chiu@intel.com>
Cc: Nate DeSimone <nathaniel.l.desimone@intel.com>
Cc: Liming Gao <liming.gao@intel.com>
Signed-off-by: Shenglei Zhang <shenglei.zhang@intel.com>
---
 .../Test/Library/TestPointCheckLib/DxeCheckBootVariable.c   | 6 ++++++
 .../Test/Library/TestPointCheckLib/DxeCheckGcd.c            | 4 +++-
 2 files changed, 9 insertions(+), 1 deletion(-)

diff --git a/Platform/Intel/MinPlatformPkg/Test/Library/TestPointCheckLib/DxeCheckBootVariable.c b/Platform/Intel/MinPlatformPkg/Test/Library/TestPointCheckLib/DxeCheckBootVariable.c
index 85bd5b3d..a9af33a3 100644
--- a/Platform/Intel/MinPlatformPkg/Test/Library/TestPointCheckLib/DxeCheckBootVariable.c
+++ b/Platform/Intel/MinPlatformPkg/Test/Library/TestPointCheckLib/DxeCheckBootVariable.c
@@ -130,6 +130,9 @@ TestPointCheckLoadOptionVariable (
   for (ListIndex = 0; ListIndex < sizeof(mLoadOptionVariableList)/sizeof(mLoadOptionVariableList[0]); ListIndex++) {
     UnicodeSPrint (BootOrderName, sizeof(BootOrderName), L"%sOrder", mLoadOptionVariableList[ListIndex]);
     Status = GetVariable2 (BootOrderName, &gEfiGlobalVariableGuid, (VOID **)&BootOrder, &OrderSize);
+	if(BootOrder == NULL) {
+      return EFI_NOT_FOUND;;
+	}
     if (EFI_ERROR(Status)) {
       continue;
     }
@@ -222,6 +225,9 @@ TestPointCheckKeyOptionVariable (
     for (Index = 0; ; Index++) {
       UnicodeSPrint (KeyOptionName, sizeof(KeyOptionName), L"%s%04x", mKeyOptionVariableList[ListIndex], Index);
       Status = GetVariable2 (KeyOptionName, &gEfiGlobalVariableGuid, &Variable, &Size);
+      if(Variable == NULL) {
+        return EFI_NOT_FOUND;;
+      } 
       if (!EFI_ERROR(Status)) {
         DumpKeyOption (KeyOptionName, Variable, Size);
       } else {
diff --git a/Platform/Intel/MinPlatformPkg/Test/Library/TestPointCheckLib/DxeCheckGcd.c b/Platform/Intel/MinPlatformPkg/Test/Library/TestPointCheckLib/DxeCheckGcd.c
index 82709d44..989606b6 100644
--- a/Platform/Intel/MinPlatformPkg/Test/Library/TestPointCheckLib/DxeCheckGcd.c
+++ b/Platform/Intel/MinPlatformPkg/Test/Library/TestPointCheckLib/DxeCheckGcd.c
@@ -241,7 +241,9 @@ TestPointDumpGcd (
       }
     }
     if (GcdMemoryMap != NULL) {
-      *GcdIoMap = AllocateCopyPool (NumberOfDescriptors * sizeof(EFI_GCD_IO_SPACE_DESCRIPTOR), IoMap);
+      if (GcdIoMap != NULL){
+        *GcdIoMap = AllocateCopyPool (NumberOfDescriptors * sizeof(EFI_GCD_IO_SPACE_DESCRIPTOR), IoMap);
+      }
       *GcdIoMapNumberOfDescriptors = NumberOfDescriptors;
     }
   }
-- 
2.18.0.windows.1

[PATCH] MinPlatformPkg/TestPointCheckLib: Add check for pointer Variable

[Zhang, Shenglei]

Add check for pointer Variable to ensure it is not NULL when used.

Cc: Michael Kubacki <michael.a.kubacki@intel.com>
Cc: Chasel Chiu <chasel.chiu@intel.com>
Cc: Nate DeSimone <nathaniel.l.desimone@intel.com>
Cc: Liming Gao <liming.gao@intel.com>
Signed-off-by: Shenglei Zhang <shenglei.zhang@intel.com>
---
 .../Test/Library/TestPointCheckLib/DxeCheckUefiSecureBoot.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/Platform/Intel/MinPlatformPkg/Test/Library/TestPointCheckLib/DxeCheckUefiSecureBoot.c b/Platform/Intel/MinPlatformPkg/Test/Library/TestPointCheckLib/DxeCheckUefiSecureBoot.c
index b53a09ab..f5125ede 100644
--- a/Platform/Intel/MinPlatformPkg/Test/Library/TestPointCheckLib/DxeCheckUefiSecureBoot.c
+++ b/Platform/Intel/MinPlatformPkg/Test/Library/TestPointCheckLib/DxeCheckUefiSecureBoot.c
@@ -52,6 +52,9 @@ TestPointCheckUefiSecureBoot (
   ReturnStatus = EFI_SUCCESS;
   for (Index = 0; Index < sizeof(mUefiSecureBootVariable)/sizeof(mUefiSecureBootVariable[0]); Index++) {
     Status = GetVariable2 (mUefiSecureBootVariable[Index].Name, mUefiSecureBootVariable[Index].Guid, &Variable, &Size);
+    if(Variable == NULL) {
+      return EFI_NOT_FOUND;
+    }
     if (EFI_ERROR(Status)) {
       DEBUG ((DEBUG_ERROR, "Variable - %S not found\n", mUefiSecureBootVariable[Index].Name));
       ReturnStatus = Status;
@@ -69,6 +72,9 @@ TestPointCheckUefiSecureBoot (
 
   for (Index = 0; Index < sizeof(mUefiSecureBootModeVariable)/sizeof(mUefiSecureBootModeVariable[0]); Index++) {
     Status = GetVariable2 (mUefiSecureBootModeVariable[Index].Name, mUefiSecureBootModeVariable[Index].Guid, &Variable, &Size);
+    if(Variable == NULL) {
+      return EFI_NOT_FOUND;
+    }
     if (EFI_ERROR(Status)) {
       DEBUG ((DEBUG_ERROR, "Variable - %S not found\n", mUefiSecureBootModeVariable[Index].Name));
       ReturnStatus = Status;
-- 
2.18.0.windows.1

Re: [PATCH] MinPlatformPkg/TestPointCheckLib: Add check for pointers

[Chiu, Chasel]

Please extend copyright for files you touched, and one more update in below inline.

> -----Original Message-----
> From: Zhang, Shenglei
> Sent: Monday, September 2, 2019 8:35 PM
> To: devel@edk2.groups.io
> Cc: Kubacki, Michael A <michael.a.kubacki@intel.com>; Chiu, Chasel
> <chasel.chiu@intel.com>; Desimone, Nathaniel L
> <nathaniel.l.desimone@intel.com>; Gao, Liming <liming.gao@intel.com>
> Subject: [PATCH] MinPlatformPkg/TestPointCheckLib: Add check for pointers
> 
> In DxeCheckBootVariable.c, add check for BootOrder and Variable that
> return EFI_NOT_FOUND when they are NULL.
> In DxeCheckGcd.c, add check for GcdIoMap to ensure it not NULL when
> allocating memory to what it points to.
> 
> Cc: Michael Kubacki <michael.a.kubacki@intel.com>
> Cc: Chasel Chiu <chasel.chiu@intel.com>
> Cc: Nate DeSimone <nathaniel.l.desimone@intel.com>
> Cc: Liming Gao <liming.gao@intel.com>
> Signed-off-by: Shenglei Zhang <shenglei.zhang@intel.com>
> ---
>  .../Test/Library/TestPointCheckLib/DxeCheckBootVariable.c   | 6 ++++++
>  .../Test/Library/TestPointCheckLib/DxeCheckGcd.c            | 4 +++-
>  2 files changed, 9 insertions(+), 1 deletion(-)
> 
> diff --git
> a/Platform/Intel/MinPlatformPkg/Test/Library/TestPointCheckLib/DxeCheckB
> ootVariable.c
> b/Platform/Intel/MinPlatformPkg/Test/Library/TestPointCheckLib/DxeCheckB
> ootVariable.c
> index 85bd5b3d..a9af33a3 100644
> ---
> a/Platform/Intel/MinPlatformPkg/Test/Library/TestPointCheckLib/DxeCheckB
> ootVariable.c
> +++ b/Platform/Intel/MinPlatformPkg/Test/Library/TestPointCheckLib/DxeCh
> +++ eckBootVariable.c
> @@ -130,6 +130,9 @@ TestPointCheckLoadOptionVariable (
>    for (ListIndex = 0; ListIndex <
> sizeof(mLoadOptionVariableList)/sizeof(mLoadOptionVariableList[0]);
> ListIndex++) {
>      UnicodeSPrint (BootOrderName, sizeof(BootOrderName), L"%sOrder",
> mLoadOptionVariableList[ListIndex]);
>      Status = GetVariable2 (BootOrderName, &gEfiGlobalVariableGuid,
> (VOID **)&BootOrder, &OrderSize);
> +	if(BootOrder == NULL) {
> +      return EFI_NOT_FOUND;;
> +	}

Align indents with previous lines.


>      if (EFI_ERROR(Status)) {
>        continue;
>      }
> @@ -222,6 +225,9 @@ TestPointCheckKeyOptionVariable (
>      for (Index = 0; ; Index++) {
>        UnicodeSPrint (KeyOptionName, sizeof(KeyOptionName), L"%s%04x",
> mKeyOptionVariableList[ListIndex], Index);
>        Status = GetVariable2 (KeyOptionName, &gEfiGlobalVariableGuid,
> &Variable, &Size);
> +      if(Variable == NULL) {
> +        return EFI_NOT_FOUND;;
> +      }
>        if (!EFI_ERROR(Status)) {
>          DumpKeyOption (KeyOptionName, Variable, Size);
>        } else {
> diff --git
> a/Platform/Intel/MinPlatformPkg/Test/Library/TestPointCheckLib/DxeCheckG
> cd.c
> b/Platform/Intel/MinPlatformPkg/Test/Library/TestPointCheckLib/DxeCheckG
> cd.c
> index 82709d44..989606b6 100644
> ---
> a/Platform/Intel/MinPlatformPkg/Test/Library/TestPointCheckLib/DxeCheckG
> cd.c
> +++ b/Platform/Intel/MinPlatformPkg/Test/Library/TestPointCheckLib/DxeCh
> +++ eckGcd.c
> @@ -241,7 +241,9 @@ TestPointDumpGcd (
>        }
>      }
>      if (GcdMemoryMap != NULL) {
> -      *GcdIoMap = AllocateCopyPool (NumberOfDescriptors *
> sizeof(EFI_GCD_IO_SPACE_DESCRIPTOR), IoMap);
> +      if (GcdIoMap != NULL){
> +        *GcdIoMap = AllocateCopyPool (NumberOfDescriptors *
> sizeof(EFI_GCD_IO_SPACE_DESCRIPTOR), IoMap);
> +      }
>        *GcdIoMapNumberOfDescriptors = NumberOfDescriptors;
>      }
>    }
> --
> 2.18.0.windows.1

Re: [PATCH] MinPlatformPkg/TestPointCheckLib: Add return value when OutTable is NULL

[Chiu, Chasel]

Extend copyright and please follow the coding style:
if () {
...
} else {
...
}

> -----Original Message-----
> From: Zhang, Shenglei
> Sent: Monday, September 2, 2019 8:35 PM
> To: devel@edk2.groups.io
> Cc: Kubacki, Michael A <michael.a.kubacki@intel.com>; Chiu, Chasel
> <chasel.chiu@intel.com>; Desimone, Nathaniel L
> <nathaniel.l.desimone@intel.com>; Gao, Liming <liming.gao@intel.com>
> Subject: [PATCH] MinPlatformPkg/TestPointCheckLib: Add return value when
> OutTable is NULL
> 
> Currently there is no check for the parameter OutTable.
> So add the logic that return value EFI_INVALID_PARAMETER when the
> OutTable is NULL.
> 
> Cc: Michael Kubacki <michael.a.kubacki@intel.com>
> Cc: Chasel Chiu <chasel.chiu@intel.com>
> Cc: Nate DeSimone <nathaniel.l.desimone@intel.com>
> Cc: Liming Gao <liming.gao@intel.com>
> Signed-off-by: Shenglei Zhang <shenglei.zhang@intel.com>
> ---
>  .../Test/Library/TestPointCheckLib/DxeCheckAcpi.c           | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git
> a/Platform/Intel/MinPlatformPkg/Test/Library/TestPointCheckLib/DxeCheckA
> cpi.c
> b/Platform/Intel/MinPlatformPkg/Test/Library/TestPointCheckLib/DxeCheckA
> cpi.c
> index 263781a2..3828ab72 100644
> ---
> a/Platform/Intel/MinPlatformPkg/Test/Library/TestPointCheckLib/DxeCheckA
> cpi.c
> +++ b/Platform/Intel/MinPlatformPkg/Test/Library/TestPointCheckLib/DxeCh
> +++ eckAcpi.c
> @@ -611,6 +611,9 @@ DumpAcpiRsdt (
>    if (OutTable != NULL) {
>      *OutTable = NULL;
>    }
> +  else{
> +    return EFI_INVALID_PARAMETER;
> +  }
> 
>    ReturnStatus = EFI_SUCCESS;
>    EntryCount = (Rsdt->Length - sizeof (EFI_ACPI_DESCRIPTION_HEADER)) /
> sizeof(UINT32); @@ -664,6 +667,9 @@ DumpAcpiXsdt (
>    if (OutTable != NULL) {
>      *OutTable = NULL;
>    }
> +  else{
> +    return EFI_INVALID_PARAMETER;
> +  }
> 
>    ReturnStatus = EFI_SUCCESS;
>    EntryCount = (Xsdt->Length - sizeof (EFI_ACPI_DESCRIPTION_HEADER)) /
> sizeof(UINT64);
> --
> 2.18.0.windows.1

Re: [PATCH] MinPlatformPkg/TestPointCheckLib: Add check for pointer Variable

[Chiu, Chasel]

Please extend copyright, with this update Reviewed-by: Chasel Chiu <chasel.chiu@intel.com>

> -----Original Message-----
> From: Zhang, Shenglei
> Sent: Monday, September 2, 2019 8:35 PM
> To: devel@edk2.groups.io
> Cc: Kubacki, Michael A <michael.a.kubacki@intel.com>; Chiu, Chasel
> <chasel.chiu@intel.com>; Desimone, Nathaniel L
> <nathaniel.l.desimone@intel.com>; Gao, Liming <liming.gao@intel.com>
> Subject: [PATCH] MinPlatformPkg/TestPointCheckLib: Add check for pointer
> Variable
> 
> Add check for pointer Variable to ensure it is not NULL when used.
> 
> Cc: Michael Kubacki <michael.a.kubacki@intel.com>
> Cc: Chasel Chiu <chasel.chiu@intel.com>
> Cc: Nate DeSimone <nathaniel.l.desimone@intel.com>
> Cc: Liming Gao <liming.gao@intel.com>
> Signed-off-by: Shenglei Zhang <shenglei.zhang@intel.com>
> ---
>  .../Test/Library/TestPointCheckLib/DxeCheckUefiSecureBoot.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git
> a/Platform/Intel/MinPlatformPkg/Test/Library/TestPointCheckLib/DxeCheckU
> efiSecureBoot.c
> b/Platform/Intel/MinPlatformPkg/Test/Library/TestPointCheckLib/DxeCheckU
> efiSecureBoot.c
> index b53a09ab..f5125ede 100644
> ---
> a/Platform/Intel/MinPlatformPkg/Test/Library/TestPointCheckLib/DxeCheckU
> efiSecureBoot.c
> +++
> b/Platform/Intel/MinPlatformPkg/Test/Library/TestPointCheckLib/DxeCheckU
> efiSecureBoot.c
> @@ -52,6 +52,9 @@ TestPointCheckUefiSecureBoot (
>    ReturnStatus = EFI_SUCCESS;
>    for (Index = 0; Index <
> sizeof(mUefiSecureBootVariable)/sizeof(mUefiSecureBootVariable[0]);
> Index++) {
>      Status = GetVariable2 (mUefiSecureBootVariable[Index].Name,
> mUefiSecureBootVariable[Index].Guid, &Variable, &Size);
> +    if(Variable == NULL) {
> +      return EFI_NOT_FOUND;
> +    }
>      if (EFI_ERROR(Status)) {
>        DEBUG ((DEBUG_ERROR, "Variable - %S not found\n",
> mUefiSecureBootVariable[Index].Name));
>        ReturnStatus = Status;
> @@ -69,6 +72,9 @@ TestPointCheckUefiSecureBoot (
> 
>    for (Index = 0; Index <
> sizeof(mUefiSecureBootModeVariable)/sizeof(mUefiSecureBootModeVariabl
> e[0]); Index++) {
>      Status = GetVariable2 (mUefiSecureBootModeVariable[Index].Name,
> mUefiSecureBootModeVariable[Index].Guid, &Variable, &Size);
> +    if(Variable == NULL) {
> +      return EFI_NOT_FOUND;
> +    }
>      if (EFI_ERROR(Status)) {
>        DEBUG ((DEBUG_ERROR, "Variable - %S not found\n",
> mUefiSecureBootModeVariable[Index].Name));
>        ReturnStatus = Status;
> --
> 2.18.0.windows.1

Re: [edk2-devel] [PATCH] MinPlatformPkg/SecFspWrapperPlatformSecLib: Change TopOfTemporaryRam type

[Chiu, Chasel]

Hi Shenglei,

Would you please elaborate a little on how casting to UINTN can resolve the overflow scenario and why 64bits OS will affect this code?

Thanks!
Chasel

> -----Original Message-----
> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Zhang,
> Shenglei
> Sent: Monday, September 2, 2019 8:35 PM
> To: devel@edk2.groups.io
> Cc: Kubacki, Michael A <michael.a.kubacki@intel.com>; Chiu, Chasel
> <chasel.chiu@intel.com>; Desimone, Nathaniel L
> <nathaniel.l.desimone@intel.com>; Gao, Liming <liming.gao@intel.com>
> Subject: [edk2-devel] [PATCH] MinPlatformPkg/SecFspWrapperPlatformSecLib:
> Change TopOfTemporaryRam type
> 
> Cast TopOfTemporaryRam's from UINT32 to UINTN in the expression.
> The original code (TopOfTemporaryRam - sizeof (UINT32)) may cause
> overflow. As a result the operation under 64-bit OS environment, (UINT)(...),
> may cast a overflowed 4-byte result to 8-byte one.
> 
> Cc: Michael Kubacki <michael.a.kubacki@intel.com>
> Cc: Chasel Chiu <chasel.chiu@intel.com>
> Cc: Nate DeSimone <nathaniel.l.desimone@intel.com>
> Cc: Liming Gao <liming.gao@intel.com>
> Signed-off-by: Shenglei Zhang <shenglei.zhang@intel.com>
> ---
>  .../Library/SecFspWrapperPlatformSecLib/SecGetPerformance.c     | 2 +-
>  .../SecFspWrapperPlatformSecLib/SecPlatformInformation.c        | 2 +-
>  2 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git
> a/Platform/Intel/MinPlatformPkg/FspWrapper/Library/SecFspWrapperPlatfor
> mSecLib/SecGetPerformance.c
> b/Platform/Intel/MinPlatformPkg/FspWrapper/Library/SecFspWrapperPlatfor
> mSecLib/SecGetPerformance.c
> index c4eeb2b1..0cc42f96 100644
> ---
> a/Platform/Intel/MinPlatformPkg/FspWrapper/Library/SecFspWrapperPlatfor
> mSecLib/SecGetPerformance.c
> +++
> b/Platform/Intel/MinPlatformPkg/FspWrapper/Library/SecFspWrapperPlat
> +++ formSecLib/SecGetPerformance.c
> @@ -79,7 +79,7 @@ SecGetPerformance (
>    //
>    TopOfTemporaryRam = (UINT32)(UINTN)TopOfTemporaryRamPpi -
> sizeof(UINT32);
>    TopOfTemporaryRam -= sizeof(UINT32) * 2;
> -  Count             = *(UINT32 *) (UINTN) (TopOfTemporaryRam - sizeof
> (UINT32));
> +  Count             = *(UINT32 *)((UINTN)TopOfTemporaryRam - sizeof
> (UINT32));
>    Size              = Count * sizeof (UINT32);
> 
>    Ticker = *(UINT64 *) (UINTN) (TopOfTemporaryRam - sizeof (UINT32) - Size
> - sizeof (UINT32) * 2); diff --git
> a/Platform/Intel/MinPlatformPkg/FspWrapper/Library/SecFspWrapperPlatfor
> mSecLib/SecPlatformInformation.c
> b/Platform/Intel/MinPlatformPkg/FspWrapper/Library/SecFspWrapperPlatfor
> mSecLib/SecPlatformInformation.c
> index 5b94ed2b..1bcee5f4 100644
> ---
> a/Platform/Intel/MinPlatformPkg/FspWrapper/Library/SecFspWrapperPlatfor
> mSecLib/SecPlatformInformation.c
> +++
> b/Platform/Intel/MinPlatformPkg/FspWrapper/Library/SecFspWrapperPlat
> +++ formSecLib/SecPlatformInformation.c
> @@ -61,7 +61,7 @@ SecPlatformInformation (
>    //
>    TopOfTemporaryRam = (UINT32)(UINTN)TopOfTemporaryRamPpi - sizeof
> (UINT32);
>    TopOfTemporaryRam -= sizeof(UINT32) * 2;
> -  Count             = *((UINT32 *)(UINTN) (TopOfTemporaryRam - sizeof
> (UINT32)));
> +  Count             = *((UINT32 *)((UINTN)TopOfTemporaryRam - sizeof
> (UINT32)));
>    Size              = Count * sizeof (IA32_HANDOFF_STATUS);
> 
>    if ((*StructureSize) < (UINT64) Size) {
> --
> 2.18.0.windows.1
> 
> 
> 

Re: [edk2-devel] [PATCH] MinPlatformPkg/SecFspWrapperPlatformSecLib: Change TopOfTemporaryRam type

[Zhang, Shenglei]

Hi chasel,

> -----Original Message-----
> From: Chiu, Chasel
> Sent: Wednesday, September 4, 2019 8:13 PM
> To: devel@edk2.groups.io; Zhang, Shenglei <shenglei.zhang@intel.com>
> Cc: Kubacki, Michael A <michael.a.kubacki@intel.com>; Desimone, Nathaniel
> L <nathaniel.l.desimone@intel.com>; Gao, Liming <liming.gao@intel.com>
> Subject: RE: [edk2-devel] [PATCH]
> MinPlatformPkg/SecFspWrapperPlatformSecLib: Change
> TopOfTemporaryRam type
> 
> 
> Hi Shenglei,
> 
> Would you please elaborate a little on how casting to UINTN can resolve the
> overflow scenario and why 64bits OS will affect this code?

Actually casting to UINTN can't  resolve the overflow.
What I mean is the result of (TopOfTemporaryRam - sizeof (UINT32)) may overflow.
While it's meaningless to cast an already overflowed result to another type. So I update
the code to cast the variable to UINT before it is arithmetically operated.

Under 64bits OS, the size of UINTN is 64-bit and the original "(TopOfTemporaryRam - sizeof (UINT32)) "
is 32-bit. So the operation for casting will be performed. That's why 64bits OS will affect this code.

Thanks,
Shenglei

> 
> Thanks!
> Chasel
> 
> > -----Original Message-----
> > From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Zhang,
> > Shenglei
> > Sent: Monday, September 2, 2019 8:35 PM
> > To: devel@edk2.groups.io
> > Cc: Kubacki, Michael A <michael.a.kubacki@intel.com>; Chiu, Chasel
> > <chasel.chiu@intel.com>; Desimone, Nathaniel L
> > <nathaniel.l.desimone@intel.com>; Gao, Liming <liming.gao@intel.com>
> > Subject: [edk2-devel] [PATCH]
> MinPlatformPkg/SecFspWrapperPlatformSecLib:
> > Change TopOfTemporaryRam type
> >
> > Cast TopOfTemporaryRam's from UINT32 to UINTN in the expression.
> > The original code (TopOfTemporaryRam - sizeof (UINT32)) may cause
> > overflow. As a result the operation under 64-bit OS environment,
> (UINT)(...),
> > may cast a overflowed 4-byte result to 8-byte one.
> >
> > Cc: Michael Kubacki <michael.a.kubacki@intel.com>
> > Cc: Chasel Chiu <chasel.chiu@intel.com>
> > Cc: Nate DeSimone <nathaniel.l.desimone@intel.com>
> > Cc: Liming Gao <liming.gao@intel.com>
> > Signed-off-by: Shenglei Zhang <shenglei.zhang@intel.com>
> > ---
> >  .../Library/SecFspWrapperPlatformSecLib/SecGetPerformance.c     | 2 +-
> >  .../SecFspWrapperPlatformSecLib/SecPlatformInformation.c        | 2 +-
> >  2 files changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git
> >
> a/Platform/Intel/MinPlatformPkg/FspWrapper/Library/SecFspWrapperPlatf
> or
> > mSecLib/SecGetPerformance.c
> >
> b/Platform/Intel/MinPlatformPkg/FspWrapper/Library/SecFspWrapperPlatf
> or
> > mSecLib/SecGetPerformance.c
> > index c4eeb2b1..0cc42f96 100644
> > ---
> >
> a/Platform/Intel/MinPlatformPkg/FspWrapper/Library/SecFspWrapperPlatf
> or
> > mSecLib/SecGetPerformance.c
> > +++
> > b/Platform/Intel/MinPlatformPkg/FspWrapper/Library/SecFspWrapperPlat
> > +++ formSecLib/SecGetPerformance.c
> > @@ -79,7 +79,7 @@ SecGetPerformance (
> >    //
> >    TopOfTemporaryRam = (UINT32)(UINTN)TopOfTemporaryRamPpi -
> > sizeof(UINT32);
> >    TopOfTemporaryRam -= sizeof(UINT32) * 2;
> > -  Count             = *(UINT32 *) (UINTN) (TopOfTemporaryRam - sizeof
> > (UINT32));
> > +  Count             = *(UINT32 *)((UINTN)TopOfTemporaryRam - sizeof
> > (UINT32));
> >    Size              = Count * sizeof (UINT32);
> >
> >    Ticker = *(UINT64 *) (UINTN) (TopOfTemporaryRam - sizeof (UINT32) -
> Size
> > - sizeof (UINT32) * 2); diff --git
> >
> a/Platform/Intel/MinPlatformPkg/FspWrapper/Library/SecFspWrapperPlatf
> or
> > mSecLib/SecPlatformInformation.c
> >
> b/Platform/Intel/MinPlatformPkg/FspWrapper/Library/SecFspWrapperPlatf
> or
> > mSecLib/SecPlatformInformation.c
> > index 5b94ed2b..1bcee5f4 100644
> > ---
> >
> a/Platform/Intel/MinPlatformPkg/FspWrapper/Library/SecFspWrapperPlatf
> or
> > mSecLib/SecPlatformInformation.c
> > +++
> > b/Platform/Intel/MinPlatformPkg/FspWrapper/Library/SecFspWrapperPlat
> > +++ formSecLib/SecPlatformInformation.c
> > @@ -61,7 +61,7 @@ SecPlatformInformation (
> >    //
> >    TopOfTemporaryRam = (UINT32)(UINTN)TopOfTemporaryRamPpi - sizeof
> > (UINT32);
> >    TopOfTemporaryRam -= sizeof(UINT32) * 2;
> > -  Count             = *((UINT32 *)(UINTN) (TopOfTemporaryRam - sizeof
> > (UINT32)));
> > +  Count             = *((UINT32 *)((UINTN)TopOfTemporaryRam - sizeof
> > (UINT32)));
> >    Size              = Count * sizeof (IA32_HANDOFF_STATUS);
> >
> >    if ((*StructureSize) < (UINT64) Size) {
> > --
> > 2.18.0.windows.1
> >
> >
> > 

Re: [edk2-devel] [PATCH] MinPlatformPkg/SecFspWrapperPlatformSecLib: Change TopOfTemporaryRam type

[Chiu, Chasel]

Thanks for explanations.
So looks like the intention is to support X64 build scenarios, I think TopOfTemporaryRam should be defined as UINTN from beginning so it can hold 64bits address in X64 build.
Please evaluate if we could make these changes.

Thanks!
Chasel

> -----Original Message-----
> From: Zhang, Shenglei
> Sent: Thursday, September 5, 2019 10:11 AM
> To: Chiu, Chasel <chasel.chiu@intel.com>; devel@edk2.groups.io
> Cc: Kubacki, Michael A <michael.a.kubacki@intel.com>; Desimone, Nathaniel
> L <nathaniel.l.desimone@intel.com>; Gao, Liming <liming.gao@intel.com>
> Subject: RE: [edk2-devel] [PATCH]
> MinPlatformPkg/SecFspWrapperPlatformSecLib: Change
> TopOfTemporaryRam type
> 
> Hi chasel,
> 
> > -----Original Message-----
> > From: Chiu, Chasel
> > Sent: Wednesday, September 4, 2019 8:13 PM
> > To: devel@edk2.groups.io; Zhang, Shenglei <shenglei.zhang@intel.com>
> > Cc: Kubacki, Michael A <michael.a.kubacki@intel.com>; Desimone,
> > Nathaniel L <nathaniel.l.desimone@intel.com>; Gao, Liming
> > <liming.gao@intel.com>
> > Subject: RE: [edk2-devel] [PATCH]
> > MinPlatformPkg/SecFspWrapperPlatformSecLib: Change
> TopOfTemporaryRam
> > type
> >
> >
> > Hi Shenglei,
> >
> > Would you please elaborate a little on how casting to UINTN can
> > resolve the overflow scenario and why 64bits OS will affect this code?
> 
> Actually casting to UINTN can't  resolve the overflow.
> What I mean is the result of (TopOfTemporaryRam - sizeof (UINT32)) may
> overflow.
> While it's meaningless to cast an already overflowed result to another type.
> So I update the code to cast the variable to UINT before it is arithmetically
> operated.
> 
> Under 64bits OS, the size of UINTN is 64-bit and the original
> "(TopOfTemporaryRam - sizeof (UINT32)) "
> is 32-bit. So the operation for casting will be performed. That's why 64bits
> OS will affect this code.
> 
> Thanks,
> Shenglei
> 
> >
> > Thanks!
> > Chasel
> >
> > > -----Original Message-----
> > > From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of
> > > Zhang, Shenglei
> > > Sent: Monday, September 2, 2019 8:35 PM
> > > To: devel@edk2.groups.io
> > > Cc: Kubacki, Michael A <michael.a.kubacki@intel.com>; Chiu, Chasel
> > > <chasel.chiu@intel.com>; Desimone, Nathaniel L
> > > <nathaniel.l.desimone@intel.com>; Gao, Liming <liming.gao@intel.com>
> > > Subject: [edk2-devel] [PATCH]
> > MinPlatformPkg/SecFspWrapperPlatformSecLib:
> > > Change TopOfTemporaryRam type
> > >
> > > Cast TopOfTemporaryRam's from UINT32 to UINTN in the expression.
> > > The original code (TopOfTemporaryRam - sizeof (UINT32)) may cause
> > > overflow. As a result the operation under 64-bit OS environment,
> > (UINT)(...),
> > > may cast a overflowed 4-byte result to 8-byte one.
> > >
> > > Cc: Michael Kubacki <michael.a.kubacki@intel.com>
> > > Cc: Chasel Chiu <chasel.chiu@intel.com>
> > > Cc: Nate DeSimone <nathaniel.l.desimone@intel.com>
> > > Cc: Liming Gao <liming.gao@intel.com>
> > > Signed-off-by: Shenglei Zhang <shenglei.zhang@intel.com>
> > > ---
> > >  .../Library/SecFspWrapperPlatformSecLib/SecGetPerformance.c     | 2
> +-
> > >  .../SecFspWrapperPlatformSecLib/SecPlatformInformation.c        | 2
> +-
> > >  2 files changed, 2 insertions(+), 2 deletions(-)
> > >
> > > diff --git
> > >
> > a/Platform/Intel/MinPlatformPkg/FspWrapper/Library/SecFspWrapperPlatf
> > or
> > > mSecLib/SecGetPerformance.c
> > >
> > b/Platform/Intel/MinPlatformPkg/FspWrapper/Library/SecFspWrapperPlatf
> > or
> > > mSecLib/SecGetPerformance.c
> > > index c4eeb2b1..0cc42f96 100644
> > > ---
> > >
> > a/Platform/Intel/MinPlatformPkg/FspWrapper/Library/SecFspWrapperPlatf
> > or
> > > mSecLib/SecGetPerformance.c
> > > +++
> > >
> b/Platform/Intel/MinPlatformPkg/FspWrapper/Library/SecFspWrapperPlat
> > > +++ formSecLib/SecGetPerformance.c
> > > @@ -79,7 +79,7 @@ SecGetPerformance (
> > >    //
> > >    TopOfTemporaryRam = (UINT32)(UINTN)TopOfTemporaryRamPpi -
> > > sizeof(UINT32);
> > >    TopOfTemporaryRam -= sizeof(UINT32) * 2;
> > > -  Count             = *(UINT32 *) (UINTN) (TopOfTemporaryRam -
> sizeof
> > > (UINT32));
> > > +  Count             = *(UINT32 *)((UINTN)TopOfTemporaryRam -
> sizeof
> > > (UINT32));
> > >    Size              = Count * sizeof (UINT32);
> > >
> > >    Ticker = *(UINT64 *) (UINTN) (TopOfTemporaryRam - sizeof (UINT32)
> > > -
> > Size
> > > - sizeof (UINT32) * 2); diff --git
> > >
> > a/Platform/Intel/MinPlatformPkg/FspWrapper/Library/SecFspWrapperPlatf
> > or
> > > mSecLib/SecPlatformInformation.c
> > >
> > b/Platform/Intel/MinPlatformPkg/FspWrapper/Library/SecFspWrapperPlatf
> > or
> > > mSecLib/SecPlatformInformation.c
> > > index 5b94ed2b..1bcee5f4 100644
> > > ---
> > >
> > a/Platform/Intel/MinPlatformPkg/FspWrapper/Library/SecFspWrapperPlatf
> > or
> > > mSecLib/SecPlatformInformation.c
> > > +++
> > >
> b/Platform/Intel/MinPlatformPkg/FspWrapper/Library/SecFspWrapperPlat
> > > +++ formSecLib/SecPlatformInformation.c
> > > @@ -61,7 +61,7 @@ SecPlatformInformation (
> > >    //
> > >    TopOfTemporaryRam = (UINT32)(UINTN)TopOfTemporaryRamPpi -
> sizeof
> > > (UINT32);
> > >    TopOfTemporaryRam -= sizeof(UINT32) * 2;
> > > -  Count             = *((UINT32 *)(UINTN) (TopOfTemporaryRam -
> sizeof
> > > (UINT32)));
> > > +  Count             = *((UINT32 *)((UINTN)TopOfTemporaryRam -
> sizeof
> > > (UINT32)));
> > >    Size              = Count * sizeof (IA32_HANDOFF_STATUS);
> > >
> > >    if ((*StructureSize) < (UINT64) Size) {
> > > --
> > > 2.18.0.windows.1
> > >
> > >
> > > 

Re: [edk2-devel] [PATCH] MinPlatformPkg/SecFspWrapperPlatformSecLib: Change TopOfTemporaryRam type

[Zhang, Shenglei]

It works after I apply the change according to your advice.
And that makes the code more clean. Thanks for your advice.
I'll sent out a v2 patch.

Best Regards,
Shenglei

> -----Original Message-----
> From: Chiu, Chasel
> Sent: Thursday, September 5, 2019 10:54 AM
> To: Zhang, Shenglei <shenglei.zhang@intel.com>; devel@edk2.groups.io
> Cc: Kubacki, Michael A <michael.a.kubacki@intel.com>; Desimone, Nathaniel
> L <nathaniel.l.desimone@intel.com>; Gao, Liming <liming.gao@intel.com>
> Subject: RE: [edk2-devel] [PATCH]
> MinPlatformPkg/SecFspWrapperPlatformSecLib: Change
> TopOfTemporaryRam type
> 
> 
> Thanks for explanations.
> So looks like the intention is to support X64 build scenarios, I think
> TopOfTemporaryRam should be defined as UINTN from beginning so it can
> hold 64bits address in X64 build.
> Please evaluate if we could make these changes.
> 
> Thanks!
> Chasel
> 
> > -----Original Message-----
> > From: Zhang, Shenglei
> > Sent: Thursday, September 5, 2019 10:11 AM
> > To: Chiu, Chasel <chasel.chiu@intel.com>; devel@edk2.groups.io
> > Cc: Kubacki, Michael A <michael.a.kubacki@intel.com>; Desimone,
> Nathaniel
> > L <nathaniel.l.desimone@intel.com>; Gao, Liming <liming.gao@intel.com>
> > Subject: RE: [edk2-devel] [PATCH]
> > MinPlatformPkg/SecFspWrapperPlatformSecLib: Change
> > TopOfTemporaryRam type
> >
> > Hi chasel,
> >
> > > -----Original Message-----
> > > From: Chiu, Chasel
> > > Sent: Wednesday, September 4, 2019 8:13 PM
> > > To: devel@edk2.groups.io; Zhang, Shenglei <shenglei.zhang@intel.com>
> > > Cc: Kubacki, Michael A <michael.a.kubacki@intel.com>; Desimone,
> > > Nathaniel L <nathaniel.l.desimone@intel.com>; Gao, Liming
> > > <liming.gao@intel.com>
> > > Subject: RE: [edk2-devel] [PATCH]
> > > MinPlatformPkg/SecFspWrapperPlatformSecLib: Change
> > TopOfTemporaryRam
> > > type
> > >
> > >
> > > Hi Shenglei,
> > >
> > > Would you please elaborate a little on how casting to UINTN can
> > > resolve the overflow scenario and why 64bits OS will affect this code?
> >
> > Actually casting to UINTN can't  resolve the overflow.
> > What I mean is the result of (TopOfTemporaryRam - sizeof (UINT32)) may
> > overflow.
> > While it's meaningless to cast an already overflowed result to another type.
> > So I update the code to cast the variable to UINT before it is arithmetically
> > operated.
> >
> > Under 64bits OS, the size of UINTN is 64-bit and the original
> > "(TopOfTemporaryRam - sizeof (UINT32)) "
> > is 32-bit. So the operation for casting will be performed. That's why 64bits
> > OS will affect this code.
> >
> > Thanks,
> > Shenglei
> >
> > >
> > > Thanks!
> > > Chasel
> > >
> > > > -----Original Message-----
> > > > From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of
> > > > Zhang, Shenglei
> > > > Sent: Monday, September 2, 2019 8:35 PM
> > > > To: devel@edk2.groups.io
> > > > Cc: Kubacki, Michael A <michael.a.kubacki@intel.com>; Chiu, Chasel
> > > > <chasel.chiu@intel.com>; Desimone, Nathaniel L
> > > > <nathaniel.l.desimone@intel.com>; Gao, Liming <liming.gao@intel.com>
> > > > Subject: [edk2-devel] [PATCH]
> > > MinPlatformPkg/SecFspWrapperPlatformSecLib:
> > > > Change TopOfTemporaryRam type
> > > >
> > > > Cast TopOfTemporaryRam's from UINT32 to UINTN in the expression.
> > > > The original code (TopOfTemporaryRam - sizeof (UINT32)) may cause
> > > > overflow. As a result the operation under 64-bit OS environment,
> > > (UINT)(...),
> > > > may cast a overflowed 4-byte result to 8-byte one.
> > > >
> > > > Cc: Michael Kubacki <michael.a.kubacki@intel.com>
> > > > Cc: Chasel Chiu <chasel.chiu@intel.com>
> > > > Cc: Nate DeSimone <nathaniel.l.desimone@intel.com>
> > > > Cc: Liming Gao <liming.gao@intel.com>
> > > > Signed-off-by: Shenglei Zhang <shenglei.zhang@intel.com>
> > > > ---
> > > >  .../Library/SecFspWrapperPlatformSecLib/SecGetPerformance.c     | 2
> > +-
> > > >  .../SecFspWrapperPlatformSecLib/SecPlatformInformation.c        | 2
> > +-
> > > >  2 files changed, 2 insertions(+), 2 deletions(-)
> > > >
> > > > diff --git
> > > >
> > >
> a/Platform/Intel/MinPlatformPkg/FspWrapper/Library/SecFspWrapperPlatf
> > > or
> > > > mSecLib/SecGetPerformance.c
> > > >
> > >
> b/Platform/Intel/MinPlatformPkg/FspWrapper/Library/SecFspWrapperPlatf
> > > or
> > > > mSecLib/SecGetPerformance.c
> > > > index c4eeb2b1..0cc42f96 100644
> > > > ---
> > > >
> > >
> a/Platform/Intel/MinPlatformPkg/FspWrapper/Library/SecFspWrapperPlatf
> > > or
> > > > mSecLib/SecGetPerformance.c
> > > > +++
> > > >
> > b/Platform/Intel/MinPlatformPkg/FspWrapper/Library/SecFspWrapperPlat
> > > > +++ formSecLib/SecGetPerformance.c
> > > > @@ -79,7 +79,7 @@ SecGetPerformance (
> > > >    //
> > > >    TopOfTemporaryRam = (UINT32)(UINTN)TopOfTemporaryRamPpi -
> > > > sizeof(UINT32);
> > > >    TopOfTemporaryRam -= sizeof(UINT32) * 2;
> > > > -  Count             = *(UINT32 *) (UINTN) (TopOfTemporaryRam -
> > sizeof
> > > > (UINT32));
> > > > +  Count             = *(UINT32 *)((UINTN)TopOfTemporaryRam -
> > sizeof
> > > > (UINT32));
> > > >    Size              = Count * sizeof (UINT32);
> > > >
> > > >    Ticker = *(UINT64 *) (UINTN) (TopOfTemporaryRam - sizeof (UINT32)
> > > > -
> > > Size
> > > > - sizeof (UINT32) * 2); diff --git
> > > >
> > >
> a/Platform/Intel/MinPlatformPkg/FspWrapper/Library/SecFspWrapperPlatf
> > > or
> > > > mSecLib/SecPlatformInformation.c
> > > >
> > >
> b/Platform/Intel/MinPlatformPkg/FspWrapper/Library/SecFspWrapperPlatf
> > > or
> > > > mSecLib/SecPlatformInformation.c
> > > > index 5b94ed2b..1bcee5f4 100644
> > > > ---
> > > >
> > >
> a/Platform/Intel/MinPlatformPkg/FspWrapper/Library/SecFspWrapperPlatf
> > > or
> > > > mSecLib/SecPlatformInformation.c
> > > > +++
> > > >
> > b/Platform/Intel/MinPlatformPkg/FspWrapper/Library/SecFspWrapperPlat
> > > > +++ formSecLib/SecPlatformInformation.c
> > > > @@ -61,7 +61,7 @@ SecPlatformInformation (
> > > >    //
> > > >    TopOfTemporaryRam = (UINT32)(UINTN)TopOfTemporaryRamPpi -
> > sizeof
> > > > (UINT32);
> > > >    TopOfTemporaryRam -= sizeof(UINT32) * 2;
> > > > -  Count             = *((UINT32 *)(UINTN) (TopOfTemporaryRam -
> > sizeof
> > > > (UINT32)));
> > > > +  Count             = *((UINT32 *)((UINTN)TopOfTemporaryRam -
> > sizeof
> > > > (UINT32)));
> > > >    Size              = Count * sizeof (IA32_HANDOFF_STATUS);
> > > >
> > > >    if ((*StructureSize) < (UINT64) Size) {
> > > > --
> > > > 2.18.0.windows.1
> > > >
> > > >
> > > > 

Re: [PATCH] MinPlatformPkg/TestPointCheckLib: Add check for pointers

[Nate DeSimone]

Hi Shenglei,

Please send a second patch series with the copyrights updated.

Thanks,
Nate

-----Original Message-----
From: Zhang, Shenglei 
Sent: Monday, September 2, 2019 5:35 AM
To: devel@edk2.groups.io
Cc: Kubacki, Michael A <michael.a.kubacki@intel.com>; Chiu, Chasel <chasel.chiu@intel.com>; Desimone, Nathaniel L <nathaniel.l.desimone@intel.com>; Gao, Liming <liming.gao@intel.com>
Subject: [PATCH] MinPlatformPkg/TestPointCheckLib: Add check for pointers

In DxeCheckBootVariable.c, add check for BootOrder and Variable that return EFI_NOT_FOUND when they are NULL.
In DxeCheckGcd.c, add check for GcdIoMap to ensure it not NULL when allocating memory to what it points to.

Cc: Michael Kubacki <michael.a.kubacki@intel.com>
Cc: Chasel Chiu <chasel.chiu@intel.com>
Cc: Nate DeSimone <nathaniel.l.desimone@intel.com>
Cc: Liming Gao <liming.gao@intel.com>
Signed-off-by: Shenglei Zhang <shenglei.zhang@intel.com>
---
 .../Test/Library/TestPointCheckLib/DxeCheckBootVariable.c   | 6 ++++++
 .../Test/Library/TestPointCheckLib/DxeCheckGcd.c            | 4 +++-
 2 files changed, 9 insertions(+), 1 deletion(-)

diff --git a/Platform/Intel/MinPlatformPkg/Test/Library/TestPointCheckLib/DxeCheckBootVariable.c b/Platform/Intel/MinPlatformPkg/Test/Library/TestPointCheckLib/DxeCheckBootVariable.c
index 85bd5b3d..a9af33a3 100644
--- a/Platform/Intel/MinPlatformPkg/Test/Library/TestPointCheckLib/DxeCheckBootVariable.c
+++ b/Platform/Intel/MinPlatformPkg/Test/Library/TestPointCheckLib/DxeCh
+++ eckBootVariable.c
@@ -130,6 +130,9 @@ TestPointCheckLoadOptionVariable (
   for (ListIndex = 0; ListIndex < sizeof(mLoadOptionVariableList)/sizeof(mLoadOptionVariableList[0]); ListIndex++) {
     UnicodeSPrint (BootOrderName, sizeof(BootOrderName), L"%sOrder", mLoadOptionVariableList[ListIndex]);
     Status = GetVariable2 (BootOrderName, &gEfiGlobalVariableGuid, (VOID **)&BootOrder, &OrderSize);
+	if(BootOrder == NULL) {
+      return EFI_NOT_FOUND;;
+	}
     if (EFI_ERROR(Status)) {
       continue;
     }
@@ -222,6 +225,9 @@ TestPointCheckKeyOptionVariable (
     for (Index = 0; ; Index++) {
       UnicodeSPrint (KeyOptionName, sizeof(KeyOptionName), L"%s%04x", mKeyOptionVariableList[ListIndex], Index);
       Status = GetVariable2 (KeyOptionName, &gEfiGlobalVariableGuid, &Variable, &Size);
+      if(Variable == NULL) {
+        return EFI_NOT_FOUND;;
+      }
       if (!EFI_ERROR(Status)) {
         DumpKeyOption (KeyOptionName, Variable, Size);
       } else {
diff --git a/Platform/Intel/MinPlatformPkg/Test/Library/TestPointCheckLib/DxeCheckGcd.c b/Platform/Intel/MinPlatformPkg/Test/Library/TestPointCheckLib/DxeCheckGcd.c
index 82709d44..989606b6 100644
--- a/Platform/Intel/MinPlatformPkg/Test/Library/TestPointCheckLib/DxeCheckGcd.c
+++ b/Platform/Intel/MinPlatformPkg/Test/Library/TestPointCheckLib/DxeCh
+++ eckGcd.c
@@ -241,7 +241,9 @@ TestPointDumpGcd (
       }
     }
     if (GcdMemoryMap != NULL) {
-      *GcdIoMap = AllocateCopyPool (NumberOfDescriptors * sizeof(EFI_GCD_IO_SPACE_DESCRIPTOR), IoMap);
+      if (GcdIoMap != NULL){
+        *GcdIoMap = AllocateCopyPool (NumberOfDescriptors * sizeof(EFI_GCD_IO_SPACE_DESCRIPTOR), IoMap);
+      }
       *GcdIoMapNumberOfDescriptors = NumberOfDescriptors;
     }
   }
--
2.18.0.windows.1