From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mx1.redhat.com (mx1.redhat.com [209.132.183.28]) (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 C1F4B21A04817 for ; Mon, 10 Apr 2017 08:46:54 -0700 (PDT) Received: from smtp.corp.redhat.com (int-mx04.intmail.prod.int.phx2.redhat.com [10.5.11.14]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 1C2EA8046E; Mon, 10 Apr 2017 15:46:54 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mx1.redhat.com 1C2EA8046E Authentication-Results: ext-mx04.extmail.prod.ext.phx2.redhat.com; dmarc=none (p=none dis=none) header.from=redhat.com Authentication-Results: ext-mx04.extmail.prod.ext.phx2.redhat.com; spf=pass smtp.mailfrom=lersek@redhat.com DKIM-Filter: OpenDKIM Filter v2.11.0 mx1.redhat.com 1C2EA8046E Received: from lacos-laptop-7.usersys.redhat.com (ovpn-116-49.phx2.redhat.com [10.3.116.49]) by smtp.corp.redhat.com (Postfix) with ESMTP id C583DA0FE1; Mon, 10 Apr 2017 15:46:52 +0000 (UTC) To: Hao Wu , edk2-devel@lists.01.org References: <20170410074324.16340-1-hao.a.wu@intel.com> Cc: Feng Tian , guoheyi@huawei.com From: Laszlo Ersek Message-ID: Date: Mon, 10 Apr 2017 17:46:51 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.8.0 MIME-Version: 1.0 In-Reply-To: <20170410074324.16340-1-hao.a.wu@intel.com> X-Scanned-By: MIMEDefang 2.79 on 10.5.11.14 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.28]); Mon, 10 Apr 2017 15:46:54 +0000 (UTC) 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: Mon, 10 Apr 2017 15:46:55 -0000 Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 8bit (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 > 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; > 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