Re: [edk2-platforms][PATCH v3 01/11] Ext4Pkg: Fix memory leak in Ext4RetrieveDirent

["Pedro Falcato" <pedro.falcato@gmail.com>]

On Fri, Jan 27, 2023 at 9:29 AM Savva Mitrofanov <savvamtr@gmail.com> wrote:
>
> We need to free buffer on return if BlockRemainder != 0. Also changed
> return logic from function to use use common exit to prevent code
> duplication.
>
> Cc: Marvin Häuser <mhaeuser@posteo.de>
> Cc: Pedro Falcato <pedro.falcato@gmail.com>
> Cc: Vitaly Cheptsov <vit9696@protonmail.com>
> Fixes: d9ceedca6c8f ("Ext4Pkg: Add Ext4Dxe driver.")
> Signed-off-by: Savva Mitrofanov <savvamtr@gmail.com>
> ---
>  Features/Ext4Pkg/Ext4Dxe/Directory.c | 30 +++++++++++---------
>  1 file changed, 16 insertions(+), 14 deletions(-)
>
> diff --git a/Features/Ext4Pkg/Ext4Dxe/Directory.c b/Features/Ext4Pkg/Ext4Dxe/Directory.c
> index 456916453952..f80b1aacd459 100644
> --- a/Features/Ext4Pkg/Ext4Dxe/Directory.c
> +++ b/Features/Ext4Pkg/Ext4Dxe/Directory.c
> @@ -113,8 +113,7 @@ Ext4RetrieveDirent (
>    UINTN           ToCopy;
>    UINTN           BlockOffset;
>
> -  Status = EFI_NOT_FOUND;
> -  Buf    = AllocatePool (Partition->BlockSize);
> +  Buf = AllocatePool (Partition->BlockSize);
>
>    if (Buf == NULL) {
>      return EFI_OUT_OF_RESOURCES;
> @@ -128,7 +127,8 @@ Ext4RetrieveDirent (
>    DivU64x32Remainder (DirInoSize, Partition->BlockSize, &BlockRemainder);
>    if (BlockRemainder != 0) {
>      // Directory inodes need to have block aligned sizes
> -    return EFI_VOLUME_CORRUPTED;
> +    Status = EFI_VOLUME_CORRUPTED;
> +    goto Out;
>    }
>
>    while (Off < DirInoSize) {
> @@ -137,8 +137,7 @@ Ext4RetrieveDirent (
>      Status = Ext4Read (Partition, Directory, Buf, Off, &Length);
>
>      if (Status != EFI_SUCCESS) {
> -      FreePool (Buf);
> -      return Status;
> +      goto Out;
>      }
>
>      for (BlockOffset = 0; BlockOffset < Partition->BlockSize; ) {
> @@ -146,19 +145,19 @@ Ext4RetrieveDirent (
>        RemainingBlock = Partition->BlockSize - BlockOffset;
>        // Check if the minimum directory entry fits inside [BlockOffset, EndOfBlock]
>        if (RemainingBlock < EXT4_MIN_DIR_ENTRY_LEN) {
> -        FreePool (Buf);
> -        return EFI_VOLUME_CORRUPTED;
> +        Status = EFI_VOLUME_CORRUPTED;
> +        goto Out;
>        }
>
>        if (!Ext4ValidDirent (Entry)) {
> -        FreePool (Buf);
> -        return EFI_VOLUME_CORRUPTED;
> +        Status = EFI_VOLUME_CORRUPTED;
> +        goto Out;
>        }
>
>        if ((Entry->name_len > RemainingBlock) || (Entry->rec_len > RemainingBlock)) {
>          // Corrupted filesystem
> -        FreePool (Buf);
> -        return EFI_VOLUME_CORRUPTED;
> +        Status = EFI_VOLUME_CORRUPTED;
> +        goto Out;
>        }
>
>        // Unused entry
> @@ -193,8 +192,8 @@ Ext4RetrieveDirent (
>          ToCopy = MIN (Entry->rec_len, sizeof (EXT4_DIR_ENTRY));
>
>          CopyMem (Result, Entry, ToCopy);
> -        FreePool (Buf);
> -        return EFI_SUCCESS;
> +        Status = EFI_SUCCESS;
> +        goto Out;
>        }
>
>        BlockOffset += Entry->rec_len;
> @@ -203,8 +202,11 @@ Ext4RetrieveDirent (
>      Off += Partition->BlockSize;
>    }
>
> +  Status = EFI_NOT_FOUND;
> +
> +Out:
>    FreePool (Buf);
> -  return EFI_NOT_FOUND;
> +  return Status;
>  }
>
>  /**
> --
> 2.39.0
>

Reviewed-by: Pedro Falcato <pedro.falcato@gmail.com>
-- 
Pedro