From mboxrd@z Thu Jan 1 00:00:00 1970 Subject: [PATCH] FatPkg/EnhancedFatDxe: Fix various Coverity issues To: devel@edk2.groups.io From: "Ranbir Singh" X-Originating-Location: Bengaluru, Karnataka, IN (122.172.85.38) X-Originating-Platform: Windows Chrome 108 User-Agent: GROUPS.IO Web Poster MIME-Version: 1.0 Date: Wed, 04 Jan 2023 06:09:47 -0800 Message-ID: Content-Type: multipart/alternative; boundary="rNKqDLNz1eBL8nQHWb3v" --rNKqDLNz1eBL8nQHWb3v Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: quoted-printable The functions FatGetDirEntInfo and FatOpenDirEnt in the file FatPkg/EnhancedFatDxe/DirectoryManage.c contains the code statements Cluster=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =3D (Entry->FileClusterHig= h << 16) | Entry->FileCluster; and OFile->FileCluster =3D ((DirEnt->Entry.FileClusterHigh) << 16) | (DirEnt->E= ntry.FileCluster); respectively. In both these statements, there is an "Operand1" with type "UINT16" (16 bits, unsigned) which is promoted in the operation "(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 "Operand1" with UINTN which is the data type of the variable on the LHS of the assignment. The function FatInitializeDiskCache in the file FatPkg/EnhancedFatDxe/DiskCache.c evaluates an expression FAT_DATACACHE_GROUP_COUNT << DiskCache[CacheData].PageAlignment and assigns it to DataCacheSize which is of type UINTN; As per Coverity report, FAT_DATACACHE_GROUP_COUNT << DiskCache[CacheData].PageAlignment is a potentially overflowing expression with type "int" (32 bits, signed) evaluated using 32-bit arithmetic, and then used in a context that expects an expression of type "UINTN" (64 bits, unsigned). To avoid overflow, cast "FAT_DATACACHE_GROUP_COUNT" to type "UINTN". REF: https://bugzilla.tianocore.org/show_bug.cgi?id=3D4249 Signed-off-by: Ranbir Singh --- FatPkg/EnhancedFatDxe/DirectoryManage.c | 4 ++-- FatPkg/EnhancedFatDxe/DiskCache.c=C2=A0 =C2=A0 =C2=A0 =C2=A0| 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/FatPkg/EnhancedFatDxe/DirectoryManage.c b/FatPkg/EnhancedFatDx= e/DirectoryManage.c index 723fc35f38..cdae864be0 100644 --- a/FatPkg/EnhancedFatDxe/DirectoryManage.c +++ b/FatPkg/EnhancedFatDxe/DirectoryManage.c @@ -474,7 +474,7 @@ FatGetDirEntInfo ( Info=C2=A0 =C2=A0 =C2=A0 =C2=A0=3D Buffer; Info->Size =3D ResultSize; if ((Entry->Attributes & FAT_ATTRIBUTE_DIRECTORY) !=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)(Entry->FileClusterHigh) << 16) | (UINTN)Entry->FileCluster; Info->PhysicalSize =3D FatPhysicalDirSize (Volume, Cluster); Info->FileSize=C2=A0 =C2=A0 =C2=A0=3D Info->PhysicalSize; } else { @@ -1167,7 +1167,7 @@ FatOpenDirEnt ( // Volume=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0=3D Parent->Volume; OFile->FullPathLen =3D Parent->FullPathLen + 1 + StrLen (DirEnt->FileString= ); -=C2=A0 =C2=A0 =C2=A0 OFile->FileCluster =3D ((DirEnt->Entry.FileClusterHig= h) << 16) | (DirEnt->Entry.FileCluster); +=C2=A0 =C2=A0 =C2=A0 OFile->FileCluster =3D ((UINTN)(DirEnt->Entry.FileClu= sterHigh) << 16) | (DirEnt->Entry.FileCluster); InsertTailList (&Parent->ChildHead, &OFile->ChildLink); } else { // diff --git a/FatPkg/EnhancedFatDxe/DiskCache.c b/FatPkg/EnhancedFatDxe/Disk= Cache.c index d1a34a6a64..d56e338586 100644 --- a/FatPkg/EnhancedFatDxe/DiskCache.c +++ b/FatPkg/EnhancedFatDxe/DiskCache.c @@ -477,7 +477,7 @@ FatInitializeDiskCache ( DiskCache[CacheFat].BaseAddress=C2=A0 =C2=A0=3D Volume->FatPos; DiskCache[CacheFat].LimitAddress=C2=A0 =3D Volume->FatPos + Volume->FatSize= ; FatCacheSize=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 =C2=A0 =3D FatCacheGroupCount << DiskCache[CacheFat].PageAlignment; -=C2=A0 DataCacheSize=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 =C2=A0 =C2=A0=3D FAT_DATACACHE_GROUP_COUNT << DiskCache[CacheDat= a].PageAlignment; +=C2=A0 DataCacheSize=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 =C2=A0 =C2=A0=3D (UINTN)FAT_DATACACHE_GROUP_COUNT << DiskCache[C= acheData].PageAlignment; // // Allocate the Fat Cache buffer // -- 2.36.1.windows.1 --rNKqDLNz1eBL8nQHWb3v Content-Type: text/html; charset="utf-8" Content-Transfer-Encoding: quoted-printable
The functions FatGetDirEntInfo and FatOpenDirEnt in the file
FatPkg/EnhancedFatDxe/DirectoryManage.c contains the code statements
 
      Cluster            = =3D (Entry->FileClusterHigh << 16) | Entry->FileCluster;
      and
      OFile->FileCluster =3D ((DirEnt->Entry.File= ClusterHigh) << 16) | (DirEnt->Entry.FileCluster);
 
