[PATCH v2 0/6] Resolve undefined behaviours in left shift OPs

[PATCH v2 0/6] Resolve undefined behaviours in left shift OPs

[Hao Wu]

According to the feebacks from Paolo, the following changes are made in
this version of patch series:
  a. Refine the code logic in Tpl.c to void left shifting the negative
     value. Also makes the code more readable;
  b. Remove the '~' operator before 'Time.Hour' in DxeNetLib.c, since it
     seems like an implementation choice for generating the seed;
  c. Use '1U' instead of '(UINT32)1' in Crc32.c.


V1 history:
The series resolves two kinds of undefined behaviours in left shift
operations:
  a. Left-shifting negative values;
  b. Left-shifting that incurs the result being out of range.

Cc: Steven Shi <steven.shi@intel.com>
Cc: Michael Kinney <michael.d.kinney@intel.com>
Cc: Liming Gao <liming.gao@intel.com>
Cc: Star Zeng <star.zeng@intel.com>
Cc: Eric Dong <eric.dong@intel.com>
Cc: Fu Siyuan <siyuan.fu@intel.com>
Cc: Ye Ting <ting.ye@intel.com>
Cc: Wu Jiaxin <jiaxin.wu@intel.com>
Cc: Qin Long <qin.long@intel.com>
Cc: Paolo Bonzini <pbonzini@redhat.com> 

Hao Wu (6):
  MdePkg/PrintLib: Fix possible negative value left shift
  MdeModulePkg/PrintLib: Fix possible negative value left shift
  MdeModulePkg/Tpl: Fix negative value left shift
  MdeModulePkg/DxeNetLib: Fix negative value left shift
  MdeModulePkg/Crc32: Fix possible out of range left shift
  MdeModulePkg/AtaAtapiPassThru: Fix possible out of range left shift

 MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AhciMode.c          |  4 ++--
 MdeModulePkg/Core/Dxe/Event/Tpl.c                         | 12 +++++++++---
 MdeModulePkg/Core/RuntimeDxe/Crc32.c                      |  6 +++---
 MdeModulePkg/Library/DxeNetLib/DxeNetLib.c                |  2 +-
 MdeModulePkg/Library/DxePrintLibPrint2Protocol/PrintLib.c |  2 +-
 MdePkg/Library/BasePrintLib/PrintLibInternal.c            |  2 +-
 6 files changed, 17 insertions(+), 11 deletions(-)

-- 
2.12.0.windows.1

[PATCH v2 1/6] MdePkg/PrintLib: Fix possible negative value left shift

[Hao Wu]

REF: https://bugzilla.tianocore.org/show_bug.cgi?id=702

Within function InternalPrintLibSPrintMarker(), possible left shift of a
negative value is found in:
"(*(ArgumentString + 1) << 8)"

which involves undefined behavior.

Since '*(ArgumentString + 1)' is of type CONST CHAR8 (signed), it will be
promoted to type int (signed) during the left shift operation. If
'*(ArgumentString + 1)' is a negative value, the behavior will be
undefined.

According to the C11 spec, Section 6.5.7:
> 4 The result of E1 << E2 is E1 left-shifted E2 bit positions; vacated
>   bits are filled with zeros. If E1 has an unsigned type, the value
>   of the result is E1 * 2^E2 , reduced modulo one more than the
>   maximum value representable in the result type. If E1 has a signed
>   type and nonnegative value, and E1 * 2^E2 is representable in the
>   result type, then that is the resulting value; otherwise, the
>   behavior is undefined.

This commit explicitly cast '*(ArgumentString + 1)' with UINT8 to resolve
this issue.

Cc: Steven Shi <steven.shi@intel.com>
Cc: Michael Kinney <michael.d.kinney@intel.com>
Cc: Liming Gao <liming.gao@intel.com>
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Hao Wu <hao.a.wu@intel.com>
---
 MdePkg/Library/BasePrintLib/PrintLibInternal.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/MdePkg/Library/BasePrintLib/PrintLibInternal.c b/MdePkg/Library/BasePrintLib/PrintLibInternal.c
