Re: [edk2-devel] [PATCH v2 1/2] FatPkg/EnhancedFatDxe: Fix SIGN_EXTENSION Coverity issues

["Ranbir Singh" <rsingh@ventanamicro.com>]

[-- Attachment #1: Type: text/plain, Size: 4648 bytes --]

Can we consider UINTN to be equal to larger than UINT32?

The essential difference between Laszlo's suggestion

 Cluster            = ((UINT32)FileClusterHigh << 16) | FileCluster

and the patchset

 Cluster            = (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 PM 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
> >
> >       Cluster            = (Entry->FileClusterHigh << 16) |
> Entry->FileCluster;
> >       and
> >       OFile->FileCluster = ((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, 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.
> >
> > 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=4249
> >
> > Cc: Ray Ni <ray.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>
> > ---
> >  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       = Buffer;
> >      Info->Size = ResultSize;
> >      if ((Entry->Attributes & FAT_ATTRIBUTE_DIRECTORY) != 0) {
> > -      Cluster            = (Entry->FileClusterHigh << 16) |
> Entry->FileCluster;
> > +      Cluster            = (UINTN)((UINTN)(Entry->FileClusterHigh) <<
> 16) | Entry->FileCluster;
> >        Info->PhysicalSize = FatPhysicalDirSize (Volume, Cluster);
> >        Info->FileSize     = Info->PhysicalSize;
> >      } else {
> > @@ -1167,7 +1167,7 @@ FatOpenDirEnt (
> >        //
> >        Volume             = Parent->Volume;
> >        OFile->FullPathLen = Parent->FullPathLen + 1 + StrLen
> (DirEnt->FileString);
> > -      OFile->FileCluster = ((DirEnt->Entry.FileClusterHigh) << 16) |
> (DirEnt->Entry.FileCluster);
> > +      OFile->FileCluster =
> (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
>
>


-=-=-=-=-=-=-=-=-=-=-=-
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]
-=-=-=-=-=-=-=-=-=-=-=-



[-- Attachment #2: Type: text/html, Size: 6405 bytes --]