> -----Original Message----- > From: Laszlo Ersek [mailto:ler...@redhat.com] > Sent: Monday, April 10, 2017 11:47 PM > To: Wu, Hao A; edk2-devel@lists.01.org > Cc: Tian, Feng; guoh...@huawei.com > Subject: Re: [edk2] [PATCH] MdeModulePkg/ScsiDiskDxe: Fix potential implicit > sign extension > > (Gary, is your Linaro email still alive?) > > On 04/10/17 09:43, Hao Wu wrote: > > In function GetMediaInfo(), the following expression: > > ScsiDiskDevice->BlkIo.Media->LastBlock = (Capacity10->LastLba3 << 24) | > > (Capacity10->LastLba2 << 16) | > > (Capacity10->LastLba1 << 8) | > > Capacity10->LastLba0; > > will involve implicit sign extension. > > > > Since Capacity10->LastLbaX is of type UINT8, and > > ScsiDiskDevice->BlkIo.Media->LastBlock is of type UINT64. Therefore, > > Capacity10->LastLbaX will be promoted to int (32 bits, signed) first, > > perform the bit shift and or. Then the result will be sign-extended to > > type UINT64. > > > > If bit 7 of Capacity10->LastLba3 is 1, then the high 32 bits of > > ScsiDiskDevice->BlkIo.Media->LastBlock will all be set to 1. > > > > This commit will add an explicit UINT32 type cast for > > Capacity10->LastLba3 to resolve this issue. > > > > Cc: Feng Tian <feng.t...@intel.com> > > Contributed-under: TianoCore Contribution Agreement 1.0 > > Signed-off-by: Hao Wu <hao.a...@intel.com> > > --- > > MdeModulePkg/Bus/Scsi/ScsiDiskDxe/ScsiDisk.c | 4 ++-- > > 1 file changed, 2 insertions(+), 2 deletions(-) > > > > diff --git a/MdeModulePkg/Bus/Scsi/ScsiDiskDxe/ScsiDisk.c > b/MdeModulePkg/Bus/Scsi/ScsiDiskDxe/ScsiDisk.c > > index b5eff25b9b..2289f20152 100644 > > --- a/MdeModulePkg/Bus/Scsi/ScsiDiskDxe/ScsiDisk.c > > +++ b/MdeModulePkg/Bus/Scsi/ScsiDiskDxe/ScsiDisk.c > > @@ -1,7 +1,7 @@ > > /** @file > > SCSI disk driver that layers on every SCSI IO protocol in the system. > > > > -Copyright (c) 2006 - 2016, 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 > > @@ -2754,7 +2754,7 @@ GetMediaInfo ( > > UINT8 *Ptr; > > > > if (!ScsiDiskDevice->Cdb16Byte) { > > - ScsiDiskDevice->BlkIo.Media->LastBlock = (Capacity10->LastLba3 << 24) > > | > > + ScsiDiskDevice->BlkIo.Media->LastBlock = ((UINT32) Capacity10- > >LastLba3 << 24) | > > (Capacity10->LastLba2 << 16) > > | > > (Capacity10->LastLba1 << 8) > > | > > Capacity10->LastLba0; > > > > The patch looks okay to me, but I would like to comment on the commit > message. The commit message says "sign extension", which is an assembly > language / processor level concept; the C language doesn't know about > "sign extension". > > Instead, what we have here is undefined behavior. > > 6.5.7 Bitwise shift operators > > 3 The integer promotions are performed on each of the operands. The > type of the result is that of the promoted left operand. If the > value of the right operand is negative or is greater than or equal > to the width of the promoted left operand, the behavior is > undefined. > > 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, we have a left operand here that is promoted to "int". It means that > "E1" has signed type. Its value is nonnegative. However, E1 × 2^E2 is > not representable in the result type, hence the behavior is undefined. > > The fact that this undefined shift *happens* to result in the sign bit > being set (resulting in a negative "int" value), which in turn is > converted to a 64-bit unsigned value as proscribed by the usual > arithmetic conversions (*), is just an implementation detail. The root > cause is that the signed left shift invokes undefined behavior. > > ( > > (*) > > 6.3.1.8 Usual arithmetic conversions > > 1 [...] Otherwise, if the operand that has unsigned integer type has > rank greater or equal to the rank of the type of the other operand, > then the operand with signed integer type is converted to the type > of the operand with unsigned integer type. > > 6.3.1.3 Signed and unsigned integers > > 2 Otherwise, if the new type is unsigned, the value is converted by > repeatedly adding or subtracting one more than the maximum value > that can be represented in the new type until the value is in the > range of the new type. > > ) > > So I suggest to reformulate the subject line and the commit message to > say something like: > > MdeModulePkg/ScsiDiskDxe: fix undefined behavior in signed left shift >
Thanks Laszlo, I agree that the undefined behavior for the left shift operation is the cause here. I will refine the commit title and message in the V2 series. Best Regards, Hao Wu > Thanks > Laszlo _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
