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

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

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