Re: [edk2] [PATCH v2] MdeModulePkg/ScsiDiskDxe: Fix undefined behavior in signed left shift
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
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Hao Wu
> ---
> 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.
> +Copyright (c) 2006 - 2017, Intel Corporation. All rights reserved.
> 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
Laszlo
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Re: [edk2] [PATCH v2] MdeModulePkg/ScsiDiskDxe: Fix undefined behavior in signed left shift
Reviewed-by: Feng Tian
Thanks
Feng
-Original Message-
From: Wu, Hao A
Sent: Tuesday, April 11, 2017 9:30 AM
To: edk2-devel@lists.01.org; ler...@redhat.com; guoh...@huawei.com
Cc: Wu, Hao A ; Tian, Feng
Subject: [PATCH v2] MdeModulePkg/ScsiDiskDxe: Fix undefined behavior in signed
left shift
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
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Hao Wu
---
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.
+Copyright (c) 2006 - 2017, Intel Corporation. All rights reserved.
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
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
[edk2] [PATCH v2] MdeModulePkg/ScsiDiskDxe: Fix undefined behavior in signed left shift
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
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Hao Wu
---
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.
+Copyright (c) 2006 - 2017, Intel Corporation. All rights reserved.
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
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
