On 11/7/23 07:28, Ranbir Singh wrote:
> From: Ranbir Singh <[email protected]>
>
> 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 <[email protected]>
> Co-authored-by: Veeresh Sangolli <[email protected]>
> Signed-off-by: Ranbir Singh <[email protected]>
> Signed-off-by: Ranbir Singh <[email protected]>
> ---
> 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/21656
Group Owner: [email protected]
Unsubscribe:
https://edk2.groups.io/g/devel/leave/9847357/21656/1706620634/xyzzy
[[email protected]]
-=-=-=-=-=-=-=-=-=-=-=-
