* [PATCH] MinPlatformPkg/TestPointCheckLib: Add return value when OutTable is NULL
2019-09-02 12:34 [PATCH] MinPlatformPkg/SecFspWrapperPlatformSecLib: Change TopOfTemporaryRam type Zhang, Shenglei
@ 2019-09-02 12:34 ` Zhang, Shenglei
2019-09-02 14:04 ` Chiu, Chasel
2019-09-02 12:34 ` [PATCH] MinPlatformPkg/TestPointCheckLib: Add check for pointers Zhang, Shenglei
` (2 subsequent siblings)
3 siblings, 1 reply; 12+ messages in thread
From: Zhang, Shenglei @ 2019-09-02 12:34 UTC (permalink / raw)
To: devel; +Cc: Michael Kubacki, Chasel Chiu, Nate DeSimone, Liming Gao
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
^ permalink raw reply related [flat|nested] 12+ messages in thread* Re: [PATCH] MinPlatformPkg/TestPointCheckLib: Add return value when OutTable is NULL
2019-09-02 12:34 ` [PATCH] MinPlatformPkg/TestPointCheckLib: Add return value when OutTable is NULL Zhang, Shenglei
@ 2019-09-02 14:04 ` Chiu, Chasel
0 siblings, 0 replies; 12+ messages in thread
From: Chiu, Chasel @ 2019-09-02 14:04 UTC (permalink / raw)
To: Zhang, Shenglei, devel@edk2.groups.io
Cc: Kubacki, Michael A, Desimone, Nathaniel L, Gao, Liming
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
^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH] MinPlatformPkg/TestPointCheckLib: Add check for pointers
2019-09-02 12:34 [PATCH] MinPlatformPkg/SecFspWrapperPlatformSecLib: Change TopOfTemporaryRam type Zhang, Shenglei
2019-09-02 12:34 ` [PATCH] MinPlatformPkg/TestPointCheckLib: Add return value when OutTable is NULL Zhang, Shenglei
@ 2019-09-02 12:34 ` Zhang, Shenglei
2019-09-02 14:01 ` Chiu, Chasel
2019-09-10 22:28 ` Nate DeSimone
2019-09-02 12:34 ` [PATCH] MinPlatformPkg/TestPointCheckLib: Add check for pointer Variable Zhang, Shenglei
2019-09-04 12:13 ` [edk2-devel] [PATCH] MinPlatformPkg/SecFspWrapperPlatformSecLib: Change TopOfTemporaryRam type Chiu, Chasel
3 siblings, 2 replies; 12+ messages in thread
From: Zhang, Shenglei @ 2019-09-02 12:34 UTC (permalink / raw)
To: devel; +Cc: Michael Kubacki, Chasel Chiu, Nate DeSimone, Liming Gao
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
^ permalink raw reply related [flat|nested] 12+ messages in thread* Re: [PATCH] MinPlatformPkg/TestPointCheckLib: Add check for pointers
2019-09-02 12:34 ` [PATCH] MinPlatformPkg/TestPointCheckLib: Add check for pointers Zhang, Shenglei
@ 2019-09-02 14:01 ` Chiu, Chasel
2019-09-10 22:28 ` Nate DeSimone
1 sibling, 0 replies; 12+ messages in thread
From: Chiu, Chasel @ 2019-09-02 14:01 UTC (permalink / raw)
To: Zhang, Shenglei, devel@edk2.groups.io
Cc: Kubacki, Michael A, Desimone, Nathaniel L, Gao, Liming
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
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [PATCH] MinPlatformPkg/TestPointCheckLib: Add check for pointers
2019-09-02 12:34 ` [PATCH] MinPlatformPkg/TestPointCheckLib: Add check for pointers Zhang, Shenglei
2019-09-02 14:01 ` Chiu, Chasel
@ 2019-09-10 22:28 ` Nate DeSimone
1 sibling, 0 replies; 12+ messages in thread
From: Nate DeSimone @ 2019-09-10 22:28 UTC (permalink / raw)
To: Zhang, Shenglei, devel@edk2.groups.io
Cc: Kubacki, Michael A, Chiu, Chasel, Gao, Liming
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
^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH] MinPlatformPkg/TestPointCheckLib: Add check for pointer Variable
2019-09-02 12:34 [PATCH] MinPlatformPkg/SecFspWrapperPlatformSecLib: Change TopOfTemporaryRam type Zhang, Shenglei
2019-09-02 12:34 ` [PATCH] MinPlatformPkg/TestPointCheckLib: Add return value when OutTable is NULL Zhang, Shenglei
2019-09-02 12:34 ` [PATCH] MinPlatformPkg/TestPointCheckLib: Add check for pointers Zhang, Shenglei
@ 2019-09-02 12:34 ` Zhang, Shenglei
2019-09-02 14:06 ` Chiu, Chasel
2019-09-04 12:13 ` [edk2-devel] [PATCH] MinPlatformPkg/SecFspWrapperPlatformSecLib: Change TopOfTemporaryRam type Chiu, Chasel
3 siblings, 1 reply; 12+ messages in thread
From: Zhang, Shenglei @ 2019-09-02 12:34 UTC (permalink / raw)
To: devel; +Cc: Michael Kubacki, Chasel Chiu, Nate DeSimone, Liming Gao
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
^ permalink raw reply related [flat|nested] 12+ messages in thread* Re: [PATCH] MinPlatformPkg/TestPointCheckLib: Add check for pointer Variable
2019-09-02 12:34 ` [PATCH] MinPlatformPkg/TestPointCheckLib: Add check for pointer Variable Zhang, Shenglei
@ 2019-09-02 14:06 ` Chiu, Chasel
0 siblings, 0 replies; 12+ messages in thread
From: Chiu, Chasel @ 2019-09-02 14:06 UTC (permalink / raw)
To: Zhang, Shenglei, devel@edk2.groups.io
Cc: Kubacki, Michael A, Desimone, Nathaniel L, Gao, Liming
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
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [edk2-devel] [PATCH] MinPlatformPkg/SecFspWrapperPlatformSecLib: Change TopOfTemporaryRam type
2019-09-02 12:34 [PATCH] MinPlatformPkg/SecFspWrapperPlatformSecLib: Change TopOfTemporaryRam type Zhang, Shenglei
` (2 preceding siblings ...)
2019-09-02 12:34 ` [PATCH] MinPlatformPkg/TestPointCheckLib: Add check for pointer Variable Zhang, Shenglei
@ 2019-09-04 12:13 ` Chiu, Chasel
2019-09-05 2:11 ` Zhang, Shenglei
3 siblings, 1 reply; 12+ messages in thread
From: Chiu, Chasel @ 2019-09-04 12:13 UTC (permalink / raw)
To: devel@edk2.groups.io, Zhang, Shenglei
Cc: Kubacki, Michael A, Desimone, Nathaniel L, Gao, Liming
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
>
>
>
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [edk2-devel] [PATCH] MinPlatformPkg/SecFspWrapperPlatformSecLib: Change TopOfTemporaryRam type
2019-09-04 12:13 ` [edk2-devel] [PATCH] MinPlatformPkg/SecFspWrapperPlatformSecLib: Change TopOfTemporaryRam type Chiu, Chasel
@ 2019-09-05 2:11 ` Zhang, Shenglei
2019-09-05 2:53 ` Chiu, Chasel
0 siblings, 1 reply; 12+ messages in thread
From: Zhang, Shenglei @ 2019-09-05 2:11 UTC (permalink / raw)
To: Chiu, Chasel, devel@edk2.groups.io
Cc: Kubacki, Michael A, Desimone, Nathaniel L, Gao, Liming
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
> >
> >
> >
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [edk2-devel] [PATCH] MinPlatformPkg/SecFspWrapperPlatformSecLib: Change TopOfTemporaryRam type
2019-09-05 2:11 ` Zhang, Shenglei
@ 2019-09-05 2:53 ` Chiu, Chasel
2019-09-05 5:51 ` Zhang, Shenglei
0 siblings, 1 reply; 12+ messages in thread
From: Chiu, Chasel @ 2019-09-05 2:53 UTC (permalink / raw)
To: Zhang, Shenglei, devel@edk2.groups.io
Cc: Kubacki, Michael A, Desimone, Nathaniel L, Gao, Liming
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
> > >
> > >
> > >
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [edk2-devel] [PATCH] MinPlatformPkg/SecFspWrapperPlatformSecLib: Change TopOfTemporaryRam type
2019-09-05 2:53 ` Chiu, Chasel
@ 2019-09-05 5:51 ` Zhang, Shenglei
0 siblings, 0 replies; 12+ messages in thread
From: Zhang, Shenglei @ 2019-09-05 5:51 UTC (permalink / raw)
To: Chiu, Chasel, devel@edk2.groups.io
Cc: Kubacki, Michael A, Desimone, Nathaniel L, Gao, Liming
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
> > > >
> > > >
> > > >
^ permalink raw reply [flat|nested] 12+ messages in thread