In function ReadCapacity(), the following expression:
MediaInfo->LastBlock = (Data.LastLba3 << 24) |
  (Data.LastLba2 << 16) |
  (Data.LastLba1 << 8) |
  Data.LastLba0;

(There is also a similar case in this function.)

will involve undefined behavior in signed left shift operations.

Since Data.LastLbaX is of type UINT8, and MediaInfo->LastBlock is of type
UINTN. Therefore, Data.LastLbaX will be promoted to int (32 bits, signed)
first, and then perform the left shift operation.

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.

So if bit 7 of Data.LastLba3 is 1, (Data.LastLba3 << 24) will be out of
the range within int type. The undefined behavior of the signed left shift
will lead to a potential of setting the high 32 bits of
MediaInfo->LastBlock to 1 during the cast from type int to type UINT64
for X64 builds.

This commit will add an explicit UINT32 type cast for Data.LastLba3 to
resolve this issue.

Cc: Feng Tian <feng.tian@intel.com>
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Hao Wu <hao.a.wu@intel.com>
---
 MdeModulePkg/Bus/Pci/IdeBusPei/AtapiPeim.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/MdeModulePkg/Bus/Pci/IdeBusPei/AtapiPeim.c b/MdeModulePkg/Bus/Pci/IdeBusPei/AtapiPeim.c
index b1ab34d597..58bef161fb 100644
--- a/MdeModulePkg/Bus/Pci/IdeBusPei/AtapiPeim.c
+++ b/MdeModulePkg/Bus/Pci/IdeBusPei/AtapiPeim.c
@@ -2140,7 +2140,7 @@ ReadCapacity (
 
     if (MediaInfo->DeviceType == IdeCDROM) {
 
-      MediaInfo->LastBlock    = (Data.LastLba3 << 24) | (Data.LastLba2 << 16) | (Data.LastLba1 << 8) | Data.LastLba0;
+      MediaInfo->LastBlock    = ((UINT32) Data.LastLba3 << 24) | (Data.LastLba2 << 16) | (Data.LastLba1 << 8) | Data.LastLba0;
       MediaInfo->MediaPresent = TRUE;
       //
       // Because the user data portion in the sector of the Data CD supported
@@ -2161,7 +2161,7 @@ ReadCapacity (
         MediaInfo2->MediaPresent = FALSE;
         MediaInfo2->LastBlock    = 0;
       } else {
-        MediaInfo->LastBlock = (FormatData.LastLba3 << 24) |
+        MediaInfo->LastBlock = ((UINT32) FormatData.LastLba3 << 24) |
           (FormatData.LastLba2 << 16) |
           (FormatData.LastLba1 << 8) |
           FormatData.LastLba0;
-- 
2.12.0.windows.1

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