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

["Laszlo Ersek" <lersek@redhat.com>]

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