Re: [PATCH] MdeModulePkg/ScsiDiskDxe: Fix potential implicit sign extension

["Wu, Hao A" <hao.a.wu@intel.com>]

> -----Original Message-----
> From: Laszlo Ersek [mailto:lersek@redhat.com]
> Sent: Monday, April 10, 2017 11:47 PM
> To: Wu, Hao A; edk2-devel@lists.01.org
> Cc: Tian, Feng; guoheyi@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.tian@intel.com>
> > Contributed-under: TianoCore Contribution Agreement 1.0
> > Signed-off-by: Hao Wu <hao.a.wu@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