From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail05.groups.io (mail05.groups.io [45.79.224.7]) by spool.mail.gandi.net (Postfix) with ESMTPS id 3CA0C74003D for ; Mon, 6 Jan 2025 06:55:31 +0000 (UTC) DKIM-Signature: a=rsa-sha256; bh=2StatWyhd3ONGgkVglOoaBUo1D5kaMxDWoPT7j9yMyg=; c=relaxed/simple; d=groups.io; h=MIME-Version:References:In-Reply-To:From:Date:Message-ID:Subject:To:Cc:Precedence:List-Subscribe:List-Help:Sender:List-Id:Mailing-List:Delivered-To:Resent-Date:Resent-From:Reply-To:List-Unsubscribe-Post:List-Unsubscribe:Content-Type; s=20240830; t=1736146531; v=1; x=1736405730; b=otaKZsRNPBt7vCqvt4X0fGIREoqhGSvUkoMfH9r6dLi6c7kVNx5ZS9+wEaiYxe9ui6k6araL 82Z9DSkq+OlndU7/ZalLMg9nM2JPSsA156Sj6t0LTNNx83c3bivUgPWCV7TUp92kBg9T5+70huA n93qRnBT5hQpdnDv7swPfDiBRvp9Rn4phMxWAnv8FWSCH5leQix0QEURu+P1tMgGOecrYY+a9Ow Ilm3OLA4xkKm6vS0B4O1BAZym0yaEYJfQ3pI2WLmUzxYyygOrLaJ2ZgRUUFWPTxwihICNA8UvPD so6+LXZ55V2qdgfJozCIahIyMMv8aecWNLSgRnuSCC4Ww== X-Received: by 127.0.0.2 with SMTP id DveZYY7687511xuGFAbjaQGx; Sun, 05 Jan 2025 22:55:30 -0800 X-Received: from mail-ua1-f54.google.com (mail-ua1-f54.google.com [209.85.222.54]) by mx.groups.io with SMTP id smtpd.web11.54324.1736146524640682257 for ; Sun, 05 Jan 2025 22:55:24 -0800 X-Received: by mail-ua1-f54.google.com with SMTP id a1e0cc1a2514c-85bad9e0214so4976536241.3 for ; Sun, 05 Jan 2025 22:55:24 -0800 (PST) X-Gm-Message-State: y0MxfinK1pcuphc9048RqdYax7686176AA= X-Gm-Gg: ASbGncuiZ8SsxYPXXDlXZOvDLl7/7SY9ywb0uRs7lnFDQJsLe+AmuTLGwPZYTdrc9ww SI5yBHfd+tq4SYSJz12RlEioBxhH6hAfk0Ms2cD1i9IlLhYy8qqmi/WU73pkuWVeQ0/tzoKU= X-Google-Smtp-Source: AGHT+IHlm94v0Wo4FR7q19vsbWEevMLi6T7BsXK/AQZP2Yf8LcR6/91lgjyJa/2drK2p3/sin9VMiLVGYBEJnVybxOQ= X-Received: by 2002:a05:6102:6e88:b0:4b2:cc94:187c with SMTP id ada2fe7eead31-4b2cc941e3fmr35553011137.2.1736146523604; Sun, 05 Jan 2025 22:55:23 -0800 (PST) MIME-Version: 1.0 References: <20231107062842.116670-1-rsingh@ventanamicro.com> <20231107062842.116670-2-rsingh@ventanamicro.com> <3b477659-e53c-da9d-53fe-2982575c07f5@redhat.com> In-Reply-To: <3b477659-e53c-da9d-53fe-2982575c07f5@redhat.com> From: "Ranbir Singh" Date: Mon, 6 Jan 2025 12:25:12 +0530 Message-ID: Subject: Re: [edk2-devel] [PATCH v2 1/2] FatPkg/EnhancedFatDxe: Fix SIGN_EXTENSION Coverity issues To: Laszlo Ersek , Oliver Smith-Denny Cc: devel@edk2.groups.io, Ray Ni , Veeresh Sangolli 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 Resent-Date: Sun, 05 Jan 2025 22:55:24 -0800 Resent-From: rsingh@ventanamicro.com Reply-To: devel@edk2.groups.io,rsingh@ventanamicro.com List-Unsubscribe-Post: List-Unsubscribe=One-Click List-Unsubscribe: Content-Type: multipart/alternative; boundary="000000000000d15c32062b041cd5" X-GND-Status: LEGIT Authentication-Results: spool.mail.gandi.net; dkim=pass header.d=groups.io header.s=20240830 header.b=otaKZsRN; dmarc=none; spf=pass (spool.mail.gandi.net: domain of bounce@groups.io designates 45.79.224.7 as permitted sender) smtp.mailfrom=bounce@groups.io --000000000000d15c32062b041cd5 Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable Can we consider UINTN to be equal to larger than UINT32? The essential difference between Laszlo's suggestion Cluster =3D ((UINT32)FileClusterHigh << 16) | FileCluster and the patchset Cluster =3D (UINTN)((UINTN)FileClusterHigh << 16) | FileCluster is that the latter involves one more outer UINTN typecast. It has now gone out of my memory whether it was purposely included as without outer typecast the Coverity error was still getting reflected. However, my belief is that was the case. Also, the question can be - Is it harmless given that it is of the same type as LHS ? On Tue, Nov 7, 2023 at 11:01=E2=80=AFPM Laszlo Ersek wr= ote: > On 11/7/23 07:28, Ranbir Singh wrote: > > From: Ranbir Singh > > > > The functions FatGetDirEntInfo and FatOpenDirEnt contains the code > > statements > > > > Cluster =3D (Entry->FileClusterHigh << 16) | > Entry->FileCluster; > > and > > OFile->FileCluster =3D ((DirEnt->Entry.FileClusterHigh) << 16) | > (DirEnt->Entry.FileCluster); > > > > respectively. As per Coverity report, in both these statements, there i= s > > an "Operand1" with type "UINT16" (16 bits, unsigned) which is promoted = in > > "(Operand1 << 16) | Operand2" to type "int" (32 bits, signed), then sig= n- > > extended to type "unsigned long long" (64 bits, unsigned). If the resul= t > > of "(Operand1 << 16) | Operand2" is greater than 0x7FFFFFFF, the upper > > bits of the result will all be 1. > > > > 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. > > > > REF: https://bugzilla.tianocore.org/show_bug.cgi?id=3D4249 > > > > 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(-) > > > > diff --git a/FatPkg/EnhancedFatDxe/DirectoryManage.c > b/FatPkg/EnhancedFatDxe/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->FileCluster; > > + 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->FileString); > > - OFile->FileCluster =3D ((DirEnt->Entry.FileClusterHigh) << 16) | > (DirEnt->Entry.FileCluster); > > + OFile->FileCluster =3D > (UINTN)((UINTN)(DirEnt->Entry.FileClusterHigh) << 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 (#120959): https://edk2.groups.io/g/devel/message/120959 Mute This Topic: https://groups.io/mt/102438364/7686176 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io] -=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D- --000000000000d15c32062b041cd5 Content-Type: text/html; charset="UTF-8" Content-Transfer-Encoding: quoted-printable
Can we consider UINTN to be equal to larger than UINT32?
The essential difference between Laszlo's suggestion

