From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga07.intel.com (mga07.intel.com [134.134.136.100]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id 9A4DE2194235A for ; Mon, 10 Apr 2017 18:07:08 -0700 (PDT) Received: from fmsmga002.fm.intel.com ([10.253.24.26]) by orsmga105.jf.intel.com with ESMTP; 10 Apr 2017 18:07:08 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.37,184,1488873600"; d="scan'208";a="1154114439" Received: from fmsmsx108.amr.corp.intel.com ([10.18.124.206]) by fmsmga002.fm.intel.com with ESMTP; 10 Apr 2017 18:07:07 -0700 Received: from fmsmsx119.amr.corp.intel.com (10.18.124.207) by FMSMSX108.amr.corp.intel.com (10.18.124.206) with Microsoft SMTP Server (TLS) id 14.3.319.2; Mon, 10 Apr 2017 18:07:06 -0700 Received: from shsmsx151.ccr.corp.intel.com (10.239.6.50) by FMSMSX119.amr.corp.intel.com (10.18.124.207) with Microsoft SMTP Server (TLS) id 14.3.319.2; Mon, 10 Apr 2017 18:07:05 -0700 Received: from shsmsx104.ccr.corp.intel.com ([169.254.5.178]) by SHSMSX151.ccr.corp.intel.com ([169.254.3.224]) with mapi id 14.03.0319.002; Tue, 11 Apr 2017 09:07:02 +0800 From: "Wu, Hao A" To: Laszlo Ersek , "edk2-devel@lists.01.org" CC: "Tian, Feng" , "guoheyi@huawei.com" Thread-Topic: [edk2] [PATCH] MdeModulePkg/ScsiDiskDxe: Fix potential implicit sign extension Thread-Index: AQHSsc4yKr6oCoque06NrzRZWrZl6qG+OZ6AgAEh6WA= Date: Tue, 11 Apr 2017 01:07:02 +0000 Message-ID: References: <20170410074324.16340-1-hao.a.wu@intel.com> In-Reply-To: Accept-Language: zh-CN, en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-originating-ip: [10.239.127.40] MIME-Version: 1.0 Subject: Re: [PATCH] MdeModulePkg/ScsiDiskDxe: Fix potential implicit sign extension X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.22 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 11 Apr 2017 01:07:08 -0000 Content-Language: en-US Content-Type: text/plain; charset="iso-8859-1" Content-Transfer-Encoding: quoted-printable > -----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 impli= cit > sign extension >=20 > (Gary, is your Linaro email still alive?) >=20 > On 04/10/17 09:43, Hao Wu wrote: > > In function GetMediaInfo(), the following expression: > > ScsiDiskDevice->BlkIo.Media->LastBlock =3D (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 > > 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 =3D (Capacity10->LastLba3 = << 24) | > > + ScsiDiskDevice->BlkIo.Media->LastBlock =3D ((UINT32) Capacity10- > >LastLba3 << 24) | > > (Capacity10->LastLba2 <<= 16) | > > (Capacity10->LastLba1 <<= 8) | > > Capacity10->LastLba0; > > >=20 > 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". >=20 > Instead, what we have here is undefined behavior. >=20 > 6.5.7 Bitwise shift operators >=20 > 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. >=20 > 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 =D7 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 =D7 2^E2 is representable in the > result type, then that is the resulting value; otherwise, the > behavior is undefined. >=20 > 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 =D7 2^E2 is > not representable in the result type, hence the behavior is undefined. >=20 > 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. >=20 > ( >=20 > (*) >=20 > 6.3.1.8 Usual arithmetic conversions >=20 > 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. >=20 > 6.3.1.3 Signed and unsigned integers >=20 > 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. >=20 > ) >=20 > So I suggest to reformulate the subject line and the commit message to > say something like: >=20 > MdeModulePkg/ScsiDiskDxe: fix undefined behavior in signed left shift >=20 Thanks Laszlo, I agree that the undefined behavior for the left shift operation is the cau= se here. I will refine the commit title and message in the V2 series. Best Regards, Hao Wu > Thanks > Laszlo