From 3778a4dfcd8d8286de3eed6fb5e33871854879e5 Mon Sep 17 00:00:00 2001 From: Hao Wu Date: Wed, 12 Apr 2017 08:52:51 +0800 Subject: [PATCH] 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 Reviewed-by: Feng Tian --- 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.39.5