[PATCH 1/2] UefiCpuPkg/MpInitLib: Fix ASSERT for success.

[PATCH 1/2] UefiCpuPkg/MpInitLib: Fix ASSERT for success.

[Marvin Häuser]

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

[PATCH 2/2] UefiCpuPkg/PiSmmCpuDxeSmm: Fix ASSERT for success.

[Marvin Häuser]

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

Re: [PATCH 2/2] UefiCpuPkg/PiSmmCpuDxeSmm: Fix ASSERT for success.

[Dong, Eric]

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

Re: [PATCH 2/2] UefiCpuPkg/PiSmmCpuDxeSmm: Fix ASSERT for success.

[Marvin Häuser]

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

Re: [PATCH 2/2] UefiCpuPkg/PiSmmCpuDxeSmm: Fix ASSERT for success.

[Laszlo Ersek]

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