Re: [PATCH v2] MdeModulePkg/ScsiDiskDxe: Fix undefined behavior in signed left shift

[Laszlo Ersek <lersek@redhat.com>]

On 04/11/17 03:29, 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 undefined behavior in signed left shift operations.
> 
> 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,
> 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 Capacity10->LastLba3 is 1, (Capacity10->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 ScsiDiskDevice->BlkIo.Media->LastBlock to 1 during the cast from type
> int to type UINT64.
> 
> 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;
> 

Thank you very much for the update.

Reviewed-by: Laszlo Ersek <lersek@redhat.com>

Laszlo