UefiCpuPkg/Library/MtrrLib/MtrrLib.c | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-)
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
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
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
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
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
© 2016 - 2024 Red Hat, Inc.