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 1F42521939234 for ; Tue, 11 Apr 2017 05:01:44 -0700 (PDT) Received: from smtp.corp.redhat.com (int-mx02.intmail.prod.int.phx2.redhat.com [10.5.11.12]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 884A88AE71; Tue, 11 Apr 2017 12:01:43 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mx1.redhat.com 884A88AE71 Authentication-Results: ext-mx03.extmail.prod.ext.phx2.redhat.com; dmarc=none (p=none dis=none) header.from=redhat.com Authentication-Results: ext-mx03.extmail.prod.ext.phx2.redhat.com; spf=pass smtp.mailfrom=lersek@redhat.com DKIM-Filter: OpenDKIM Filter v2.11.0 mx1.redhat.com 884A88AE71 Received: from lacos-laptop-7.usersys.redhat.com (ovpn-116-128.phx2.redhat.com [10.3.116.128]) by smtp.corp.redhat.com (Postfix) with ESMTP id 78D6084950; Tue, 11 Apr 2017 12:01:42 +0000 (UTC) To: Hao Wu , edk2-devel@lists.01.org, guoheyi@huawei.com References: <20170411012945.16788-1-hao.a.wu@intel.com> Cc: Feng Tian From: Laszlo Ersek Message-ID: <37df9db3-8f47-2a47-3af4-8b1975c70556@redhat.com> Date: Tue, 11 Apr 2017 14:00:59 +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: <20170411012945.16788-1-hao.a.wu@intel.com> X-Scanned-By: MIMEDefang 2.79 on 10.5.11.12 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.27]); Tue, 11 Apr 2017 12:01:43 +0000 (UTC) Subject: Re: [PATCH v2] MdeModulePkg/ScsiDiskDxe: Fix undefined behavior in signed left shift 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 12:01:44 -0000 Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit 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