* [PATCH 1/2] UefiCpuPkg/MpInitLib: Fix ASSERT for success. [not found] <cover.1540719996.git.Marvin.Haeuser@outlook.com> @ 2018-10-28 8:51 ` Marvin Häuser 2018-10-28 8:51 ` [PATCH 2/2] UefiCpuPkg/PiSmmCpuDxeSmm: " Marvin Häuser 1 sibling, 0 replies; 5+ messages in thread From: Marvin Häuser @ 2018-10-28 8:51 UTC (permalink / raw) To: edk2-devel@lists.01.org; +Cc: eric.dong@intel.com, lersek@redhat.com Index is initialized to MAX_UINT16 as default failure value, which is what the ASSERT is supposed to test for. The ASSERT condition however can never return FALSE for INT16 != int, as due to Integer Promotion[1], Index is converted to int, which can never result in -1. Furthermore, Index is used as a for loop index variable inbetween its initialization and the ASSERT, so the value is unconditionally overwritten too. Fix the ASSERT check to compare Index to its upper boundary, which it will be equal to if the loop was not broken out of on success. [1] ISO/IEC 9899:2011, 6.5.9.4 Contributed-under: TianoCore Contribution Agreement 1.1 Signed-off-by: Marvin Haeuser <Marvin.Haeuser@outlook.com> --- UefiCpuPkg/Library/MpInitLib/DxeMpLib.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c b/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c index c17daa0fc0da..b2307cbb618d 100644 --- a/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c +++ b/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c @@ -236,7 +236,6 @@ GetProtectedModeCS ( UINTN GdtEntryCount; UINT16 Index; - Index = (UINT16) -1; AsmReadGdtr (&GdtrDesc); GdtEntryCount = (GdtrDesc.Limit + 1) / sizeof (IA32_SEGMENT_DESCRIPTOR); GdtEntry = (IA32_SEGMENT_DESCRIPTOR *) GdtrDesc.Base; @@ -248,7 +247,7 @@ GetProtectedModeCS ( } GdtEntry++; } - ASSERT (Index != -1); + ASSERT (Index != GdtEntryCount); return Index * 8; } -- 2.19.1.windows.1 ^ permalink raw reply related [flat|nested] 5+ messages in thread
* [PATCH 2/2] UefiCpuPkg/PiSmmCpuDxeSmm: Fix ASSERT for success. [not found] <cover.1540719996.git.Marvin.Haeuser@outlook.com> 2018-10-28 8:51 ` [PATCH 1/2] UefiCpuPkg/MpInitLib: Fix ASSERT for success Marvin Häuser @ 2018-10-28 8:51 ` Marvin Häuser 2018-10-30 2:30 ` Dong, Eric 1 sibling, 1 reply; 5+ messages in thread From: Marvin Häuser @ 2018-10-28 8:51 UTC (permalink / raw) To: edk2-devel@lists.01.org; +Cc: eric.dong@intel.com, lersek@redhat.com Index is initialized to MAX_UINT16 as default failure value, which is what the ASSERT is supposed to test for. The ASSERT condition however can never return FALSE for INT16 != int, as due to Integer Promotion[1], Index is converted to int, which can never result in -1. Furthermore, Index is used as a for loop index variable inbetween its initialization and the ASSERT, so the value is unconditionally overwritten too. Fix the ASSERT check to compare Index to its upper boundary, which it will be equal to if the loop was not broken out of on success. [1] ISO/IEC 9899:2011, 6.5.9.4 Contributed-under: TianoCore Contribution Agreement 1.1 Signed-off-by: Marvin Haeuser <Marvin.Haeuser@outlook.com> --- UefiCpuPkg/PiSmmCpuDxeSmm/X64/SmmFuncsArch.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/X64/SmmFuncsArch.c b/UefiCpuPkg/PiSmmCpuDxeSmm/X64/SmmFuncsArch.c index b7c3ad31e82c..89b3f2b7257f 100644 --- a/UefiCpuPkg/PiSmmCpuDxeSmm/X64/SmmFuncsArch.c +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/X64/SmmFuncsArch.c @@ -110,7 +110,6 @@ GetProtectedModeCS ( UINTN GdtEntryCount; UINT16 Index; - Index = (UINT16) -1; AsmReadGdtr (&GdtrDesc); GdtEntryCount = (GdtrDesc.Limit + 1) / sizeof (IA32_SEGMENT_DESCRIPTOR); GdtEntry = (IA32_SEGMENT_DESCRIPTOR *) GdtrDesc.Base; @@ -122,7 +121,7 @@ GetProtectedModeCS ( } GdtEntry++; } - ASSERT (Index != -1); + ASSERT (Index != GdtEntryCount); return Index * 8; } -- 2.19.1.windows.1 ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH 2/2] UefiCpuPkg/PiSmmCpuDxeSmm: Fix ASSERT for success. 2018-10-28 8:51 ` [PATCH 2/2] UefiCpuPkg/PiSmmCpuDxeSmm: " Marvin Häuser @ 2018-10-30 2:30 ` Dong, Eric 2018-10-31 19:55 ` Marvin Häuser 0 siblings, 1 reply; 5+ messages in thread From: Dong, Eric @ 2018-10-30 2:30 UTC (permalink / raw) To: Marvin Häuser, edk2-devel@lists.01.org; +Cc: lersek@redhat.com Hi Marvin, Thanks for your contribution. I have reviewed them and pushed to trunk. SHA numbers are: SHA-1: 4222e8e7e421e9c8d2c2f319a3860dd3332d6255 SHA-1: 37fba7c2762e114a280e3b361b53ded034aac7e3 One more question which just curious by me, how you find this issue? by tool or code review? Thanks, Eric > -----Original Message----- > From: Marvin Häuser [mailto:Marvin.Haeuser@outlook.com] > Sent: Sunday, October 28, 2018 4:51 PM > To: edk2-devel@lists.01.org > Cc: Dong, Eric <eric.dong@intel.com>; lersek@redhat.com > Subject: [PATCH 2/2] UefiCpuPkg/PiSmmCpuDxeSmm: Fix ASSERT for > success. > > Index is initialized to MAX_UINT16 as default failure value, which is what the > ASSERT is supposed to test for. The ASSERT condition however can never > return FALSE for INT16 != int, as due to Integer Promotion[1], Index is > converted to int, which can never result in -1. > > Furthermore, Index is used as a for loop index variable inbetween its > initialization and the ASSERT, so the value is unconditionally overwritten too. > > Fix the ASSERT check to compare Index to its upper boundary, which it will be > equal to if the loop was not broken out of on success. > > [1] ISO/IEC 9899:2011, 6.5.9.4 > > Contributed-under: TianoCore Contribution Agreement 1.1 > Signed-off-by: Marvin Haeuser <Marvin.Haeuser@outlook.com> > --- > UefiCpuPkg/PiSmmCpuDxeSmm/X64/SmmFuncsArch.c | 3 +-- > 1 file changed, 1 insertion(+), 2 deletions(-) > > diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/X64/SmmFuncsArch.c > b/UefiCpuPkg/PiSmmCpuDxeSmm/X64/SmmFuncsArch.c > index b7c3ad31e82c..89b3f2b7257f 100644 > --- a/UefiCpuPkg/PiSmmCpuDxeSmm/X64/SmmFuncsArch.c > +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/X64/SmmFuncsArch.c > @@ -110,7 +110,6 @@ GetProtectedModeCS ( > UINTN GdtEntryCount; > UINT16 Index; > > - Index = (UINT16) -1; > AsmReadGdtr (&GdtrDesc); > GdtEntryCount = (GdtrDesc.Limit + 1) / sizeof > (IA32_SEGMENT_DESCRIPTOR); > GdtEntry = (IA32_SEGMENT_DESCRIPTOR *) GdtrDesc.Base; @@ -122,7 > +121,7 @@ GetProtectedModeCS ( > } > GdtEntry++; > } > - ASSERT (Index != -1); > + ASSERT (Index != GdtEntryCount); > return Index * 8; > } > > -- > 2.19.1.windows.1 ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH 2/2] UefiCpuPkg/PiSmmCpuDxeSmm: Fix ASSERT for success. 2018-10-30 2:30 ` Dong, Eric @ 2018-10-31 19:55 ` Marvin Häuser 2018-11-05 16:50 ` Laszlo Ersek 0 siblings, 1 reply; 5+ messages in thread From: Marvin Häuser @ 2018-10-31 19:55 UTC (permalink / raw) To: Dong, Eric, edk2-devel@lists.01.org; +Cc: lersek@redhat.com Hey Eric, I discovered it by accident, no tool was involved. Regards, Marvin > -----Original Message----- > From: Dong, Eric <eric.dong@intel.com> > Sent: Tuesday, October 30, 2018 3:30 AM > To: Marvin Häuser <Marvin.Haeuser@outlook.com>; edk2- > devel@lists.01.org > Cc: lersek@redhat.com > Subject: RE: [PATCH 2/2] UefiCpuPkg/PiSmmCpuDxeSmm: Fix ASSERT for > success. > > Hi Marvin, > > Thanks for your contribution. I have reviewed them and pushed to trunk. > SHA numbers are: > SHA-1: 4222e8e7e421e9c8d2c2f319a3860dd3332d6255 > SHA-1: 37fba7c2762e114a280e3b361b53ded034aac7e3 > > One more question which just curious by me, how you find this issue? by tool > or code review? > > Thanks, > Eric > > > -----Original Message----- > > From: Marvin Häuser [mailto:Marvin.Haeuser@outlook.com] > > Sent: Sunday, October 28, 2018 4:51 PM > > To: edk2-devel@lists.01.org > > Cc: Dong, Eric <eric.dong@intel.com>; lersek@redhat.com > > Subject: [PATCH 2/2] UefiCpuPkg/PiSmmCpuDxeSmm: Fix ASSERT for > > success. > > > > Index is initialized to MAX_UINT16 as default failure value, which is > > what the ASSERT is supposed to test for. The ASSERT condition however > > can never return FALSE for INT16 != int, as due to Integer > > Promotion[1], Index is converted to int, which can never result in -1. > > > > Furthermore, Index is used as a for loop index variable inbetween its > > initialization and the ASSERT, so the value is unconditionally overwritten > too. > > > > Fix the ASSERT check to compare Index to its upper boundary, which it > > will be equal to if the loop was not broken out of on success. > > > > [1] ISO/IEC 9899:2011, 6.5.9.4 > > > > Contributed-under: TianoCore Contribution Agreement 1.1 > > Signed-off-by: Marvin Haeuser <Marvin.Haeuser@outlook.com> > > --- > > UefiCpuPkg/PiSmmCpuDxeSmm/X64/SmmFuncsArch.c | 3 +-- > > 1 file changed, 1 insertion(+), 2 deletions(-) > > > > diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/X64/SmmFuncsArch.c > > b/UefiCpuPkg/PiSmmCpuDxeSmm/X64/SmmFuncsArch.c > > index b7c3ad31e82c..89b3f2b7257f 100644 > > --- a/UefiCpuPkg/PiSmmCpuDxeSmm/X64/SmmFuncsArch.c > > +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/X64/SmmFuncsArch.c > > @@ -110,7 +110,6 @@ GetProtectedModeCS ( > > UINTN GdtEntryCount; > > UINT16 Index; > > > > - Index = (UINT16) -1; > > AsmReadGdtr (&GdtrDesc); > > GdtEntryCount = (GdtrDesc.Limit + 1) / sizeof > > (IA32_SEGMENT_DESCRIPTOR); > > GdtEntry = (IA32_SEGMENT_DESCRIPTOR *) GdtrDesc.Base; @@ -122,7 > > +121,7 @@ GetProtectedModeCS ( > > } > > GdtEntry++; > > } > > - ASSERT (Index != -1); > > + ASSERT (Index != GdtEntryCount); > > return Index * 8; > > } > > > > -- > > 2.19.1.windows.1 ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH 2/2] UefiCpuPkg/PiSmmCpuDxeSmm: Fix ASSERT for success. 2018-10-31 19:55 ` Marvin Häuser @ 2018-11-05 16:50 ` Laszlo Ersek 0 siblings, 0 replies; 5+ messages in thread From: Laszlo Ersek @ 2018-11-05 16:50 UTC (permalink / raw) To: Marvin Häuser, Dong, Eric, edk2-devel@lists.01.org On 10/31/18 20:55, Marvin Häuser wrote: > Hey Eric, > > I discovered it by accident, no tool was involved. [...] Please always post a 0/nnn cover letter, and manually CC the sum of all individual patch CC's on it. Otherwise the series quite falls apart in a threaded view. Thanks Laszlo ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2018-11-05 16:50 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <cover.1540719996.git.Marvin.Haeuser@outlook.com>
2018-10-28 8:51 ` [PATCH 1/2] UefiCpuPkg/MpInitLib: Fix ASSERT for success Marvin Häuser
2018-10-28 8:51 ` [PATCH 2/2] UefiCpuPkg/PiSmmCpuDxeSmm: " Marvin Häuser
2018-10-30 2:30 ` Dong, Eric
2018-10-31 19:55 ` Marvin Häuser
2018-11-05 16:50 ` Laszlo Ersek
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox