UefiCpuPkg/MtrrLib: Fix MtrrDebugPrintAllMtrrsWorker to avoid hang

[edk2] [PATCH] UefiCpuPkg/MtrrLib: Fix MtrrDebugPrintAllMtrrsWorker to avoid hang

Posted by Ruiyu Ni 6 years, 6 months ago

ARRAY_SIZE(Mtrrs->Variables.Mtrr) was used in
MtrrDebugPrintAllMtrrsWorker() to parse the MTRR registers.
Instead, the actual variable MTRR count should be used.
Otherwise, the uninitialized random data in MtrrSetting may cause
MtrrLibSetMemoryType() hang.

Steven Shi found this bug in QEMU when using Q35 chip.

Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Ruiyu Ni <ruiyu.ni@intel.com>
Cc: Steven Shi <steven.shi@intel.com>
Cc: Laszlo Ersek <lersek@redhat.com>
---
 UefiCpuPkg/Library/MtrrLib/MtrrLib.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/UefiCpuPkg/Library/MtrrLib/MtrrLib.c b/UefiCpuPkg/Library/MtrrLib/MtrrLib.c
index 2fd1d0153e..cb22558103 100644
--- a/UefiCpuPkg/Library/MtrrLib/MtrrLib.c
+++ b/UefiCpuPkg/Library/MtrrLib/MtrrLib.c
@@ -2776,6 +2776,7 @@ MtrrDebugPrintAllMtrrsWorker (
     UINTN             RangeCount;
     UINT64            MtrrValidBitsMask;
     UINT64            MtrrValidAddressMask;
+    UINT32            VariableMtrrCount;
     MTRR_MEMORY_RANGE Ranges[
       ARRAY_SIZE (mMtrrLibFixedMtrrTable) * sizeof (UINT64) + 2 * ARRAY_SIZE (Mtrrs->Variables.Mtrr) + 1
       ];
@@ -2785,6 +2786,8 @@ MtrrDebugPrintAllMtrrsWorker (
       return;
     }
 
+    VariableMtrrCount = GetVariableMtrrCountWorker ();
+
     if (MtrrSetting != NULL) {
       Mtrrs = MtrrSetting;
     } else {
@@ -2802,7 +2805,7 @@ MtrrDebugPrintAllMtrrsWorker (
       DEBUG((DEBUG_CACHE, "Fixed MTRR[%02d]   : %016lx\n", Index, Mtrrs->Fixed.Mtrr[Index]));
     }
 
-    for (Index = 0; Index < ARRAY_SIZE (Mtrrs->Variables.Mtrr); Index++) {
+    for (Index = 0; Index < VariableMtrrCount; Index++) {
       if (((MSR_IA32_MTRR_PHYSMASK_REGISTER *)&Mtrrs->Variables.Mtrr[Index].Mask)->Bits.V == 0) {
         //
         // If mask is not valid, then do not display range
@@ -2829,11 +2832,11 @@ MtrrDebugPrintAllMtrrsWorker (
     RangeCount = 1;
 
     MtrrLibGetRawVariableRanges (
-      &Mtrrs->Variables, ARRAY_SIZE (Mtrrs->Variables.Mtrr),
+      &Mtrrs->Variables, VariableMtrrCount,
       MtrrValidBitsMask, MtrrValidAddressMask, RawVariableRanges
       );
     MtrrLibApplyVariableMtrrs (
-      RawVariableRanges, ARRAY_SIZE (RawVariableRanges),
+      RawVariableRanges, VariableMtrrCount,
       Ranges, ARRAY_SIZE (Ranges), &RangeCount
       );
 
-- 
2.12.2.windows.2

_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [PATCH] UefiCpuPkg/MtrrLib: Fix MtrrDebugPrintAllMtrrsWorker to avoid hang

Posted by Shi, Steven 6 years, 6 months ago

Reviewed-by: Steven Shi <steven.shi@intel.com>


Steven Shi
Intel\SSG\STO\UEFI Firmware

Tel: +86 021-61166522
iNet: 821-6522


> -----Original Message-----
> From: Ni, Ruiyu
> Sent: Tuesday, October 17, 2017 9:47 AM
> To: edk2-devel@lists.01.org
> Cc: Shi, Steven <steven.shi@intel.com>; Laszlo Ersek <lersek@redhat.com>
> Subject: [PATCH] UefiCpuPkg/MtrrLib: Fix MtrrDebugPrintAllMtrrsWorker to
> avoid hang
> 
> ARRAY_SIZE(Mtrrs->Variables.Mtrr) was used in
> MtrrDebugPrintAllMtrrsWorker() to parse the MTRR registers.
> Instead, the actual variable MTRR count should be used.
> Otherwise, the uninitialized random data in MtrrSetting may cause
> MtrrLibSetMemoryType() hang.
> 
> Steven Shi found this bug in QEMU when using Q35 chip.
> 
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Ruiyu Ni <ruiyu.ni@intel.com>
> Cc: Steven Shi <steven.shi@intel.com>
> Cc: Laszlo Ersek <lersek@redhat.com>
> ---
>  UefiCpuPkg/Library/MtrrLib/MtrrLib.c | 9 ++++++---
>  1 file changed, 6 insertions(+), 3 deletions(-)
> 
> diff --git a/UefiCpuPkg/Library/MtrrLib/MtrrLib.c
> b/UefiCpuPkg/Library/MtrrLib/MtrrLib.c
> index 2fd1d0153e..cb22558103 100644
> --- a/UefiCpuPkg/Library/MtrrLib/MtrrLib.c
> +++ b/UefiCpuPkg/Library/MtrrLib/MtrrLib.c
> @@ -2776,6 +2776,7 @@ MtrrDebugPrintAllMtrrsWorker (
>      UINTN             RangeCount;
>      UINT64            MtrrValidBitsMask;
>      UINT64            MtrrValidAddressMask;
> +    UINT32            VariableMtrrCount;
>      MTRR_MEMORY_RANGE Ranges[
>        ARRAY_SIZE (mMtrrLibFixedMtrrTable) * sizeof (UINT64) + 2 *
> ARRAY_SIZE (Mtrrs->Variables.Mtrr) + 1
>        ];
> @@ -2785,6 +2786,8 @@ MtrrDebugPrintAllMtrrsWorker (
>        return;
>      }
> 
> +    VariableMtrrCount = GetVariableMtrrCountWorker ();
> +
>      if (MtrrSetting != NULL) {
>        Mtrrs = MtrrSetting;
>      } else {
> @@ -2802,7 +2805,7 @@ MtrrDebugPrintAllMtrrsWorker (
>        DEBUG((DEBUG_CACHE, "Fixed MTRR[%02d]   : %016lx\n", Index, Mtrrs-
> >Fixed.Mtrr[Index]));
>      }
> 
> -    for (Index = 0; Index < ARRAY_SIZE (Mtrrs->Variables.Mtrr); Index++) {
> +    for (Index = 0; Index < VariableMtrrCount; Index++) {
>        if (((MSR_IA32_MTRR_PHYSMASK_REGISTER *)&Mtrrs-
> >Variables.Mtrr[Index].Mask)->Bits.V == 0) {
>          //
>          // If mask is not valid, then do not display range
> @@ -2829,11 +2832,11 @@ MtrrDebugPrintAllMtrrsWorker (
>      RangeCount = 1;
> 
>      MtrrLibGetRawVariableRanges (
> -      &Mtrrs->Variables, ARRAY_SIZE (Mtrrs->Variables.Mtrr),
> +      &Mtrrs->Variables, VariableMtrrCount,
>        MtrrValidBitsMask, MtrrValidAddressMask, RawVariableRanges
>        );
>      MtrrLibApplyVariableMtrrs (
> -      RawVariableRanges, ARRAY_SIZE (RawVariableRanges),
> +      RawVariableRanges, VariableMtrrCount,
>        Ranges, ARRAY_SIZE (Ranges), &RangeCount
>        );
> 
> --
> 2.12.2.windows.2

_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [PATCH] UefiCpuPkg/MtrrLib: Fix MtrrDebugPrintAllMtrrsWorker to avoid hang

Posted by Ni, Ruiyu 6 years, 6 months ago

Thanks Steven.

All,
I will check in this patch ASAP.
Because it's just a bug fix in a corner of implementation and it may cause all system hang.


-----Original Message-----
From: Shi, Steven 
Sent: Tuesday, October 17, 2017 10:04 AM
To: Ni, Ruiyu <ruiyu.ni@intel.com>; edk2-devel@lists.01.org
Cc: Laszlo Ersek <lersek@redhat.com>
Subject: RE: [PATCH] UefiCpuPkg/MtrrLib: Fix MtrrDebugPrintAllMtrrsWorker to avoid hang

Reviewed-by: Steven Shi <steven.shi@intel.com>


Steven Shi
Intel\SSG\STO\UEFI Firmware

Tel: +86 021-61166522
iNet: 821-6522


> -----Original Message-----
> From: Ni, Ruiyu
> Sent: Tuesday, October 17, 2017 9:47 AM
> To: edk2-devel@lists.01.org
> Cc: Shi, Steven <steven.shi@intel.com>; Laszlo Ersek 
> <lersek@redhat.com>
> Subject: [PATCH] UefiCpuPkg/MtrrLib: Fix MtrrDebugPrintAllMtrrsWorker 
> to avoid hang
> 
> ARRAY_SIZE(Mtrrs->Variables.Mtrr) was used in
> MtrrDebugPrintAllMtrrsWorker() to parse the MTRR registers.
> Instead, the actual variable MTRR count should be used.
> Otherwise, the uninitialized random data in MtrrSetting may cause
> MtrrLibSetMemoryType() hang.
> 
> Steven Shi found this bug in QEMU when using Q35 chip.
> 
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Ruiyu Ni <ruiyu.ni@intel.com>
> Cc: Steven Shi <steven.shi@intel.com>
> Cc: Laszlo Ersek <lersek@redhat.com>
> ---
>  UefiCpuPkg/Library/MtrrLib/MtrrLib.c | 9 ++++++---
>  1 file changed, 6 insertions(+), 3 deletions(-)
> 
> diff --git a/UefiCpuPkg/Library/MtrrLib/MtrrLib.c
> b/UefiCpuPkg/Library/MtrrLib/MtrrLib.c
> index 2fd1d0153e..cb22558103 100644
> --- a/UefiCpuPkg/Library/MtrrLib/MtrrLib.c
> +++ b/UefiCpuPkg/Library/MtrrLib/MtrrLib.c
> @@ -2776,6 +2776,7 @@ MtrrDebugPrintAllMtrrsWorker (
>      UINTN             RangeCount;
>      UINT64            MtrrValidBitsMask;
>      UINT64            MtrrValidAddressMask;
> +    UINT32            VariableMtrrCount;
>      MTRR_MEMORY_RANGE Ranges[
>        ARRAY_SIZE (mMtrrLibFixedMtrrTable) * sizeof (UINT64) + 2 * 
> ARRAY_SIZE (Mtrrs->Variables.Mtrr) + 1
>        ];
> @@ -2785,6 +2786,8 @@ MtrrDebugPrintAllMtrrsWorker (
>        return;
>      }
> 
> +    VariableMtrrCount = GetVariableMtrrCountWorker ();
> +
>      if (MtrrSetting != NULL) {
>        Mtrrs = MtrrSetting;
>      } else {
> @@ -2802,7 +2805,7 @@ MtrrDebugPrintAllMtrrsWorker (
>        DEBUG((DEBUG_CACHE, "Fixed MTRR[%02d]   : %016lx\n", Index, Mtrrs-
> >Fixed.Mtrr[Index]));
>      }
> 
> -    for (Index = 0; Index < ARRAY_SIZE (Mtrrs->Variables.Mtrr); Index++) {
> +    for (Index = 0; Index < VariableMtrrCount; Index++) {
>        if (((MSR_IA32_MTRR_PHYSMASK_REGISTER *)&Mtrrs-
> >Variables.Mtrr[Index].Mask)->Bits.V == 0) {
>          //
>          // If mask is not valid, then do not display range @@ 
> -2829,11 +2832,11 @@ MtrrDebugPrintAllMtrrsWorker (
>      RangeCount = 1;
> 
>      MtrrLibGetRawVariableRanges (
> -      &Mtrrs->Variables, ARRAY_SIZE (Mtrrs->Variables.Mtrr),
> +      &Mtrrs->Variables, VariableMtrrCount,
>        MtrrValidBitsMask, MtrrValidAddressMask, RawVariableRanges
>        );
>      MtrrLibApplyVariableMtrrs (
> -      RawVariableRanges, ARRAY_SIZE (RawVariableRanges),
> +      RawVariableRanges, VariableMtrrCount,
>        Ranges, ARRAY_SIZE (Ranges), &RangeCount
>        );
> 
> --
> 2.12.2.windows.2

_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [PATCH] UefiCpuPkg/MtrrLib: Fix MtrrDebugPrintAllMtrrsWorker to avoid hang

Posted by Laszlo Ersek 6 years, 6 months ago

On 10/17/17 03:46, Ruiyu Ni wrote:
> ARRAY_SIZE(Mtrrs->Variables.Mtrr) was used in
> MtrrDebugPrintAllMtrrsWorker() to parse the MTRR registers.
> Instead, the actual variable MTRR count should be used.
> Otherwise, the uninitialized random data in MtrrSetting may cause
> MtrrLibSetMemoryType() hang.
> 
> Steven Shi found this bug in QEMU when using Q35 chip.
> 
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Ruiyu Ni <ruiyu.ni@intel.com>
> Cc: Steven Shi <steven.shi@intel.com>
> Cc: Laszlo Ersek <lersek@redhat.com>
> ---
>  UefiCpuPkg/Library/MtrrLib/MtrrLib.c | 9 ++++++---
>  1 file changed, 6 insertions(+), 3 deletions(-)
> 
> diff --git a/UefiCpuPkg/Library/MtrrLib/MtrrLib.c b/UefiCpuPkg/Library/MtrrLib/MtrrLib.c
> index 2fd1d0153e..cb22558103 100644
> --- a/UefiCpuPkg/Library/MtrrLib/MtrrLib.c
> +++ b/UefiCpuPkg/Library/MtrrLib/MtrrLib.c
> @@ -2776,6 +2776,7 @@ MtrrDebugPrintAllMtrrsWorker (
>      UINTN             RangeCount;
>      UINT64            MtrrValidBitsMask;
>      UINT64            MtrrValidAddressMask;
> +    UINT32            VariableMtrrCount;
>      MTRR_MEMORY_RANGE Ranges[
>        ARRAY_SIZE (mMtrrLibFixedMtrrTable) * sizeof (UINT64) + 2 * ARRAY_SIZE (Mtrrs->Variables.Mtrr) + 1
>        ];
> @@ -2785,6 +2786,8 @@ MtrrDebugPrintAllMtrrsWorker (
>        return;
>      }
>  
> +    VariableMtrrCount = GetVariableMtrrCountWorker ();
> +
>      if (MtrrSetting != NULL) {
>        Mtrrs = MtrrSetting;
>      } else {
> @@ -2802,7 +2805,7 @@ MtrrDebugPrintAllMtrrsWorker (
>        DEBUG((DEBUG_CACHE, "Fixed MTRR[%02d]   : %016lx\n", Index, Mtrrs->Fixed.Mtrr[Index]));
>      }
>  
> -    for (Index = 0; Index < ARRAY_SIZE (Mtrrs->Variables.Mtrr); Index++) {
> +    for (Index = 0; Index < VariableMtrrCount; Index++) {
>        if (((MSR_IA32_MTRR_PHYSMASK_REGISTER *)&Mtrrs->Variables.Mtrr[Index].Mask)->Bits.V == 0) {
>          //
>          // If mask is not valid, then do not display range
> @@ -2829,11 +2832,11 @@ MtrrDebugPrintAllMtrrsWorker (
>      RangeCount = 1;
>  
>      MtrrLibGetRawVariableRanges (
> -      &Mtrrs->Variables, ARRAY_SIZE (Mtrrs->Variables.Mtrr),
> +      &Mtrrs->Variables, VariableMtrrCount,
>        MtrrValidBitsMask, MtrrValidAddressMask, RawVariableRanges
>        );
>      MtrrLibApplyVariableMtrrs (
> -      RawVariableRanges, ARRAY_SIZE (RawVariableRanges),
> +      RawVariableRanges, VariableMtrrCount,
>        Ranges, ARRAY_SIZE (Ranges), &RangeCount
>        );
>  
> 

Assuming this patch is not committed yet:

Reviewed-by: Laszlo Ersek <lersek@redhat.com>


I have another (independent) comment:

This function is currently named "MtrrDebugPrintAllMtrrsWorker", and its
leading comment says "Worker function prints all MTRRs for debugging."

Because of this name and the documentation, I didn't understand
initially how the problem could cause a hang, given that none of the
printing loops would actually access anything out-of-bounds. Some of the
information printed would be garbage, but it should not cause a hang.

That's when I noticed that the function actually *applies* MTRR
settings, by calling MtrrLibApplyVariableMtrrs(). Even worse, the
MtrrLibApplyVariableMtrrs() and MtrrLibApplyFixedMtrrs() function calls
are wrapped by DEBUG_CODE(). This means that in a DEBUG/NOOPT build, the
function will apply MTRR settings, and in a RELEASE build, it won't.

I think this is wrong and should be fixed. A debug function (esp. one
that behaves differently in DEBUG/NOOPT vs. RELEASE) should have no side
effects.

The current situation is similar to:

  ASSERT (FunctionWithSideEffects () == EXPECTED_RETURN_VALUE);

which we all know is wrong.

Thanks
Laszlo
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [PATCH] UefiCpuPkg/MtrrLib: Fix MtrrDebugPrintAllMtrrsWorker to avoid hang

Posted by Ni, Ruiyu 6 years, 6 months ago

Laszlo,
I think the function name confused you.
MtrrLibApplyVariableMtrrs() is not to apply the MTRR setting to CPU/HW.
It's to apply the setting read from CPU/HW to the range array stored in memory.
It doesn't have side effect.

The basic idea of MtrrDebugPrintAllMtrrsWorker() is to read the setting from HW/CPU,
Convert that setting to range array. Then dump the range array.

-----Original Message-----
From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Laszlo Ersek
Sent: Tuesday, October 17, 2017 3:56 PM
To: Ni, Ruiyu <ruiyu.ni@intel.com>; edk2-devel@lists.01.org
Subject: Re: [edk2] [PATCH] UefiCpuPkg/MtrrLib: Fix MtrrDebugPrintAllMtrrsWorker to avoid hang

On 10/17/17 03:46, Ruiyu Ni wrote:
> ARRAY_SIZE(Mtrrs->Variables.Mtrr) was used in
> MtrrDebugPrintAllMtrrsWorker() to parse the MTRR registers.
> Instead, the actual variable MTRR count should be used.
> Otherwise, the uninitialized random data in MtrrSetting may cause
> MtrrLibSetMemoryType() hang.
> 
> Steven Shi found this bug in QEMU when using Q35 chip.
> 
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Ruiyu Ni <ruiyu.ni@intel.com>
> Cc: Steven Shi <steven.shi@intel.com>
> Cc: Laszlo Ersek <lersek@redhat.com>
> ---
>  UefiCpuPkg/Library/MtrrLib/MtrrLib.c | 9 ++++++---
>  1 file changed, 6 insertions(+), 3 deletions(-)
> 
> diff --git a/UefiCpuPkg/Library/MtrrLib/MtrrLib.c 
> b/UefiCpuPkg/Library/MtrrLib/MtrrLib.c
> index 2fd1d0153e..cb22558103 100644
> --- a/UefiCpuPkg/Library/MtrrLib/MtrrLib.c
> +++ b/UefiCpuPkg/Library/MtrrLib/MtrrLib.c
> @@ -2776,6 +2776,7 @@ MtrrDebugPrintAllMtrrsWorker (
>      UINTN             RangeCount;
>      UINT64            MtrrValidBitsMask;
>      UINT64            MtrrValidAddressMask;
> +    UINT32            VariableMtrrCount;
>      MTRR_MEMORY_RANGE Ranges[
>        ARRAY_SIZE (mMtrrLibFixedMtrrTable) * sizeof (UINT64) + 2 * ARRAY_SIZE (Mtrrs->Variables.Mtrr) + 1
>        ];
> @@ -2785,6 +2786,8 @@ MtrrDebugPrintAllMtrrsWorker (
>        return;
>      }
>  
> +    VariableMtrrCount = GetVariableMtrrCountWorker ();
> +
>      if (MtrrSetting != NULL) {
>        Mtrrs = MtrrSetting;
>      } else {
> @@ -2802,7 +2805,7 @@ MtrrDebugPrintAllMtrrsWorker (
>        DEBUG((DEBUG_CACHE, "Fixed MTRR[%02d]   : %016lx\n", Index, Mtrrs->Fixed.Mtrr[Index]));
>      }
>  
> -    for (Index = 0; Index < ARRAY_SIZE (Mtrrs->Variables.Mtrr); Index++) {
> +    for (Index = 0; Index < VariableMtrrCount; Index++) {
>        if (((MSR_IA32_MTRR_PHYSMASK_REGISTER *)&Mtrrs->Variables.Mtrr[Index].Mask)->Bits.V == 0) {
>          //
>          // If mask is not valid, then do not display range @@ 
> -2829,11 +2832,11 @@ MtrrDebugPrintAllMtrrsWorker (
>      RangeCount = 1;
>  
>      MtrrLibGetRawVariableRanges (
> -      &Mtrrs->Variables, ARRAY_SIZE (Mtrrs->Variables.Mtrr),
> +      &Mtrrs->Variables, VariableMtrrCount,
>        MtrrValidBitsMask, MtrrValidAddressMask, RawVariableRanges
>        );
>      MtrrLibApplyVariableMtrrs (
> -      RawVariableRanges, ARRAY_SIZE (RawVariableRanges),
> +      RawVariableRanges, VariableMtrrCount,
>        Ranges, ARRAY_SIZE (Ranges), &RangeCount
>        );
>  
> 

Assuming this patch is not committed yet:

Reviewed-by: Laszlo Ersek <lersek@redhat.com>


I have another (independent) comment:

This function is currently named "MtrrDebugPrintAllMtrrsWorker", and its leading comment says "Worker function prints all MTRRs for debugging."

Because of this name and the documentation, I didn't understand initially how the problem could cause a hang, given that none of the printing loops would actually access anything out-of-bounds. Some of the information printed would be garbage, but it should not cause a hang.

That's when I noticed that the function actually *applies* MTRR settings, by calling MtrrLibApplyVariableMtrrs(). Even worse, the
MtrrLibApplyVariableMtrrs() and MtrrLibApplyFixedMtrrs() function calls are wrapped by DEBUG_CODE(). This means that in a DEBUG/NOOPT build, the function will apply MTRR settings, and in a RELEASE build, it won't.

I think this is wrong and should be fixed. A debug function (esp. one that behaves differently in DEBUG/NOOPT vs. RELEASE) should have no side effects.

The current situation is similar to:

  ASSERT (FunctionWithSideEffects () == EXPECTED_RETURN_VALUE);

which we all know is wrong.

Thanks
Laszlo
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel