From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail02.groups.io (mail02.groups.io [66.175.222.108]) by spool.mail.gandi.net (Postfix) with ESMTPS id 7F1B094193C for ; Tue, 7 Nov 2023 17:31:19 +0000 (UTC) DKIM-Signature: a=rsa-sha256; bh=YNKm60whaNvVLjrakwX02J2+Pm6ePWaD48+tMuu7Odk=; c=relaxed/simple; d=groups.io; h=Message-ID:Date:MIME-Version:Subject:To:Cc:References:From:In-Reply-To:Precedence:List-Subscribe:List-Help:Sender:List-Id:Mailing-List:Delivered-To:Reply-To:List-Unsubscribe-Post:List-Unsubscribe:Content-Language:Content-Type:Content-Transfer-Encoding; s=20140610; t=1699378278; v=1; b=hVBoMMSN58I9R3BaFwxVaQpafWIITJQ4ivtLlWuckxXuDDFZF1j0kSLS8rIjnA+QRLak+B73 ITfbHLeRYkswVSAyzCH6xdKvUkyRQzxakt7Vrtbd5/s88RpLam8RR3x6I39YKl5wgGtVua0jzHh 0hjI5x3HzQni6VtJXjqVRKjY= X-Received: by 127.0.0.2 with SMTP id tbcxYY7687511xUOQC6aH38n; Tue, 07 Nov 2023 09:31:18 -0800 X-Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.133.124]) by mx.groups.io with SMTP id smtpd.web11.949.1699378277378645340 for ; Tue, 07 Nov 2023 09:31:17 -0800 X-Received: from mimecast-mx02.redhat.com (mx-ext.redhat.com [66.187.233.73]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id us-mta-632-IQn2rfqpOK2k19h6oNxoRw-1; Tue, 07 Nov 2023 12:31:14 -0500 X-MC-Unique: IQn2rfqpOK2k19h6oNxoRw-1 X-Received: from smtp.corp.redhat.com (int-mx04.intmail.prod.int.rdu2.redhat.com [10.11.54.4]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by mimecast-mx02.redhat.com (Postfix) with ESMTPS id E377E3C1AF49; Tue, 7 Nov 2023 17:31:13 +0000 (UTC) X-Received: from [10.39.193.64] (unknown [10.39.193.64]) by smtp.corp.redhat.com (Postfix) with ESMTPS id 36C932026D68; Tue, 7 Nov 2023 17:31:13 +0000 (UTC) Message-ID: <3b477659-e53c-da9d-53fe-2982575c07f5@redhat.com> Date: Tue, 7 Nov 2023 18:31:12 +0100 MIME-Version: 1.0 Subject: Re: [edk2-devel] [PATCH v2 1/2] FatPkg/EnhancedFatDxe: Fix SIGN_EXTENSION Coverity issues To: devel@edk2.groups.io, rsingh@ventanamicro.com Cc: Ray Ni , Veeresh Sangolli References: <20231107062842.116670-1-rsingh@ventanamicro.com> <20231107062842.116670-2-rsingh@ventanamicro.com> From: "Laszlo Ersek" In-Reply-To: <20231107062842.116670-2-rsingh@ventanamicro.com> X-Scanned-By: MIMEDefang 3.4.1 on 10.11.54.4 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Precedence: Bulk List-Subscribe: List-Help: Sender: devel@edk2.groups.io List-Id: Mailing-List: list devel@edk2.groups.io; contact devel+owner@edk2.groups.io Reply-To: devel@edk2.groups.io,lersek@redhat.com List-Unsubscribe-Post: List-Unsubscribe=One-Click List-Unsubscribe: X-Gm-Message-State: lUUq3sMCiWBwVkrGKPA0qtisx7686176AA= Content-Language: en-US Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable X-GND-Status: LEGIT Authentication-Results: spool.mail.gandi.net; dkim=pass header.d=groups.io header.s=20140610 header.b=hVBoMMSN; dmarc=fail reason="SPF not aligned (relaxed), DKIM not aligned (relaxed)" header.from=redhat.com (policy=none); spf=pass (spool.mail.gandi.net: domain of bounce@groups.io designates 66.175.222.108 as permitted sender) smtp.mailfrom=bounce@groups.io On 11/7/23 07:28, Ranbir Singh wrote: > From: Ranbir Singh >=20 > The functions FatGetDirEntInfo and FatOpenDirEnt contains the code > statements >=20 > Cluster =3D (Entry->FileClusterHigh << 16) | Entry->File= Cluster; > and > OFile->FileCluster =3D ((DirEnt->Entry.FileClusterHigh) << 16) | (D= irEnt->Entry.FileCluster); >=20 > respectively. As per Coverity report, in both these statements, there is > an "Operand1" with type "UINT16" (16 bits, unsigned) which is promoted in > "(Operand1 << 16) | Operand2" to type "int" (32 bits, signed), then sign- > extended to type "unsigned long long" (64 bits, unsigned). If the result > of "(Operand1 << 16) | Operand2" is greater than 0x7FFFFFFF, the upper > bits of the result will all be 1. >=20 > So to avoid sign-extension, typecast the Operand1 and then the inter- > -mediate result after << 16 operation with UINTN. Note - UINTN is the > data type of the variable on the LHS of the assignment. >=20 > REF: https://bugzilla.tianocore.org/show_bug.cgi?id=3D4249 >=20 > Cc: Ray Ni > Co-authored-by: Veeresh Sangolli > Signed-off-by: Ranbir Singh > Signed-off-by: Ranbir Singh > --- > FatPkg/EnhancedFatDxe/DirectoryManage.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) >=20 > diff --git a/FatPkg/EnhancedFatDxe/DirectoryManage.c b/FatPkg/EnhancedFat= Dxe/DirectoryManage.c > index 723fc35f38db..a21b7973cd21 100644 > --- a/FatPkg/EnhancedFatDxe/DirectoryManage.c > +++ b/FatPkg/EnhancedFatDxe/DirectoryManage.c > @@ -474,7 +474,7 @@ FatGetDirEntInfo ( > Info =3D Buffer; > Info->Size =3D ResultSize; > if ((Entry->Attributes & FAT_ATTRIBUTE_DIRECTORY) !=3D 0) { > - Cluster =3D (Entry->FileClusterHigh << 16) | Entry->Fil= eCluster; > + Cluster =3D (UINTN)((UINTN)(Entry->FileClusterHigh) << = 16) | Entry->FileCluster; > Info->PhysicalSize =3D FatPhysicalDirSize (Volume, Cluster); > Info->FileSize =3D Info->PhysicalSize; > } else { > @@ -1167,7 +1167,7 @@ FatOpenDirEnt ( > // > Volume =3D Parent->Volume; > OFile->FullPathLen =3D Parent->FullPathLen + 1 + StrLen (DirEnt->F= ileString); > - OFile->FileCluster =3D ((DirEnt->Entry.FileClusterHigh) << 16) | (= DirEnt->Entry.FileCluster); > + OFile->FileCluster =3D (UINTN)((UINTN)(DirEnt->Entry.FileClusterHi= gh) << 16) | (DirEnt->Entry.FileCluster); > InsertTailList (&Parent->ChildHead, &OFile->ChildLink); > } else { > // The gist of the Coverity report is that the FileClusterHigh field has type UINT16 and is promoted to "int", and then shifting that "int" left by 16 bits may cause the result not to fit, and because "int" is signed, this is undefined behavior by C standard. The simplest fix is this (both cases): ((UINT32)FileClusterHigh << 16) | FileCluster I.e., just cast FileClusterHigh to UINT32. This way: - the left-shift is safe, - the FileCluster operand, which is also UINT16 and is also promoted to "int" (INT32), is converted to UINT32, for the bit-or, - the result of the bit-or has type UINT32, which, if UINTN is UINT64, is converted to UINT64 for the sake of the assignment, without change of value. The LHS in both cases is indeed UINTN. Laszlo -=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#110868): https://edk2.groups.io/g/devel/message/110868 Mute This Topic: https://groups.io/mt/102438364/7686176 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/leave/12367111/7686176/19134562= 12/xyzzy [rebecca@openfw.io] -=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-