MdeModulePkg/IdeBusPei: Fix undefined behavior in signed left shift

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.

Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Hao Wu <hao.a.wu@intel.com>
Reviewed-by: Feng Tian <feng.tian@intel.com>

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