=C2= =A0Cluster=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =3D ((UINT32)FileCluste= rHigh << 16) | FileCluster

and the patchset=C2=A0
<= br>
=C2=A0Cluster=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =3D (U= INTN)((UINTN)FileClusterHigh << 16) | FileCluster

is th= at the latter involves one more outer UINTN typecast. It has now gone out o= f my memory whether it was purposely included as without outer typecast the= Coverity error was still getting reflected. However, my=C2=A0belief is tha= t was the case. Also, the question can be - Is it harmless given that it is= of the same type as LHS ?


On Tue, Nov 7= , 2023 at 11:01=E2=80=AFPM Laszlo Ersek <lersek@redhat.com> wrote:
On 11/7/23 07:28, Ranbir Singh wrote:
> From: Ranbir Singh <Ranbir.Singh3@Dell.com>
>
> The functions FatGetDirEntInfo and FatOpenDirEnt contains the code
> statements
>
>=C2=A0 =C2=A0 =C2=A0 =C2=A0Cluster=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 =3D (Entry->FileClusterHigh << 16) | Entry->FileCluster;=
>=C2=A0 =C2=A0 =C2=A0 =C2=A0and
>=C2=A0 =C2=A0 =C2=A0 =C2=A0OFile->FileCluster =3D ((DirEnt->Entry= .FileClusterHigh) << 16) | (DirEnt->Entry.FileCluster);
>
> respectively. As per Coverity report, in both these statements, there = is
> an "Operand1" with type "UINT16" (16 bits, unsigne= d) which is promoted in
> "(Operand1 << 16) | Operand2" to type "int" = (32 bits, signed), then sign-
> extended to type "unsigned long long" (64 bits, unsigned). I= f the result
> of "(Operand1 << 16) | Operand2" is greater than 0x7FF= FFFFF, the upper
> bits of the result will all be 1.
>
> So to avoid sign-extension, typecast the Operand1 and then the inter-<= br> > -mediate result after << 16 operation with UINTN. Note - UINTN i= s the
> data type of the variable on the LHS of the assignment.
>
> REF: https://bugzilla.tianocore.org/show_b= ug.cgi?id=3D4249
>
> Cc: Ray Ni <r= ay.ni@intel.com>
> Co-authored-by: Veeresh Sangolli <veeresh.sangolli@dellteam.com>
> Signed-off-by: Ranbir Singh <Ranbir.Singh3@Dell.com>
> Signed-off-by: Ranbir Singh <rsingh@ventanamicro.com>
> ---
>=C2=A0 FatPkg/EnhancedFatDxe/DirectoryManage.c | 4 ++--
>=C2=A0 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/FatPkg/EnhancedFatDxe/DirectoryManage.c b/FatPkg/Enhanced= FatDxe/DirectoryManage.c
> index 723fc35f38db..a21b7973cd21 100644
> --- a/FatPkg/EnhancedFatDxe/DirectoryManage.c
> +++ b/FatPkg/EnhancedFatDxe/DirectoryManage.c
> @@ -474,7 +474,7 @@ FatGetDirEntInfo (
>=C2=A0 =C2=A0 =C2=A0 Info=C2=A0 =C2=A0 =C2=A0 =C2=A0=3D Buffer;
>=C2=A0 =C2=A0 =C2=A0 Info->Size =3D ResultSize;
>=C2=A0 =C2=A0 =C2=A0 if ((Entry->Attributes & FAT_ATTRIBUTE_DIRE= CTORY) !=3D 0) {
> -=C2=A0 =C2=A0 =C2=A0 Cluster=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0= =3D (Entry->FileClusterHigh << 16) | Entry->FileCluster;
> +=C2=A0 =C2=A0 =C2=A0 Cluster=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0= =3D (UINTN)((UINTN)(Entry->FileClusterHigh) << 16) | Entry->Fi= leCluster;
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 Info->PhysicalSize =3D FatPhysicalDirSiz= e (Volume, Cluster);
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 Info->FileSize=C2=A0 =C2=A0 =C2=A0=3D In= fo->PhysicalSize;
>=C2=A0 =C2=A0 =C2=A0 } else {
> @@ -1167,7 +1167,7 @@ FatOpenDirEnt (
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 //
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 Volume=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 =C2=A0=3D Parent->Volume;
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 OFile->FullPathLen =3D Parent->FullPa= thLen + 1 + StrLen (DirEnt->FileString);
> -=C2=A0 =C2=A0 =C2=A0 OFile->FileCluster =3D ((DirEnt->Entry.Fil= eClusterHigh) << 16) | (DirEnt->Entry.FileCluster);
> +=C2=A0 =C2=A0 =C2=A0 OFile->FileCluster =3D (UINTN)((UINTN)(DirEnt= ->Entry.FileClusterHigh) << 16) | (DirEnt->Entry.FileCluster);<= br> >=C2=A0 =C2=A0 =C2=A0 =C2=A0 InsertTailList (&Parent->ChildHead, = &OFile->ChildLink);
>=C2=A0 =C2=A0 =C2=A0 } else {
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 //

The gist of the Coverity report is that the FileClusterHigh field has
type UINT16 and is promoted to "int", and then shifting that &quo= t;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):

=C2=A0 ((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

_._,_._,_
Groups.io Links:

=20 You receive all messages sent to this group. =20 =20

View/Reply Online (#120959) | =20 | Mute= This Topic | New Topic
Your Subscriptio= n | Contact Group Owner | Unsubscribe [rebecca@openfw.io]

_._,_._,_
--000000000000d15c32062b041cd5--