[edk2-devel] [PATCH v2 0/2] BZ 4249: Fix FatPkg/EnhancedFatDxe issues pointed by Coverity

[edk2-devel] [PATCH v2 0/2] BZ 4249: Fix FatPkg/EnhancedFatDxe issues pointed by Coverity

[Ranbir Singh]

v1 -> v2:
  - Rebased to latest master HEAD

Ranbir Singh (2):
  FatPkg/EnhancedFatDxe: Fix SIGN_EXTENSION Coverity issues
  FatPkg/EnhancedFatDxe: Fix OVERFLOW_BEFORE_WIDEN Coverity issue

 FatPkg/EnhancedFatDxe/DirectoryManage.c | 4 ++--
 FatPkg/EnhancedFatDxe/DiskCache.c       | 2 +-
 2 files changed, 3 insertions(+), 3 deletions(-)

-- 
2.34.1



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#110816): https://edk2.groups.io/g/devel/message/110816
Mute This Topic: https://groups.io/mt/102438361/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

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

[Ranbir Singh]

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 {
       //
-- 
2.34.1



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#110817): https://edk2.groups.io/g/devel/message/110817
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]
-=-=-=-=-=-=-=-=-=-=-=-

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

[Laszlo Ersek]

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 (#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/1913456212/xyzzy [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

[edk2-devel] [PATCH v2 2/2] FatPkg/EnhancedFatDxe: Fix OVERFLOW_BEFORE_WIDEN Coverity issue

[Ranbir Singh]

From: Ranbir Singh <Ranbir.Singh3@Dell.com>

The function FatInitializeDiskCache 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=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/DiskCache.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/FatPkg/EnhancedFatDxe/DiskCache.c b/FatPkg/EnhancedFatDxe/DiskCache.c
index d1a34a6a646f..d56e338586d8 100644
--- a/FatPkg/EnhancedFatDxe/DiskCache.c
+++ b/FatPkg/EnhancedFatDxe/DiskCache.c
@@ -477,7 +477,7 @@ FatInitializeDiskCache (
   DiskCache[CacheFat].BaseAddress   = Volume->FatPos;
   DiskCache[CacheFat].LimitAddress  = Volume->FatPos + Volume->FatSize;
   FatCacheSize                      = FatCacheGroupCount << DiskCache[CacheFat].PageAlignment;
-  DataCacheSize                     = FAT_DATACACHE_GROUP_COUNT << DiskCache[CacheData].PageAlignment;
+  DataCacheSize                     = (UINTN)FAT_DATACACHE_GROUP_COUNT << DiskCache[CacheData].PageAlignment;
   //
   // Allocate the Fat Cache buffer
   //
-- 
2.34.1



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#110818): https://edk2.groups.io/g/devel/message/110818
Mute This Topic: https://groups.io/mt/102438365/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH v2 2/2] FatPkg/EnhancedFatDxe: Fix OVERFLOW_BEFORE_WIDEN Coverity issue

[Laszlo Ersek]

On 11/7/23 07:28, Ranbir Singh wrote:
> From: Ranbir Singh <Ranbir.Singh3@Dell.com>
> 
> The function FatInitializeDiskCache 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=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/DiskCache.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/FatPkg/EnhancedFatDxe/DiskCache.c b/FatPkg/EnhancedFatDxe/DiskCache.c
> index d1a34a6a646f..d56e338586d8 100644
> --- a/FatPkg/EnhancedFatDxe/DiskCache.c
> +++ b/FatPkg/EnhancedFatDxe/DiskCache.c
> @@ -477,7 +477,7 @@ FatInitializeDiskCache (
>    DiskCache[CacheFat].BaseAddress   = Volume->FatPos;
>    DiskCache[CacheFat].LimitAddress  = Volume->FatPos + Volume->FatSize;
>    FatCacheSize                      = FatCacheGroupCount << DiskCache[CacheFat].PageAlignment;
> -  DataCacheSize                     = FAT_DATACACHE_GROUP_COUNT << DiskCache[CacheData].PageAlignment;
> +  DataCacheSize                     = (UINTN)FAT_DATACACHE_GROUP_COUNT << DiskCache[CacheData].PageAlignment;
>    //
>    // Allocate the Fat Cache buffer
>    //

I don't understand Coverity here. This is the larger context (extract):

  //
  // Configure the parameters of disk cache
  //
  if (Volume->FatType == Fat12) {
    DiskCache[CacheData].PageAlignment = FAT_DATACACHE_PAGE_MIN_ALIGNMENT;
  } else {
    DiskCache[CacheData].PageAlignment = FAT_DATACACHE_PAGE_MAX_ALIGNMENT;
  }

  DataCacheSize                     = FAT_DATACACHE_GROUP_COUNT << DiskCache[CacheData].PageAlignment;

In practice, one of two things can happen: either

  DataCacheSize = 64 << 13;

or

  DataCacheSize = 64 << 16;

The larger value is 2 MB, it happily fits in an INT32.

I don't mind the patch, but the commit message, and a new code comment as well, should state that we're only casting to shut up Coverity.

If the shift count "DiskCache[CacheData].PageAlignment" were *indeed* unbounded, then we'd have much more serious problems. First, we could shift by 64 bits or more, which is undefined even if we cast to UINT64 at first. Second, even assuming "DataCacheSize" can be calculated correctly, just below it we have

  CacheBuffer = AllocateZeroPool (FatCacheSize + DataCacheSize);

where the *addition* can nicely overflow regardless.

The patch is OK but it requires a comment, and a commit message update.

Thanks
Laszlo



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#110869): https://edk2.groups.io/g/devel/message/110869
Mute This Topic: https://groups.io/mt/102438365/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/leave/12367111/7686176/1913456212/xyzzy [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-