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

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

[Ranbir Singh]

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

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

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

[Ranbir Singh]

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

Comment and code message update is doable with no effect on Patch's
coverity status.

So, this will be included in the final patch series.

On Tue, Nov 7, 2023 at 11:09 PM Laszlo Ersek <lersek@redhat.com> wrote:

> 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 (#120958): https://edk2.groups.io/g/devel/message/120958
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]
-=-=-=-=-=-=-=-=-=-=-=-



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

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

[Oliver Smith-Denny via groups.io]

Hi Ranbir and Veeresh,

On 11/6/2023 10:28 PM, Ranbir Singh wrote:
> 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(-)
> 

I see that this patch was never merged (I don't see any response to
Laszlo's comments, which I agree with) but the GH issue is open:
https://github.com/tianocore/edk2/issues/10318.

If you want to put up a PR to address this issue, taking into account
Laszlo's review, please do so.

Thanks,
Oliver


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

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

[Ranbir Singh]

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

Hi Oliver,

Thanks for reviving the thread.

Fact of the matter is - I do not work in Dell anymore which is when the
original set of patches were generated and also tested that Coverity
flagged warnings/issues/errors indeed vanishes after applying the set of
patches.

While I was there, I did not have the means to post the patches to the EDK2
Mailing list. Just to not let all those efforts go to waste, when it later
became feasible for me, I sent out the patches for review. But now, I do
not have any means to run the code through Coverity scan. Without that I
can't guarantee that any further code change will achieve the aim with
which the original set of patches were sent. If that's acceptable to you, I
will try to put up a PR when possible.

Best Regards,
Ranbir Singh

On Fri, Jan 3, 2025 at 3:08 AM Oliver Smith-Denny <osde@linux.microsoft.com>
wrote:

> Hi Ranbir and Veeresh,
>
> On 11/6/2023 10:28 PM, Ranbir Singh wrote:
> > 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(-)
> >
>
> I see that this patch was never merged (I don't see any response to
> Laszlo's comments, which I agree with) but the GH issue is open:
> https://github.com/tianocore/edk2/issues/10318.
>
> If you want to put up a PR to address this issue, taking into account
> Laszlo's review, please do so.
>
> Thanks,
> Oliver
>


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



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

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

[Oliver Smith-Denny via groups.io]

On 1/5/2025 10:32 PM, Ranbir Singh wrote:
> Hi Oliver,
> 
> Thanks for reviving the thread.
> 
> Fact of the matter is - I do not work in Dell anymore which is when the 
> original set of patches were generated and also tested that Coverity 
> flagged warnings/issues/errors indeed vanishes after applying the set of 
> patches.
> 
> While I was there, I did not have the means to post the patches to the 
> EDK2 Mailing list. Just to not let all those efforts go to waste, when 
> it later became feasible for me, I sent out the patches for review. But 
> now, I do not have any means to run the code through Coverity scan. 
> Without that I can't guarantee that any further code change will achieve 
> the aim with which the original set of patches were sent. If that's 
> acceptable to you, I will try to put up a PR when possible.
> 

As long as the changes make sense, I am happy to take a PR for them :).

Thanks,
Oliver


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