index cec5b3bc99..28d946472f 100644
--- a/MdePkg/Library/BasePrintLib/PrintLibInternal.c
+++ b/MdePkg/Library/BasePrintLib/PrintLibInternal.c
@@ -1165,7 +1165,7 @@ BasePrintLibSPrintMarker (
     // Copy the string into the output buffer performing the required type conversions
     //
     while (Index < Count) {
-      ArgumentCharacter = ((*ArgumentString & 0xff) | (*(ArgumentString + 1) << 8)) & ArgumentMask;
+      ArgumentCharacter = ((*ArgumentString & 0xff) | (((UINT8)*(ArgumentString + 1)) << 8)) & ArgumentMask;
 
       LengthToReturn += (1 * BytesPerOutputCharacter);
       if ((Flags & COUNT_ONLY_NO_PRINT) == 0 && Buffer != NULL) {
-- 
2.12.0.windows.1

Re: [PATCH v2 1/6] MdePkg/PrintLib: Fix possible negative value left shift

[Gao, Liming]

Reviewed-by: Liming Gao <liming.gao@intel.com>

>-----Original Message-----
>From: Wu, Hao A
>Sent: Thursday, September 21, 2017 2:46 PM
>To: edk2-devel@lists.01.org
>Cc: Wu, Hao A <hao.a.wu@intel.com>; Shi, Steven <steven.shi@intel.com>;
>Kinney, Michael D <michael.d.kinney@intel.com>; Gao, Liming
><liming.gao@intel.com>
>Subject: [PATCH v2 1/6] MdePkg/PrintLib: Fix possible negative value left shift
>
>REF: https://bugzilla.tianocore.org/show_bug.cgi?id=702
>
>Within function InternalPrintLibSPrintMarker(), possible left shift of a
>negative value is found in:
>"(*(ArgumentString + 1) << 8)"
>
>which involves undefined behavior.
>
>Since '*(ArgumentString + 1)' is of type CONST CHAR8 (signed), it will be
>promoted to type int (signed) during the left shift operation. If
>'*(ArgumentString + 1)' is a negative value, the behavior will be
>undefined.
>
>According to the C11 spec, Section 6.5.7:
>> 4 The result of E1 << E2 is E1 left-shifted E2 bit positions; vacated
>>   bits are filled with zeros. If E1 has an unsigned type, the value
>>   of the result is E1 * 2^E2 , reduced modulo one more than the
>>   maximum value representable in the result type. If E1 has a signed
>>   type and nonnegative value, and E1 * 2^E2 is representable in the
>>   result type, then that is the resulting value; otherwise, the
>>   behavior is undefined.
>
>This commit explicitly cast '*(ArgumentString + 1)' with UINT8 to resolve
>this issue.
>
>Cc: Steven Shi <steven.shi@intel.com>
>Cc: Michael Kinney <michael.d.kinney@intel.com>
>Cc: Liming Gao <liming.gao@intel.com>
>Contributed-under: TianoCore Contribution Agreement 1.1
>Signed-off-by: Hao Wu <hao.a.wu@intel.com>
>---
> MdePkg/Library/BasePrintLib/PrintLibInternal.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
>diff --git a/MdePkg/Library/BasePrintLib/PrintLibInternal.c
>b/MdePkg/Library/BasePrintLib/PrintLibInternal.c
>index cec5b3bc99..28d946472f 100644
>--- a/MdePkg/Library/BasePrintLib/PrintLibInternal.c
>+++ b/MdePkg/Library/BasePrintLib/PrintLibInternal.c
>@@ -1165,7 +1165,7 @@ BasePrintLibSPrintMarker (
>     // Copy the string into the output buffer performing the required type
>conversions
>     //
>     while (Index < Count) {
>-      ArgumentCharacter = ((*ArgumentString & 0xff) | (*(ArgumentString + 1)
><< 8)) & ArgumentMask;
>+      ArgumentCharacter = ((*ArgumentString & 0xff) |
>(((UINT8)*(ArgumentString + 1)) << 8)) & ArgumentMask;
>
>       LengthToReturn += (1 * BytesPerOutputCharacter);
>       if ((Flags & COUNT_ONLY_NO_PRINT) == 0 && Buffer != NULL) {
>--
>2.12.0.windows.1

[PATCH v2 2/6] MdeModulePkg/PrintLib: Fix possible negative value left shift

[Hao Wu]

REF: https://bugzilla.tianocore.org/show_bug.cgi?id=702

Within function InternalPrintLibSPrintMarker(), possible left shift of a
negative value is found in:
"(*(ArgumentString + 1) << 8)"

which involves undefined behavior.

Since '*(ArgumentString + 1)' is of type CONST CHAR8 (signed), it will be
promoted to type int (signed) during the left shift operation. If
'*(ArgumentString + 1)' is a negative value, the behavior will be
undefined.

According to the C11 spec, Section 6.5.7:
> 4 The result of E1 << E2 is E1 left-shifted E2 bit positions; vacated
>   bits are filled with zeros. If E1 has an unsigned type, the value
>   of the result is E1 * 2^E2 , reduced modulo one more than the
>   maximum value representable in the result type. If E1 has a signed
>   type and nonnegative value, and E1 * 2^E2 is representable in the
>   result type, then that is the resulting value; otherwise, the
>   behavior is undefined.

This commit explicitly cast '*(ArgumentString + 1)' with UINT8 to resolve
this issue.

Cc: Steven Shi <steven.shi@intel.com>
Cc: Michael Kinney <michael.d.kinney@intel.com>
Cc: Liming Gao <liming.gao@intel.com>
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Hao Wu <hao.a.wu@intel.com>
---
 MdeModulePkg/Library/DxePrintLibPrint2Protocol/PrintLib.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/MdeModulePkg/Library/DxePrintLibPrint2Protocol/PrintLib.c b/MdeModulePkg/Library/DxePrintLibPrint2Protocol/PrintLib.c
index b58db8e011..56534e56c3 100644
--- a/MdeModulePkg/Library/DxePrintLibPrint2Protocol/PrintLib.c
+++ b/MdeModulePkg/Library/DxePrintLibPrint2Protocol/PrintLib.c
@@ -2108,7 +2108,7 @@ InternalPrintLibSPrintMarker (
     // Copy the string into the output buffer performing the required type conversions
     //
     while (Index < Count) {
-      ArgumentCharacter = ((*ArgumentString & 0xff) | (*(ArgumentString + 1) << 8)) & ArgumentMask;
+      ArgumentCharacter = ((*ArgumentString & 0xff) | (((UINT8)*(ArgumentString + 1)) << 8)) & ArgumentMask;
 
       LengthToReturn += (1 * BytesPerOutputCharacter);
       if ((Flags & COUNT_ONLY_NO_PRINT) == 0 && Buffer != NULL) {
-- 
2.12.0.windows.1

Re: [PATCH v2 2/6] MdeModulePkg/PrintLib: Fix possible negative value left shift

[Gao, Liming]

Reviewed-by: Liming Gao <liming.gao@intel.com>

>-----Original Message-----
>From: Wu, Hao A
>Sent: Thursday, September 21, 2017 2:46 PM
>To: edk2-devel@lists.01.org
>Cc: Wu, Hao A <hao.a.wu@intel.com>; Shi, Steven <steven.shi@intel.com>;
>Kinney, Michael D <michael.d.kinney@intel.com>; Gao, Liming
><liming.gao@intel.com>
>Subject: [PATCH v2 2/6] MdeModulePkg/PrintLib: Fix possible negative value
>left shift
>
>REF: https://bugzilla.tianocore.org/show_bug.cgi?id=702
>
>Within function InternalPrintLibSPrintMarker(), possible left shift of a
>negative value is found in:
>"(*(ArgumentString + 1) << 8)"
>
>which involves undefined behavior.
>
>Since '*(ArgumentString + 1)' is of type CONST CHAR8 (signed), it will be
>promoted to type int (signed) during the left shift operation. If
>'*(ArgumentString + 1)' is a negative value, the behavior will be
>undefined.
>
>According to the C11 spec, Section 6.5.7:
>> 4 The result of E1 << E2 is E1 left-shifted E2 bit positions; vacated
>>   bits are filled with zeros. If E1 has an unsigned type, the value
>>   of the result is E1 * 2^E2 , reduced modulo one more than the
>>   maximum value representable in the result type. If E1 has a signed
>>   type and nonnegative value, and E1 * 2^E2 is representable in the
>>   result type, then that is the resulting value; otherwise, the
>>   behavior is undefined.
>
>This commit explicitly cast '*(ArgumentString + 1)' with UINT8 to resolve
>this issue.
>
>Cc: Steven Shi <steven.shi@intel.com>
>Cc: Michael Kinney <michael.d.kinney@intel.com>
>Cc: Liming Gao <liming.gao@intel.com>
>Contributed-under: TianoCore Contribution Agreement 1.1
>Signed-off-by: Hao Wu <hao.a.wu@intel.com>
>---
> MdeModulePkg/Library/DxePrintLibPrint2Protocol/PrintLib.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
>diff --git a/MdeModulePkg/Library/DxePrintLibPrint2Protocol/PrintLib.c
>b/MdeModulePkg/Library/DxePrintLibPrint2Protocol/PrintLib.c
>index b58db8e011..56534e56c3 100644
>--- a/MdeModulePkg/Library/DxePrintLibPrint2Protocol/PrintLib.c
>+++ b/MdeModulePkg/Library/DxePrintLibPrint2Protocol/PrintLib.c
>@@ -2108,7 +2108,7 @@ InternalPrintLibSPrintMarker (
>     // Copy the string into the output buffer performing the required type
>conversions
>     //
>     while (Index < Count) {
>-      ArgumentCharacter = ((*ArgumentString & 0xff) | (*(ArgumentString + 1)
><< 8)) & ArgumentMask;
>+      ArgumentCharacter = ((*ArgumentString & 0xff) |
>(((UINT8)*(ArgumentString + 1)) << 8)) & ArgumentMask;
>
>       LengthToReturn += (1 * BytesPerOutputCharacter);
>       if ((Flags & COUNT_ONLY_NO_PRINT) == 0 && Buffer != NULL) {
>--
>2.12.0.windows.1

[PATCH v2 3/6] MdeModulePkg/Tpl: Fix negative value left shift

[Hao Wu]

REF: https://bugzilla.tianocore.org/show_bug.cgi?id=695

Within function CoreRestoreTpl(), left shift a negative value -2 is used
in:
"while (((-2 << NewTpl) & gEventPending) != 0) {"

which involves undefined behavior.

According to the C11 spec, Section 6.5.7:
> 4 The result of E1 << E2 is E1 left-shifted E2 bit positions; vacated
>   bits are filled with zeros. If E1 has an unsigned type, the value
>   of the result is E1 * 2^E2 , reduced modulo one more than the
>   maximum value representable in the result type. If E1 has a signed
>   type and nonnegative value, and E1 * 2^E2 is representable in the
>   result type, then that is the resulting value; otherwise, the
>   behavior is undefined.

This commit refines the code logic to avoid left shifting the negative
value.

Cc: Steven Shi <steven.shi@intel.com>
Cc: Star Zeng <star.zeng@intel.com>
Cc: Eric Dong <eric.dong@intel.com>
Cc: Paolo Bonzini <pbonzini@redhat.com>
Cc: Michael Kinney <michael.d.kinney@intel.com>
Cc: Liming Gao <liming.gao@intel.com>
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Hao Wu <hao.a.wu@intel.com>
---
 MdeModulePkg/Core/Dxe/Event/Tpl.c | 12 +++++++++---
 1 file changed, 9 insertions(+), 3 deletions(-)

diff --git a/MdeModulePkg/Core/Dxe/Event/Tpl.c b/MdeModulePkg/Core/Dxe/Event/Tpl.c
index 8ad0a33701..e3caf832b8 100644
--- a/MdeModulePkg/Core/Dxe/Event/Tpl.c
+++ b/MdeModulePkg/Core/Dxe/Event/Tpl.c
@@ -1,7 +1,7 @@
 /** @file
   Task priority (TPL) functions.
 
-Copyright (c) 2006 - 2015, Intel Corporation. All rights reserved.<BR>
+Copyright (c) 2006 - 2017, Intel Corporation. All rights reserved.<BR>
 This program and the accompanying materials
 are licensed and made available under the terms and conditions of the BSD License
 which accompanies this distribution.  The full text of the license may be found at
@@ -103,6 +103,7 @@ CoreRestoreTpl (
   )
 {
   EFI_TPL     OldTpl;
+  EFI_TPL     PendingTpl;
 
   OldTpl = gEfiCurrentTpl;
   if (NewTpl > OldTpl) {
@@ -123,8 +124,13 @@ CoreRestoreTpl (
   //
   // Dispatch any pending events
   //
-  while (((-2 << NewTpl) & gEventPending) != 0) {
-    gEfiCurrentTpl = (UINTN) HighBitSet64 (gEventPending);
+  while (gEventPending != 0) {
+    PendingTpl = (UINTN) HighBitSet64 (gEventPending);
+    if (PendingTpl <= NewTpl) {
+      break;
+    }
+
+    gEfiCurrentTpl = PendingTpl;
     if (gEfiCurrentTpl < TPL_HIGH_LEVEL) {
       CoreSetInterruptState (TRUE);
     }
-- 
2.12.0.windows.1

Re: [PATCH v2 3/6] MdeModulePkg/Tpl: Fix negative value left shift

[Zeng, Star]

Reviewed-by: Star Zeng <star.zeng@intel.com>

-----Original Message-----
From: Wu, Hao A 
Sent: Thursday, September 21, 2017 2:46 PM
To: edk2-devel@lists.01.org
Cc: Wu, Hao A <hao.a.wu@intel.com>; Shi, Steven <steven.shi@intel.com>; Zeng, Star <star.zeng@intel.com>; Dong, Eric <eric.dong@intel.com>; Paolo Bonzini <pbonzini@redhat.com>; Kinney, Michael D <michael.d.kinney@intel.com>; Gao, Liming <liming.gao@intel.com>
Subject: [PATCH v2 3/6] MdeModulePkg/Tpl: Fix negative value left shift

REF: https://bugzilla.tianocore.org/show_bug.cgi?id=695

Within function CoreRestoreTpl(), left shift a negative value -2 is used
in:
"while (((-2 << NewTpl) & gEventPending) != 0) {"

which involves undefined behavior.

According to the C11 spec, Section 6.5.7:
> 4 The result of E1 << E2 is E1 left-shifted E2 bit positions; vacated
>   bits are filled with zeros. If E1 has an unsigned type, the value
>   of the result is E1 * 2^E2 , reduced modulo one more than the
>   maximum value representable in the result type. If E1 has a signed
>   type and nonnegative value, and E1 * 2^E2 is representable in the
>   result type, then that is the resulting value; otherwise, the
>   behavior is undefined.

This commit refines the code logic to avoid left shifting the negative value.

Cc: Steven Shi <steven.shi@intel.com>
Cc: Star Zeng <star.zeng@intel.com>
Cc: Eric Dong <eric.dong@intel.com>
Cc: Paolo Bonzini <pbonzini@redhat.com>
Cc: Michael Kinney <michael.d.kinney@intel.com>
Cc: Liming Gao <liming.gao@intel.com>
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Hao Wu <hao.a.wu@intel.com>
---
 MdeModulePkg/Core/Dxe/Event/Tpl.c | 12 +++++++++---
 1 file changed, 9 insertions(+), 3 deletions(-)

diff --git a/MdeModulePkg/Core/Dxe/Event/Tpl.c b/MdeModulePkg/Core/Dxe/Event/Tpl.c
index 8ad0a33701..e3caf832b8 100644
--- a/MdeModulePkg/Core/Dxe/Event/Tpl.c
+++ b/MdeModulePkg/Core/Dxe/Event/Tpl.c
@@ -1,7 +1,7 @@
 /** @file
   Task priority (TPL) functions.
 
-Copyright (c) 2006 - 2015, Intel Corporation. All rights reserved.<BR>
+Copyright (c) 2006 - 2017, Intel Corporation. All rights reserved.<BR>
 This program and the accompanying materials  are licensed and made available under the terms and conditions of the BSD License  which accompanies this distribution.  The full text of the license may be found at @@ -103,6 +103,7 @@ CoreRestoreTpl (
   )
 {
   EFI_TPL     OldTpl;
+  EFI_TPL     PendingTpl;
 
   OldTpl = gEfiCurrentTpl;
   if (NewTpl > OldTpl) {
@@ -123,8 +124,13 @@ CoreRestoreTpl (
   //
   // Dispatch any pending events
   //
-  while (((-2 << NewTpl) & gEventPending) != 0) {
-    gEfiCurrentTpl = (UINTN) HighBitSet64 (gEventPending);
+  while (gEventPending != 0) {
+    PendingTpl = (UINTN) HighBitSet64 (gEventPending);
+    if (PendingTpl <= NewTpl) {
+      break;
+    }
+
+    gEfiCurrentTpl = PendingTpl;
     if (gEfiCurrentTpl < TPL_HIGH_LEVEL) {
       CoreSetInterruptState (TRUE);
     }
--
2.12.0.windows.1

[PATCH v2 4/6] MdeModulePkg/DxeNetLib: Fix negative value left shift

[Hao Wu]

REF: https://bugzilla.tianocore.org/show_bug.cgi?id=698

Within function NetRandomInitSeed(), left shift a negative value is used
in:
"~Time.Hour << 24"

which involves undefined behavior.

Since Time.Hour is of type UINT8 (range from 0 to 23), hence ~Time.Hour
will be a negative value (of type int, signed).

According to the C11 spec, Section 6.5.7:
> 4 The result of E1 << E2 is E1 left-shifted E2 bit positions; vacated
>   bits are filled with zeros. If E1 has an unsigned type, the value
>   of the result is E1 * 2^E2 , reduced modulo one more than the
>   maximum value representable in the result type. If E1 has a signed
>   type and nonnegative value, and E1 * 2^E2 is representable in the
>   result type, then that is the resulting value; otherwise, the
>   behavior is undefined.

This commit will remove the '~' operator before 'Time.Hour', since it
seems like an implementation choice for generating the seed.

Cc: Steven Shi <steven.shi@intel.com>
Cc: Fu Siyuan <siyuan.fu@intel.com>
Cc: Ye Ting <ting.ye@intel.com>
Cc: Wu Jiaxin <jiaxin.wu@intel.com>
Cc: Qin Long <qin.long@intel.com>
Cc: Star Zeng <star.zeng@intel.com>
Cc: Eric Dong <eric.dong@intel.com>
Cc: Paolo Bonzini <pbonzini@redhat.com>
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Hao Wu <hao.a.wu@intel.com>
---
 MdeModulePkg/Library/DxeNetLib/DxeNetLib.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/MdeModulePkg/Library/DxeNetLib/DxeNetLib.c b/MdeModulePkg/Library/DxeNetLib/DxeNetLib.c
index 7cd7e3aca0..1ee77fe036 100644
--- a/MdeModulePkg/Library/DxeNetLib/DxeNetLib.c
+++ b/MdeModulePkg/Library/DxeNetLib/DxeNetLib.c
@@ -872,7 +872,7 @@ NetRandomInitSeed (
   UINT64                    MonotonicCount;
 
   gRT->GetTime (&Time, NULL);
-  Seed = (~Time.Hour << 24 | Time.Day << 16 | Time.Minute << 8 | Time.Second);
+  Seed = (Time.Hour << 24 | Time.Day << 16 | Time.Minute << 8 | Time.Second);
   Seed ^= Time.Nanosecond;
   Seed ^= Time.Year << 7;
 
-- 
2.12.0.windows.1

Re: [PATCH v2 4/6] MdeModulePkg/DxeNetLib: Fix negative value left shift

[Zeng, Star]

Leave the review to module experts.

Thanks,
Star
-----Original Message-----
From: Wu, Hao A 
Sent: Thursday, September 21, 2017 2:46 PM
To: edk2-devel@lists.01.org
Cc: Wu, Hao A <hao.a.wu@intel.com>; Shi, Steven <steven.shi@intel.com>; Fu, Siyuan <siyuan.fu@intel.com>; Ye, Ting <ting.ye@intel.com>; Wu, Jiaxin <jiaxin.wu@intel.com>; Long, Qin <qin.long@intel.com>; Zeng, Star <star.zeng@intel.com>; Dong, Eric <eric.dong@intel.com>; Paolo Bonzini <pbonzini@redhat.com>
Subject: [PATCH v2 4/6] MdeModulePkg/DxeNetLib: Fix negative value left shift

REF: https://bugzilla.tianocore.org/show_bug.cgi?id=698

Within function NetRandomInitSeed(), left shift a negative value is used
in:
"~Time.Hour << 24"

which involves undefined behavior.

Since Time.Hour is of type UINT8 (range from 0 to 23), hence ~Time.Hour will be a negative value (of type int, signed).

According to the C11 spec, Section 6.5.7:
> 4 The result of E1 << E2 is E1 left-shifted E2 bit positions; vacated
>   bits are filled with zeros. If E1 has an unsigned type, the value
>   of the result is E1 * 2^E2 , reduced modulo one more than the
>   maximum value representable in the result type. If E1 has a signed
>   type and nonnegative value, and E1 * 2^E2 is representable in the
>   result type, then that is the resulting value; otherwise, the
>   behavior is undefined.

This commit will remove the '~' operator before 'Time.Hour', since it seems like an implementation choice for generating the seed.

Cc: Steven Shi <steven.shi@intel.com>
Cc: Fu Siyuan <siyuan.fu@intel.com>
Cc: Ye Ting <ting.ye@intel.com>
Cc: Wu Jiaxin <jiaxin.wu@intel.com>
Cc: Qin Long <qin.long@intel.com>
Cc: Star Zeng <star.zeng@intel.com>
Cc: Eric Dong <eric.dong@intel.com>
Cc: Paolo Bonzini <pbonzini@redhat.com>
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Hao Wu <hao.a.wu@intel.com>
---
 MdeModulePkg/Library/DxeNetLib/DxeNetLib.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/MdeModulePkg/Library/DxeNetLib/DxeNetLib.c b/MdeModulePkg/Library/DxeNetLib/DxeNetLib.c
index 7cd7e3aca0..1ee77fe036 100644
--- a/MdeModulePkg/Library/DxeNetLib/DxeNetLib.c
+++ b/MdeModulePkg/Library/DxeNetLib/DxeNetLib.c
@@ -872,7 +872,7 @@ NetRandomInitSeed (
   UINT64                    MonotonicCount;
 
   gRT->GetTime (&Time, NULL);
-  Seed = (~Time.Hour << 24 | Time.Day << 16 | Time.Minute << 8 | Time.Second);
+  Seed = (Time.Hour << 24 | Time.Day << 16 | Time.Minute << 8 | 
+ Time.Second);
   Seed ^= Time.Nanosecond;
   Seed ^= Time.Year << 7;
 
--
2.12.0.windows.1

[PATCH v2 5/6] MdeModulePkg/Crc32: Fix possible out of range left shift

[Hao Wu]

REF: https://bugzilla.tianocore.org/show_bug.cgi?id=697

Within function ReverseBits(), left shift operations of 1 in the
following statements:
"(1 << Index)" and "(1 << (31 - Index))"

will incur possible out of range left shift when Index is either 0 or
31, since "1 << 31" is possible to exceed the range of type 'int'
(signed).

According to the C11 spec, Section 6.5.7:
> 4 The result of E1 << E2 is E1 left-shifted E2 bit positions; vacated
>   bits are filled with zeros. If E1 has an unsigned type, the value
>   of the result is E1 * 2^E2 , reduced modulo one more than the
>   maximum value representable in the result type. If E1 has a signed
>   type and nonnegative value, and E1 * 2^E2 is representable in the
>   result type, then that is the resulting value; otherwise, the
>   behavior is undefined.

This commit uses '1U' instead of '1' to resolve this issue.

Cc: Steven Shi <steven.shi@intel.com>
Cc: Star Zeng <star.zeng@intel.com>
Cc: Eric Dong <eric.dong@intel.com>
Cc: Paolo Bonzini <pbonzini@redhat.com>
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Hao Wu <hao.a.wu@intel.com>
---
 MdeModulePkg/Core/RuntimeDxe/Crc32.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/MdeModulePkg/Core/RuntimeDxe/Crc32.c b/MdeModulePkg/Core/RuntimeDxe/Crc32.c
index a6fe77fa34..2cd234b562 100644
--- a/MdeModulePkg/Core/RuntimeDxe/Crc32.c
+++ b/MdeModulePkg/Core/RuntimeDxe/Crc32.c
@@ -7,7 +7,7 @@
   EFI Runtime Services Table are converted from physical address to
   virtual addresses.  This requires that the 32-bit CRC be recomputed.
 
-Copyright (c) 2006 - 2008, Intel Corporation. All rights reserved.<BR>
+Copyright (c) 2006 - 2017, Intel Corporation. All rights reserved.<BR>
 This program and the accompanying materials
 are licensed and made available under the terms and conditions of the BSD License
 which accompanies this distribution.  The full text of the license may be found at
@@ -79,8 +79,8 @@ ReverseBits (
 
   NewValue = 0;
   for (Index = 0; Index < 32; Index++) {
-    if ((Value & (1 << Index)) != 0) {
-      NewValue = NewValue | (1 << (31 - Index));
+    if ((Value & (1U << Index)) != 0) {
+      NewValue = NewValue | (1U << (31 - Index));
     }
   }
 
-- 
2.12.0.windows.1

Re: [PATCH v2 5/6] MdeModulePkg/Crc32: Fix possible out of range left shift

[Gao, Liming]

Hao:
  I sent another patch to introduce CacluateCrc32() API in BaseLib. It will update MdeModulePkg Crc32 to depend on BaseLib. And, BaseLib CacluateCrc32() has no such logic. So, I think this patch is not necessary. 

Thanks
Liming
>-----Original Message-----
>From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Hao
>Wu
>Sent: Thursday, September 21, 2017 2:46 PM
>To: edk2-devel@lists.01.org
>Cc: Wu, Hao A <hao.a.wu@intel.com>; Paolo Bonzini <pbonzini@redhat.com>;
>Dong, Eric <eric.dong@intel.com>; Zeng, Star <star.zeng@intel.com>
>Subject: [edk2] [PATCH v2 5/6] MdeModulePkg/Crc32: Fix possible out of
>range left shift
>
>REF: https://bugzilla.tianocore.org/show_bug.cgi?id=697
>
>Within function ReverseBits(), left shift operations of 1 in the
>following statements:
>"(1 << Index)" and "(1 << (31 - Index))"
>
>will incur possible out of range left shift when Index is either 0 or
>31, since "1 << 31" is possible to exceed the range of type 'int'
>(signed).
>
>According to the C11 spec, Section 6.5.7:
>> 4 The result of E1 << E2 is E1 left-shifted E2 bit positions; vacated
>>   bits are filled with zeros. If E1 has an unsigned type, the value
>>   of the result is E1 * 2^E2 , reduced modulo one more than the
>>   maximum value representable in the result type. If E1 has a signed
>>   type and nonnegative value, and E1 * 2^E2 is representable in the
>>   result type, then that is the resulting value; otherwise, the
>>   behavior is undefined.
>
>This commit uses '1U' instead of '1' to resolve this issue.
>
>Cc: Steven Shi <steven.shi@intel.com>
>Cc: Star Zeng <star.zeng@intel.com>
>Cc: Eric Dong <eric.dong@intel.com>
>Cc: Paolo Bonzini <pbonzini@redhat.com>
>Contributed-under: TianoCore Contribution Agreement 1.1
>Signed-off-by: Hao Wu <hao.a.wu@intel.com>
>---
> MdeModulePkg/Core/RuntimeDxe/Crc32.c | 6 +++---
> 1 file changed, 3 insertions(+), 3 deletions(-)
>
>diff --git a/MdeModulePkg/Core/RuntimeDxe/Crc32.c
>b/MdeModulePkg/Core/RuntimeDxe/Crc32.c
>index a6fe77fa34..2cd234b562 100644
>--- a/MdeModulePkg/Core/RuntimeDxe/Crc32.c
>+++ b/MdeModulePkg/Core/RuntimeDxe/Crc32.c
>@@ -7,7 +7,7 @@
>   EFI Runtime Services Table are converted from physical address to
>   virtual addresses.  This requires that the 32-bit CRC be recomputed.
>
>-Copyright (c) 2006 - 2008, Intel Corporation. All rights reserved.<BR>
>+Copyright (c) 2006 - 2017, Intel Corporation. All rights reserved.<BR>
> This program and the accompanying materials
> are licensed and made available under the terms and conditions of the BSD
>License
> which accompanies this distribution.  The full text of the license may be found
>at
>@@ -79,8 +79,8 @@ ReverseBits (
>
>   NewValue = 0;
>   for (Index = 0; Index < 32; Index++) {
>-    if ((Value & (1 << Index)) != 0) {
>-      NewValue = NewValue | (1 << (31 - Index));
>+    if ((Value & (1U << Index)) != 0) {
>+      NewValue = NewValue | (1U << (31 - Index));
>     }
>   }
>
>--
>2.12.0.windows.1
>
>_______________________________________________
>edk2-devel mailing list
>edk2-devel@lists.01.org
>https://lists.01.org/mailman/listinfo/edk2-devel

Re: [PATCH v2 5/6] MdeModulePkg/Crc32: Fix possible out of range left shift

[Wu, Hao A]

Got it. I will send a new series to drop this patch.


Best Regards,
Hao Wu

> -----Original Message-----
> From: Gao, Liming
> Sent: Thursday, September 28, 2017 11:54 AM
> To: Wu, Hao A; edk2-devel@lists.01.org
> Cc: Wu, Hao A; Paolo Bonzini; Dong, Eric; Zeng, Star
> Subject: RE: [edk2] [PATCH v2 5/6] MdeModulePkg/Crc32: Fix possible out of
> range left shift
> 
> Hao:
>   I sent another patch to introduce CacluateCrc32() API in BaseLib. It will update
> MdeModulePkg Crc32 to depend on BaseLib. And, BaseLib CacluateCrc32() has
> no such logic. So, I think this patch is not necessary.
> 
> Thanks
> Liming
> >-----Original Message-----
> >From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Hao
> >Wu
> >Sent: Thursday, September 21, 2017 2:46 PM
> >To: edk2-devel@lists.01.org
> >Cc: Wu, Hao A <hao.a.wu@intel.com>; Paolo Bonzini <pbonzini@redhat.com>;
> >Dong, Eric <eric.dong@intel.com>; Zeng, Star <star.zeng@intel.com>
> >Subject: [edk2] [PATCH v2 5/6] MdeModulePkg/Crc32: Fix possible out of
> >range left shift
> >
> >REF: https://bugzilla.tianocore.org/show_bug.cgi?id=697
> >
> >Within function ReverseBits(), left shift operations of 1 in the
> >following statements:
> >"(1 << Index)" and "(1 << (31 - Index))"
> >
> >will incur possible out of range left shift when Index is either 0 or
> >31, since "1 << 31" is possible to exceed the range of type 'int'
> >(signed).
> >
> >According to the C11 spec, Section 6.5.7:
> >> 4 The result of E1 << E2 is E1 left-shifted E2 bit positions; vacated
> >>   bits are filled with zeros. If E1 has an unsigned type, the value
> >>   of the result is E1 * 2^E2 , reduced modulo one more than the
> >>   maximum value representable in the result type. If E1 has a signed
> >>   type and nonnegative value, and E1 * 2^E2 is representable in the
> >>   result type, then that is the resulting value; otherwise, the
> >>   behavior is undefined.
> >
> >This commit uses '1U' instead of '1' to resolve this issue.
> >
> >Cc: Steven Shi <steven.shi@intel.com>
> >Cc: Star Zeng <star.zeng@intel.com>
> >Cc: Eric Dong <eric.dong@intel.com>
> >Cc: Paolo Bonzini <pbonzini@redhat.com>
> >Contributed-under: TianoCore Contribution Agreement 1.1
> >Signed-off-by: Hao Wu <hao.a.wu@intel.com>
> >---
> > MdeModulePkg/Core/RuntimeDxe/Crc32.c | 6 +++---
> > 1 file changed, 3 insertions(+), 3 deletions(-)
> >
> >diff --git a/MdeModulePkg/Core/RuntimeDxe/Crc32.c
> >b/MdeModulePkg/Core/RuntimeDxe/Crc32.c
> >index a6fe77fa34..2cd234b562 100644
> >--- a/MdeModulePkg/Core/RuntimeDxe/Crc32.c
> >+++ b/MdeModulePkg/Core/RuntimeDxe/Crc32.c
> >@@ -7,7 +7,7 @@
> >   EFI Runtime Services Table are converted from physical address to
> >   virtual addresses.  This requires that the 32-bit CRC be recomputed.
> >
> >-Copyright (c) 2006 - 2008, Intel Corporation. All rights reserved.<BR>
> >+Copyright (c) 2006 - 2017, Intel Corporation. All rights reserved.<BR>
> > This program and the accompanying materials
> > are licensed and made available under the terms and conditions of the BSD
> >License
> > which accompanies this distribution.  The full text of the license may be found
> >at
> >@@ -79,8 +79,8 @@ ReverseBits (
> >
> >   NewValue = 0;
> >   for (Index = 0; Index < 32; Index++) {
> >-    if ((Value & (1 << Index)) != 0) {
> >-      NewValue = NewValue | (1 << (31 - Index));
> >+    if ((Value & (1U << Index)) != 0) {
> >+      NewValue = NewValue | (1U << (31 - Index));
> >     }
> >   }
> >
> >--
> >2.12.0.windows.1
> >
> >_______________________________________________
> >edk2-devel mailing list
> >edk2-devel@lists.01.org
> >https://lists.01.org/mailman/listinfo/edk2-devel

[PATCH v2 6/6] MdeModulePkg/AtaAtapiPassThru: Fix possible out of range left shift

[Hao Wu]

REF: https://bugzilla.tianocore.org/show_bug.cgi?id=699

Within function AhciModeInitialization(), left shift operations of 'BIT0'
in the following statements:
"if ((PortImplementBitMap & (BIT0 << Port)) != 0) {"

will incur possible out of range left shift when Port is 31, since
"1 << 31" is possible to exceed the range of type 'int' (signed).

According to the C11 spec, Section 6.5.7:
> 4 The result of E1 << E2 is E1 left-shifted E2 bit positions; vacated
>   bits are filled with zeros. If E1 has an unsigned type, the value
>   of the result is E1 * 2^E2 , reduced modulo one more than the
>   maximum value representable in the result type. If E1 has a signed
>   type and nonnegative value, and E1 * 2^E2 is representable in the
>   result type, then that is the resulting value; otherwise, the
>   behavior is undefined.

This commit explicitly cast 'BIT0' with UINT32 to resolve this issue.

Cc: Steven Shi <steven.shi@intel.com>
Cc: Star Zeng <star.zeng@intel.com>
Cc: Eric Dong <eric.dong@intel.com>
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Hao Wu <hao.a.wu@intel.com>
---
 MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AhciMode.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AhciMode.c b/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AhciMode.c
index b954de8101..e6de5d65bc 100644
--- a/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AhciMode.c
+++ b/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AhciMode.c
@@ -1,7 +1,7 @@
 /** @file
   The file for AHCI mode of ATA host controller.
 
-  Copyright (c) 2010 - 2016, Intel Corporation. All rights reserved.<BR>
+  Copyright (c) 2010 - 2017, Intel Corporation. All rights reserved.<BR>
   (C) Copyright 2015 Hewlett Packard Enterprise Development LP<BR>
   This program and the accompanying materials
   are licensed and made available under the terms and conditions of the BSD License
@@ -2314,7 +2314,7 @@ AhciModeInitialization (
   }
 
   for (Port = 0; Port < EFI_AHCI_MAX_PORTS; Port ++) {
-    if ((PortImplementBitMap & (BIT0 << Port)) != 0) {
+    if ((PortImplementBitMap & (((UINT32)BIT0) << Port)) != 0) {
       //
       // According to AHCI spec, MaxPortNumber should be equal or greater than the number of implemented ports.
       //
-- 
2.12.0.windows.1

Re: [PATCH v2 6/6] MdeModulePkg/AtaAtapiPassThru: Fix possible out of range left shift

[Zeng, Star]

I prefer to have the code consistent between this patch with [PATCH v2 5/6] MdeModulePkg/Crc32: Fix possible out of range left shift.

Both to use (UINT32) typecast or 1U.

Liming, Do you have any comment?


Thanks,
Star
-----Original Message-----
From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Hao Wu
Sent: Thursday, September 21, 2017 2:46 PM
To: edk2-devel@lists.01.org
Cc: Wu, Hao A <hao.a.wu@intel.com>; Dong, Eric <eric.dong@intel.com>; Zeng, Star <star.zeng@intel.com>
Subject: [edk2] [PATCH v2 6/6] MdeModulePkg/AtaAtapiPassThru: Fix possible out of range left shift

REF: https://bugzilla.tianocore.org/show_bug.cgi?id=699

Within function AhciModeInitialization(), left shift operations of 'BIT0'
in the following statements:
"if ((PortImplementBitMap & (BIT0 << Port)) != 0) {"

will incur possible out of range left shift when Port is 31, since
"1 << 31" is possible to exceed the range of type 'int' (signed).

According to the C11 spec, Section 6.5.7:
> 4 The result of E1 << E2 is E1 left-shifted E2 bit positions; vacated
>   bits are filled with zeros. If E1 has an unsigned type, the value
>   of the result is E1 * 2^E2 , reduced modulo one more than the
>   maximum value representable in the result type. If E1 has a signed
>   type and nonnegative value, and E1 * 2^E2 is representable in the
>   result type, then that is the resulting value; otherwise, the
>   behavior is undefined.

This commit explicitly cast 'BIT0' with UINT32 to resolve this issue.

Cc: Steven Shi <steven.shi@intel.com>
Cc: Star Zeng <star.zeng@intel.com>
Cc: Eric Dong <eric.dong@intel.com>
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Hao Wu <hao.a.wu@intel.com>
---
 MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AhciMode.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AhciMode.c b/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AhciMode.c
index b954de8101..e6de5d65bc 100644
--- a/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AhciMode.c
+++ b/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AhciMode.c
@@ -1,7 +1,7 @@
 /** @file
   The file for AHCI mode of ATA host controller.
 
-  Copyright (c) 2010 - 2016, Intel Corporation. All rights reserved.<BR>
+  Copyright (c) 2010 - 2017, Intel Corporation. All rights 
+ reserved.<BR>
   (C) Copyright 2015 Hewlett Packard Enterprise Development LP<BR>
   This program and the accompanying materials
   are licensed and made available under the terms and conditions of the BSD License @@ -2314,7 +2314,7 @@ AhciModeInitialization (
   }
 
   for (Port = 0; Port < EFI_AHCI_MAX_PORTS; Port ++) {
-    if ((PortImplementBitMap & (BIT0 << Port)) != 0) {
+    if ((PortImplementBitMap & (((UINT32)BIT0) << Port)) != 0) {
       //
       // According to AHCI spec, MaxPortNumber should be equal or greater than the number of implemented ports.
       //
--
2.12.0.windows.1

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

Re: [PATCH v2 6/6] MdeModulePkg/AtaAtapiPassThru: Fix possible out of range left shift

[Gao, Liming]

Star:
 Crc32 change is not required . I just gives my comment on it. So, there is no consistent issue here. We can keep this patch. 

Thanks
Liming
>-----Original Message-----
>From: Zeng, Star
>Sent: Monday, September 25, 2017 2:25 PM
>To: Wu, Hao A <hao.a.wu@intel.com>; edk2-devel@lists.01.org
>Cc: Wu, Hao A <hao.a.wu@intel.com>; Dong, Eric <eric.dong@intel.com>; Gao,
>Liming <liming.gao@intel.com>; Zeng, Star <star.zeng@intel.com>
>Subject: RE: [edk2] [PATCH v2 6/6] MdeModulePkg/AtaAtapiPassThru: Fix
>possible out of range left shift
>
>I prefer to have the code consistent between this patch with [PATCH v2 5/6]
>MdeModulePkg/Crc32: Fix possible out of range left shift.
>
>Both to use (UINT32) typecast or 1U.
>
>Liming, Do you have any comment?
>
>
>Thanks,
>Star
>-----Original Message-----
>From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Hao
>Wu
>Sent: Thursday, September 21, 2017 2:46 PM
>To: edk2-devel@lists.01.org
>Cc: Wu, Hao A <hao.a.wu@intel.com>; Dong, Eric <eric.dong@intel.com>;
>Zeng, Star <star.zeng@intel.com>
>Subject: [edk2] [PATCH v2 6/6] MdeModulePkg/AtaAtapiPassThru: Fix
>possible out of range left shift
>
>REF: https://bugzilla.tianocore.org/show_bug.cgi?id=699
>
>Within function AhciModeInitialization(), left shift operations of 'BIT0'
>in the following statements:
>"if ((PortImplementBitMap & (BIT0 << Port)) != 0) {"
>
>will incur possible out of range left shift when Port is 31, since
>"1 << 31" is possible to exceed the range of type 'int' (signed).
>
>According to the C11 spec, Section 6.5.7:
>> 4 The result of E1 << E2 is E1 left-shifted E2 bit positions; vacated
>>   bits are filled with zeros. If E1 has an unsigned type, the value
>>   of the result is E1 * 2^E2 , reduced modulo one more than the
>>   maximum value representable in the result type. If E1 has a signed
>>   type and nonnegative value, and E1 * 2^E2 is representable in the
>>   result type, then that is the resulting value; otherwise, the
>>   behavior is undefined.
>
>This commit explicitly cast 'BIT0' with UINT32 to resolve this issue.
>
>Cc: Steven Shi <steven.shi@intel.com>
>Cc: Star Zeng <star.zeng@intel.com>
>Cc: Eric Dong <eric.dong@intel.com>
>Contributed-under: TianoCore Contribution Agreement 1.1
>Signed-off-by: Hao Wu <hao.a.wu@intel.com>
>---
> MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AhciMode.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
>diff --git a/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AhciMode.c
>b/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AhciMode.c
>index b954de8101..e6de5d65bc 100644
>--- a/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AhciMode.c
>+++ b/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AhciMode.c
>@@ -1,7 +1,7 @@
> /** @file
>   The file for AHCI mode of ATA host controller.
>
>-  Copyright (c) 2010 - 2016, Intel Corporation. All rights reserved.<BR>
>+  Copyright (c) 2010 - 2017, Intel Corporation. All rights
>+ reserved.<BR>
>   (C) Copyright 2015 Hewlett Packard Enterprise Development LP<BR>
>   This program and the accompanying materials
>   are licensed and made available under the terms and conditions of the BSD
>License @@ -2314,7 +2314,7 @@ AhciModeInitialization (
>   }
>
>   for (Port = 0; Port < EFI_AHCI_MAX_PORTS; Port ++) {
>-    if ((PortImplementBitMap & (BIT0 << Port)) != 0) {
>+    if ((PortImplementBitMap & (((UINT32)BIT0) << Port)) != 0) {
>       //
>       // According to AHCI spec, MaxPortNumber should be equal or greater
>than the number of implemented ports.
>       //
>--
>2.12.0.windows.1
>
>_______________________________________________
>edk2-devel mailing list
>edk2-devel@lists.01.org
>https://lists.01.org/mailman/listinfo/edk2-devel

Re: [PATCH v2 6/6] MdeModulePkg/AtaAtapiPassThru: Fix possible out of range left shift

[Zeng, Star]

Got it, thanks for the information. :)

Star
-----Original Message-----
From: Gao, Liming 
Sent: Thursday, September 28, 2017 11:57 AM
To: Zeng, Star <star.zeng@intel.com>; Wu, Hao A <hao.a.wu@intel.com>; edk2-devel@lists.01.org
Cc: Wu, Hao A <hao.a.wu@intel.com>; Dong, Eric <eric.dong@intel.com>
Subject: RE: [edk2] [PATCH v2 6/6] MdeModulePkg/AtaAtapiPassThru: Fix possible out of range left shift

Star:
 Crc32 change is not required . I just gives my comment on it. So, there is no consistent issue here. We can keep this patch. 

Thanks
Liming
>-----Original Message-----
>From: Zeng, Star
>Sent: Monday, September 25, 2017 2:25 PM
>To: Wu, Hao A <hao.a.wu@intel.com>; edk2-devel@lists.01.org
>Cc: Wu, Hao A <hao.a.wu@intel.com>; Dong, Eric <eric.dong@intel.com>; 
>Gao, Liming <liming.gao@intel.com>; Zeng, Star <star.zeng@intel.com>
>Subject: RE: [edk2] [PATCH v2 6/6] MdeModulePkg/AtaAtapiPassThru: Fix 
>possible out of range left shift
>
>I prefer to have the code consistent between this patch with [PATCH v2 
>5/6]
>MdeModulePkg/Crc32: Fix possible out of range left shift.
>
>Both to use (UINT32) typecast or 1U.
>
>Liming, Do you have any comment?
>
>
>Thanks,
>Star
>-----Original Message-----
>From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of 
>Hao Wu
>Sent: Thursday, September 21, 2017 2:46 PM
>To: edk2-devel@lists.01.org
>Cc: Wu, Hao A <hao.a.wu@intel.com>; Dong, Eric <eric.dong@intel.com>; 
>Zeng, Star <star.zeng@intel.com>
>Subject: [edk2] [PATCH v2 6/6] MdeModulePkg/AtaAtapiPassThru: Fix 
>possible out of range left shift
>
>REF: https://bugzilla.tianocore.org/show_bug.cgi?id=699
>
>Within function AhciModeInitialization(), left shift operations of 'BIT0'
>in the following statements:
>"if ((PortImplementBitMap & (BIT0 << Port)) != 0) {"
>
>will incur possible out of range left shift when Port is 31, since
>"1 << 31" is possible to exceed the range of type 'int' (signed).
>
>According to the C11 spec, Section 6.5.7:
>> 4 The result of E1 << E2 is E1 left-shifted E2 bit positions; vacated
>>   bits are filled with zeros. If E1 has an unsigned type, the value
>>   of the result is E1 * 2^E2 , reduced modulo one more than the
>>   maximum value representable in the result type. If E1 has a signed
>>   type and nonnegative value, and E1 * 2^E2 is representable in the
>>   result type, then that is the resulting value; otherwise, the
>>   behavior is undefined.
>
>This commit explicitly cast 'BIT0' with UINT32 to resolve this issue.
>
>Cc: Steven Shi <steven.shi@intel.com>
>Cc: Star Zeng <star.zeng@intel.com>
>Cc: Eric Dong <eric.dong@intel.com>
>Contributed-under: TianoCore Contribution Agreement 1.1
>Signed-off-by: Hao Wu <hao.a.wu@intel.com>
>---
> MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AhciMode.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
>diff --git a/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AhciMode.c
>b/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AhciMode.c
>index b954de8101..e6de5d65bc 100644
>--- a/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AhciMode.c
>+++ b/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AhciMode.c
>@@ -1,7 +1,7 @@
> /** @file
>   The file for AHCI mode of ATA host controller.
>
>-  Copyright (c) 2010 - 2016, Intel Corporation. All rights 
>reserved.<BR>
>+  Copyright (c) 2010 - 2017, Intel Corporation. All rights 
>+ reserved.<BR>
>   (C) Copyright 2015 Hewlett Packard Enterprise Development LP<BR>
>   This program and the accompanying materials
>   are licensed and made available under the terms and conditions of 
>the BSD License @@ -2314,7 +2314,7 @@ AhciModeInitialization (
>   }
>
>   for (Port = 0; Port < EFI_AHCI_MAX_PORTS; Port ++) {
>-    if ((PortImplementBitMap & (BIT0 << Port)) != 0) {
>+    if ((PortImplementBitMap & (((UINT32)BIT0) << Port)) != 0) {
>       //
>       // According to AHCI spec, MaxPortNumber should be equal or 
>greater than the number of implemented ports.
>       //
>--
>2.12.0.windows.1
>
>_______________________________________________
>edk2-devel mailing list
>edk2-devel@lists.01.org
>https://lists.01.org/mailman/listinfo/edk2-devel