respectively. In both these statements, there is an "Operand1" with
type "UINT16" (16 bits, unsigned) which is promoted in the operation
"(Operand1 << 16) | Operand2" to type "int" (32 bits, signed), t= hen
sign-extended to type "unsigned long long" (64 bits, unsigned). If the=
result of "(Operand1 << 16) | Operand2" is greater than 0x7FFFFF= FF,
the upper bits of the result will all be 1. So to avoid sign-extension= ,
typecast "Operand1" with UINTN which is the data type of the variable<= /div>
on the LHS of the assignment.
 
The function FatInitializeDiskCache in the file
FatPkg/EnhancedFatDxe/DiskCache.c evaluates an expression
 
FAT_DATACACHE_GROUP_COUNT << DiskCache[CacheData].PageAlignment<= /div>
and
assigns it to DataCacheSize which is of type UINTN;
 
As per Coverity report,
FAT_DATACACHE_GROUP_COUNT << DiskCache[CacheData].PageAlignment = is a
potentially overflowing expression with type "int" (32 bits, signed)
evaluated using 32-bit arithmetic, and then used in a context that
expects an expression of type "UINTN" (64 bits, unsigned).
To avoid overflow, cast "FAT_DATACACHE_GROUP_COUNT" to type "UINTN".
 
REF: https://bugzilla.tianocore.org/show_bug.cgi?id=3D4249
Signed-off-by: Ranbir Singh <Ranbir.Singh3@Dell.com>
---
 FatPkg/EnhancedFatDxe/DirectoryManage.c | 4 ++--
 FatPkg/EnhancedFatDxe/DiskCache.c       | 2 = +-
 2 files changed, 3 insertions(+), 3 deletions(-)
 
diff --git a/FatPkg/EnhancedFatDxe/DirectoryManage.c b/FatPkg/Enhanced= FatDxe/DirectoryManage.c
index 723fc35f38..cdae864be0 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_DIRE= CTORY) !=3D 0) {
-      Cluster           = =3D (Entry->FileClusterHigh << 16) | Entry->FileCluster;
+      Cluster           = =3D ((UINTN)(Entry->FileClusterHigh) << 16) | (UINTN)Entry->Fi= leCluster;
       Info->PhysicalSize =3D FatPhysicalDirSiz= e (Volume, Cluster);
       Info->FileSize     =3D In= fo->PhysicalSize;
     } else {
@@ -1167,7 +1167,7 @@ FatOpenDirEnt (
       //
       Volume          &n= bsp;  =3D Parent->Volume;
       OFile->FullPathLen =3D Parent->FullPa= thLen + 1 + StrLen (DirEnt->FileString);
-      OFile->FileCluster =3D ((DirEnt->Entry.Fil= eClusterHigh) << 16) | (DirEnt->Entry.FileCluster);
+      OFile->FileCluster =3D ((UINTN)(DirEnt->En= try.FileClusterHigh) << 16) | (DirEnt->Entry.FileCluster);
       InsertTailList (&Parent->ChildHead, = &OFile->ChildLink);
     } else {
       //
diff --git a/FatPkg/EnhancedFatDxe/DiskCache.c b/FatPkg/EnhancedFatDxe= /DiskCache.c
index d1a34a6a64..d56e338586 100644
--- a/FatPkg/EnhancedFatDxe/DiskCache.c
+++ b/FatPkg/EnhancedFatDxe/DiskCache.c
@@ -477,7 +477,7 @@ FatInitializeDiskCache (
   DiskCache[CacheFat].BaseAddress   =3D Volume-&g= t;FatPos;
   DiskCache[CacheFat].LimitAddress  =3D Volume->Fat= Pos + Volume->FatSize;
   FatCacheSize            &nb= sp;         =3D FatCacheGroupCount << DiskCache[C= acheFat].PageAlignment;
-  DataCacheSize              =        =3D FAT_DATACACHE_GROUP_COUNT << DiskCache= [CacheData].PageAlignment;
+  DataCacheSize              =        =3D (UINTN)FAT_DATACACHE_GROUP_COUNT << Di= skCache[CacheData].PageAlignment;
   //
   // Allocate the Fat Cache buffer
   //
--
2.36.1.windows.1
--rNKqDLNz1eBL8nQHWb3v--