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

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

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