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

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

[Hao Wu]

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;
-- 
2.12.0.windows.1

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

[Laszlo Ersek]

(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

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

[Wu, Hao A]

> -----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