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

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

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

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