From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mout01.posteo.de (mout01.posteo.de [185.67.36.65]) by mx.groups.io with SMTP id smtpd.web11.102874.1674828982009585294 for ; Fri, 27 Jan 2023 06:16:22 -0800 Authentication-Results: mx.groups.io; dkim=pass header.i=@posteo.de header.s=2017 header.b=LhiS58r/; spf=pass (domain: posteo.de, ip: 185.67.36.65, mailfrom: mhaeuser@posteo.de) Received: from submission (posteo.de [185.67.36.169]) by mout01.posteo.de (Postfix) with ESMTPS id 1EDD924055A for ; Fri, 27 Jan 2023 15:16:20 +0100 (CET) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=posteo.de; s=2017; t=1674828980; bh=qS6CeRmKL5dAkKYTJee0+lCSshB9n79V63ChM5B1EMk=; h=From:Subject:Date:Cc:To:From; b=LhiS58r/tosHskmYP5lW9XxQPxQ1N8qzyW7RSEp1ptcn8X8uLHnguK1oymj4PJua4 GCUEx3DH7+ennxyBAIxqVrSUGYE9inNNqGXV1M5tXbLVZrBEfs+wsWfV0eDW6KiKm8 YAVgged1N7ZYoII5Bsc3ZET9x7j6EPZZZbYEU5Qh8nonps3pWaI88sSqXCIG7xiODe WBqyRXqsiluXhFUIS6bHef1ZB8I4hJjkRb/RKIth4ZHt3N+wGxtYMek/T6NPEemzR3 DJ9L6up0+HcbuuSN1g+ls+utFonG2KkzmnwH+sh0Xs3idkjrsduH7bokBdvVxzzzdN srSJ0yaSGeD4Q== Received: from customer (localhost [127.0.0.1]) by submission (posteo.de) with ESMTPSA id 4P3KPb2P6tz6tmg; Fri, 27 Jan 2023 15:16:19 +0100 (CET) From: =?UTF-8?B?TWFydmluIEjDpHVzZXI=?= Message-Id: Mime-Version: 1.0 (Mac OS X Mail 16.0 \(3731.400.51.1.1\)) Subject: Re: [edk2-platforms][PATCH v3 01/11] Ext4Pkg: Fix memory leak in Ext4RetrieveDirent Date: Fri, 27 Jan 2023 14:16:08 +0000 In-Reply-To: Cc: Savva Mitrofanov , edk2-devel-groups-io , Vitaly Cheptsov To: Pedro Falcato References: <20230127092945.94389-1-savvamtr@gmail.com> <20230127092945.94389-2-savvamtr@gmail.com> Content-Type: multipart/alternative; boundary="Apple-Mail=_46C09F95-2BFC-4AF2-8544-F87BEE053A35" --Apple-Mail=_46C09F95-2BFC-4AF2-8544-F87BEE053A35 Content-Transfer-Encoding: quoted-printable Content-Type: text/plain; charset=utf-8 Reviewed-by: Marvin H=C3=A4user > > On 27. Jan 2023, at 15:12, Pedro Falcato = wrote: >=20 > On Fri, Jan 27, 2023 at 9:29 AM Savva Mitrofanov = wrote: >>=20 >> We need to free buffer on return if BlockRemainder !=3D 0. Also = changed >> return logic from function to use use common exit to prevent code >> duplication. >>=20 >> Cc: Marvin H=C3=A4user >> Cc: Pedro Falcato >> Cc: Vitaly Cheptsov >> Fixes: d9ceedca6c8f ("Ext4Pkg: Add Ext4Dxe driver.") >> Signed-off-by: Savva Mitrofanov >> --- >> Features/Ext4Pkg/Ext4Dxe/Directory.c | 30 +++++++++++--------- >> 1 file changed, 16 insertions(+), 14 deletions(-) >>=20 >> 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; >>=20 >> - Status =3D EFI_NOT_FOUND; >> - Buf =3D AllocatePool (Partition->BlockSize); >> + Buf =3D AllocatePool (Partition->BlockSize); >>=20 >> if (Buf =3D=3D NULL) { >> return EFI_OUT_OF_RESOURCES; >> @@ -128,7 +127,8 @@ Ext4RetrieveDirent ( >> DivU64x32Remainder (DirInoSize, Partition->BlockSize, = &BlockRemainder); >> if (BlockRemainder !=3D 0) { >> // Directory inodes need to have block aligned sizes >> - return EFI_VOLUME_CORRUPTED; >> + Status =3D EFI_VOLUME_CORRUPTED; >> + goto Out; >> } >>=20 >> while (Off < DirInoSize) { >> @@ -137,8 +137,7 @@ Ext4RetrieveDirent ( >> Status =3D Ext4Read (Partition, Directory, Buf, Off, &Length); >>=20 >> if (Status !=3D EFI_SUCCESS) { >> - FreePool (Buf); >> - return Status; >> + goto Out; >> } >>=20 >> for (BlockOffset =3D 0; BlockOffset < Partition->BlockSize; ) { >> @@ -146,19 +145,19 @@ Ext4RetrieveDirent ( >> RemainingBlock =3D 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 =3D EFI_VOLUME_CORRUPTED; >> + goto Out; >> } >>=20 >> if (!Ext4ValidDirent (Entry)) { >> - FreePool (Buf); >> - return EFI_VOLUME_CORRUPTED; >> + Status =3D EFI_VOLUME_CORRUPTED; >> + goto Out; >> } >>=20 >> if ((Entry->name_len > RemainingBlock) || (Entry->rec_len > = RemainingBlock)) { >> // Corrupted filesystem >> - FreePool (Buf); >> - return EFI_VOLUME_CORRUPTED; >> + Status =3D EFI_VOLUME_CORRUPTED; >> + goto Out; >> } >>=20 >> // Unused entry >> @@ -193,8 +192,8 @@ Ext4RetrieveDirent ( >> ToCopy =3D MIN (Entry->rec_len, sizeof (EXT4_DIR_ENTRY)); >>=20 >> CopyMem (Result, Entry, ToCopy); >> - FreePool (Buf); >> - return EFI_SUCCESS; >> + Status =3D EFI_SUCCESS; >> + goto Out; >> } >>=20 >> BlockOffset +=3D Entry->rec_len; >> @@ -203,8 +202,11 @@ Ext4RetrieveDirent ( >> Off +=3D Partition->BlockSize; >> } >>=20 >> + Status =3D EFI_NOT_FOUND; >> + >> +Out: >> FreePool (Buf); >> - return EFI_NOT_FOUND; >> + return Status; >> } >>=20 >> /** >> -- >> 2.39.0 >>=20 >=20 > Reviewed-by: Pedro Falcato > --=20 > Pedro --Apple-Mail=_46C09F95-2BFC-4AF2-8544-F87BEE053A35 Content-Transfer-Encoding: quoted-printable Content-Type: text/html; charset=utf-8 Reviewed-by: = Marvin H=C3=A4user <mhaeuser@posteo.de>

=
On 27. Jan 2023, at 15:12, Pedro Falcato = <pedro.falcato@gmail.com> wrote:

On Fri, Jan 27, 2023 at = 9:29 AM Savva Mitrofanov <savvamtr@gmail.com> = wrote:

We need to free buffer on return = if BlockRemainder !=3D 0. Also changed
return logic from function to = use use common exit to prevent code
duplication.

Cc: Marvin = H=C3=A4user <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 =3D EFI_NOT_FOUND;
-  Buf =    =3D AllocatePool (Partition->BlockSize);
+ =  Buf =3D AllocatePool (Partition->BlockSize);

=   if (Buf =3D=3D NULL) {
    return = EFI_OUT_OF_RESOURCES;
@@ -128,7 +127,8 @@ Ext4RetrieveDirent (
=   DivU64x32Remainder (DirInoSize, Partition->BlockSize, = &BlockRemainder);
  if (BlockRemainder !=3D 0) {
=     // Directory inodes need to have block aligned = sizes
-    return EFI_VOLUME_CORRUPTED;
+ =    Status =3D EFI_VOLUME_CORRUPTED;
+ =    goto Out;
  }

  while = (Off < DirInoSize) {
@@ -137,8 +137,7 @@ Ext4RetrieveDirent (
=     Status =3D Ext4Read (Partition, Directory, Buf, = Off, &Length);

    if (Status !=3D = EFI_SUCCESS) {
-      FreePool (Buf);
- =      return Status;
+ =      goto Out;
=     }

    for = (BlockOffset =3D 0; BlockOffset < Partition->BlockSize; ) {
@@ = -146,19 +145,19 @@ Ext4RetrieveDirent (
=       RemainingBlock =3D = 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 =3D = EFI_VOLUME_CORRUPTED;
+ =        goto Out;
=       }

=       if (!Ext4ValidDirent (Entry)) {
- =        FreePool (Buf);
- =        return = EFI_VOLUME_CORRUPTED;
+ =        Status =3D = EFI_VOLUME_CORRUPTED;
+ =        goto Out;
=       }

=       if ((Entry->name_len > = RemainingBlock) || (Entry->rec_len > RemainingBlock)) {
=         // Corrupted = filesystem
-        FreePool = (Buf);
-        return = EFI_VOLUME_CORRUPTED;
+ =        Status =3D = EFI_VOLUME_CORRUPTED;
+ =        goto Out;
=       }

=       // Unused entry
@@ -193,8 +192,8 = @@ Ext4RetrieveDirent (
=         ToCopy =3D MIN = (Entry->rec_len, sizeof (EXT4_DIR_ENTRY));

=         CopyMem (Result, Entry, = ToCopy);
-        FreePool = (Buf);
-        return = EFI_SUCCESS;
+        Status =3D = EFI_SUCCESS;
+        goto = Out;
      }

=       BlockOffset +=3D = Entry->rec_len;
@@ -203,8 +202,11 @@ Ext4RetrieveDirent (
=     Off +=3D Partition->BlockSize;
=   }

+  Status =3D EFI_NOT_FOUND;
+
+Out:
=   FreePool (Buf);
-  return EFI_NOT_FOUND;
+ =  return Status;
}

= /**
--
2.39.0


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

= --Apple-Mail=_46C09F95-2BFC-4AF2-8544-F87BEE053A35--