[PATCH 0/3] Ext4Pkg: Small ext4 fixes and improvements

[PATCH 0/3] Ext4Pkg: Small ext4 fixes and improvements

[Pedro Falcato]

This patch set includes small fixes on Ext4Dxe, including a fix for a (mostly-harmless) out-of-bounds.
Previous versions of these patches were posted standalone before and are now being consolidated into a
patch-set for easier reviewing.

Cc: Marvin Häuser <mhaeuser@posteo.de>
Cc: Savva Mitrofanov <savvamtr@gmail.com>

Pedro Falcato (3):
  Ext4Pkg: Fix out-of-bounds read in Ext4ReadDir
  Ext4Pkg: Add documentation surrounding ext4 directory entries
  Ext4Pkg: Fix and clarify handling regarding non-utf8 dir entries

 Features/Ext4Pkg/Ext4Dxe/Directory.c | 49 +++++++++++++++++++++-------
 Features/Ext4Pkg/Ext4Dxe/Ext4Disk.h  | 21 ++++++++++--
 Features/Ext4Pkg/Ext4Dxe/Ext4Dxe.h   | 13 ++++----
 3 files changed, 62 insertions(+), 21 deletions(-)

-- 
2.39.0

[PATCH 1/2] Ext4Pkg: Add documentation surrounding ext4 directory entries

[Pedro Falcato]

Several questions have popped up regarding the ext4 directory entry
layout and contents off-list. Attempt to clarify these issues by adding
some explanatory comments.

Signed-off-by: Pedro Falcato <pedro.falcato@gmail.com>
Cc: Marvin Häuser <mhaeuser@posteo.de>
---
 Features/Ext4Pkg/Ext4Dxe/Ext4Disk.h | 21 +++++++++++++++++++--
 Features/Ext4Pkg/Ext4Dxe/Ext4Dxe.h  |  5 ++---
 2 files changed, 21 insertions(+), 5 deletions(-)

diff --git a/Features/Ext4Pkg/Ext4Dxe/Ext4Disk.h b/Features/Ext4Pkg/Ext4Dxe/Ext4Disk.h
index 4fd91a423324..d0a455d0e572 100644
--- a/Features/Ext4Pkg/Ext4Dxe/Ext4Disk.h
+++ b/Features/Ext4Pkg/Ext4Dxe/Ext4Disk.h
@@ -1,7 +1,7 @@
 /** @file
   Raw filesystem data structures
 
-  Copyright (c) 2021 Pedro Falcato All rights reserved.
+  Copyright (c) 2021 - 2023 Pedro Falcato All rights reserved.
   SPDX-License-Identifier: BSD-2-Clause-Patent
 
   Layout of an EXT2/3/4 filesystem:
@@ -397,12 +397,29 @@ typedef struct _Ext4Inode {
   UINT32       i_projid;
 } EXT4_INODE;
 
+#define EXT4_NAME_MAX  255
+
 typedef struct {
+  // offset 0x0: inode number (if 0, unused entry, should skip.)
   UINT32    inode;
+  // offset 0x4: Directory entry's length.
+  //             Note: rec_len >= name_len + EXT4_MIN_DIR_ENTRY_LEN and rec_len % 4 == 0.
   UINT16    rec_len;
+  // offset 0x6: Directory entry's name's length
   UINT8     name_len;
+  // offset 0x7: Directory entry's file type indicator
   UINT8     file_type;
-  CHAR8     name[255];
+  // offset 0x8: name[name_len]: Variable length character array; not null-terminated.
+  CHAR8     name[EXT4_NAME_MAX];
+  // Further notes on names:
+  // 1) We use EXT4_NAME_MAX here instead of flexible arrays for ease of use around the driver.
+  //
+  // 2) ext4 directories are defined, as the documentation puts it, as:
+  // "a directory is more or less a flat file that maps an arbitrary byte string
+  // (usually ASCII) to an inode number on the filesystem". So, they are not
+  // necessarily encoded with ASCII, UTF-8, or any of the sort. We must treat it
+  // as a bag of bytes. When interacting with EFI interfaces themselves (which expect UCS-2)
+  // we skip any directory entry that is not valid UTF-8.
 } EXT4_DIR_ENTRY;
 
 #define EXT4_MIN_DIR_ENTRY_LEN  8
diff --git a/Features/Ext4Pkg/Ext4Dxe/Ext4Dxe.h b/Features/Ext4Pkg/Ext4Dxe/Ext4Dxe.h
index adf3c13f6ea9..466e49523030 100644
--- a/Features/Ext4Pkg/Ext4Dxe/Ext4Dxe.h
+++ b/Features/Ext4Pkg/Ext4Dxe/Ext4Dxe.h
@@ -1,7 +1,7 @@
 /** @file
   Common header for the driver
 
-  Copyright (c) 2021 - 2022 Pedro Falcato All rights reserved.
+  Copyright (c) 2021 - 2023 Pedro Falcato All rights reserved.
   SPDX-License-Identifier: BSD-2-Clause-Patent
 **/
 
@@ -31,8 +31,7 @@
 
 #include "Ext4Disk.h"
 
-#define SYMLOOP_MAX    8
-#define EXT4_NAME_MAX  255
+#define SYMLOOP_MAX  8
 //
 // We need to specify path length limit for security purposes, to prevent possible
 // overflows and dead-loop conditions. Originally this limit is absent in FS design,
-- 
2.39.0

[PATCH 1/3] Ext4Pkg: Fix out-of-bounds read in Ext4ReadDir

[Pedro Falcato]

Fix an out-of-bounds read inside CompareMem() when
checking for "." or ".." by explicitly bounding name_len
to [0, 2] beforehand.

Reported-by: Savva Mitrofanov <savvamtr@gmail.com>
Fixes: 45e37d8533ca8 ("Ext4Pkg: Hide "." and ".." entries from Read() callers.")
Cc: Marvin Häuser <mhaeuser@posteo.de>
Signed-off-by: Pedro Falcato <pedro.falcato@gmail.com>
---
 Features/Ext4Pkg/Ext4Dxe/Directory.c | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

diff --git a/Features/Ext4Pkg/Ext4Dxe/Directory.c b/Features/Ext4Pkg/Ext4Dxe/Directory.c
index 4441e6d192b6..6ed664fc632f 100644
--- a/Features/Ext4Pkg/Ext4Dxe/Directory.c
+++ b/Features/Ext4Pkg/Ext4Dxe/Directory.c
@@ -491,12 +491,14 @@ Ext4ReadDir (
     // or a checksum at the end of the directory block.
     // memcmp (and CompareMem) return 0 when the passed length is 0.
 
-    IsDotOrDotDot = Entry.name_len != 0 &&
-                    (CompareMem (Entry.name, ".", Entry.name_len) == 0 ||
-                     CompareMem (Entry.name, "..", Entry.name_len) == 0);
+    // We must bound name_len as > 0 and <= 2 to avoid any out-of-bounds accesses or bad detection of
+    // "." and "..".
+    IsDotOrDotDot = Entry.name_len > 0 && Entry.name_len <= 2 &&
+                    CompareMem (Entry.name, "..", Entry.name_len) == 0;
 
-    // When inode = 0, it's unused.
-    ShouldSkip = Entry.inode == 0 || IsDotOrDotDot;
+    // When inode = 0, it's unused. When name_len == 0, it's a nameless entry
+    // (which we should not expose to ReadDir).
+    ShouldSkip = Entry.inode == 0 || Entry.name_len == 0 || IsDotOrDotDot;
 
     if (ShouldSkip) {
       Offset += Entry.rec_len;
-- 
2.39.0

[PATCH 2/3] Ext4Pkg: Add documentation surrounding ext4 directory entries

[Pedro Falcato]

Several questions have popped up regarding the ext4 directory entry
layout and contents off-list. Attempt to clarify these issues by adding
some explanatory comments.

Signed-off-by: Pedro Falcato <pedro.falcato@gmail.com>
Cc: Marvin Häuser <mhaeuser@posteo.de>
---
 Features/Ext4Pkg/Ext4Dxe/Ext4Disk.h | 21 +++++++++++++++++++--
 Features/Ext4Pkg/Ext4Dxe/Ext4Dxe.h  |  5 ++---
 2 files changed, 21 insertions(+), 5 deletions(-)

diff --git a/Features/Ext4Pkg/Ext4Dxe/Ext4Disk.h b/Features/Ext4Pkg/Ext4Dxe/Ext4Disk.h
index 4fd91a423324..d0a455d0e572 100644
--- a/Features/Ext4Pkg/Ext4Dxe/Ext4Disk.h
+++ b/Features/Ext4Pkg/Ext4Dxe/Ext4Disk.h
@@ -1,7 +1,7 @@
 /** @file
   Raw filesystem data structures
 
-  Copyright (c) 2021 Pedro Falcato All rights reserved.
+  Copyright (c) 2021 - 2023 Pedro Falcato All rights reserved.
   SPDX-License-Identifier: BSD-2-Clause-Patent
 
   Layout of an EXT2/3/4 filesystem:
@@ -397,12 +397,29 @@ typedef struct _Ext4Inode {
   UINT32       i_projid;
 } EXT4_INODE;
 
+#define EXT4_NAME_MAX  255
+
 typedef struct {
+  // offset 0x0: inode number (if 0, unused entry, should skip.)
   UINT32    inode;
+  // offset 0x4: Directory entry's length.
+  //             Note: rec_len >= name_len + EXT4_MIN_DIR_ENTRY_LEN and rec_len % 4 == 0.
   UINT16    rec_len;
+  // offset 0x6: Directory entry's name's length
   UINT8     name_len;
+  // offset 0x7: Directory entry's file type indicator
   UINT8     file_type;
-  CHAR8     name[255];
+  // offset 0x8: name[name_len]: Variable length character array; not null-terminated.
+  CHAR8     name[EXT4_NAME_MAX];
+  // Further notes on names:
+  // 1) We use EXT4_NAME_MAX here instead of flexible arrays for ease of use around the driver.
+  //
+  // 2) ext4 directories are defined, as the documentation puts it, as:
+  // "a directory is more or less a flat file that maps an arbitrary byte string
+  // (usually ASCII) to an inode number on the filesystem". So, they are not
+  // necessarily encoded with ASCII, UTF-8, or any of the sort. We must treat it
+  // as a bag of bytes. When interacting with EFI interfaces themselves (which expect UCS-2)
+  // we skip any directory entry that is not valid UTF-8.
 } EXT4_DIR_ENTRY;
 
 #define EXT4_MIN_DIR_ENTRY_LEN  8
diff --git a/Features/Ext4Pkg/Ext4Dxe/Ext4Dxe.h b/Features/Ext4Pkg/Ext4Dxe/Ext4Dxe.h
index adf3c13f6ea9..466e49523030 100644
--- a/Features/Ext4Pkg/Ext4Dxe/Ext4Dxe.h
+++ b/Features/Ext4Pkg/Ext4Dxe/Ext4Dxe.h
@@ -1,7 +1,7 @@
 /** @file
   Common header for the driver
 
-  Copyright (c) 2021 - 2022 Pedro Falcato All rights reserved.
+  Copyright (c) 2021 - 2023 Pedro Falcato All rights reserved.
   SPDX-License-Identifier: BSD-2-Clause-Patent
 **/
 
@@ -31,8 +31,7 @@
 
 #include "Ext4Disk.h"
 
-#define SYMLOOP_MAX    8
-#define EXT4_NAME_MAX  255
+#define SYMLOOP_MAX  8
 //
 // We need to specify path length limit for security purposes, to prevent possible
 // overflows and dead-loop conditions. Originally this limit is absent in FS design,
-- 
2.39.0

[PATCH 2/2] Ext4Pkg: Fix and clarify handling regarding non-utf8 dir entries

[Pedro Falcato]

Previously, the handling was mixed and/or non-existent regarding non utf-8
dirent names. Clarify it.

Signed-off-by: Pedro Falcato <pedro.falcato@gmail.com>
Cc: Marvin Häuser <mhaeuser@posteo.de>
---
 Features/Ext4Pkg/Ext4Dxe/Directory.c | 37 ++++++++++++++++++++++------
 Features/Ext4Pkg/Ext4Dxe/Ext4Dxe.h   |  8 +++---
 2 files changed, 34 insertions(+), 11 deletions(-)

diff --git a/Features/Ext4Pkg/Ext4Dxe/Directory.c b/Features/Ext4Pkg/Ext4Dxe/Directory.c
index 6ed664fc632f..ba781bad968c 100644
--- a/Features/Ext4Pkg/Ext4Dxe/Directory.c
+++ b/Features/Ext4Pkg/Ext4Dxe/Directory.c
@@ -1,7 +1,7 @@
 /** @file
   Directory related routines
 
-  Copyright (c) 2021 Pedro Falcato All rights reserved.
+  Copyright (c) 2021 - 2023 Pedro Falcato All rights reserved.
 
   SPDX-License-Identifier: BSD-2-Clause-Patent
 **/
@@ -16,8 +16,9 @@
    @param[in]      Entry   Pointer to a EXT4_DIR_ENTRY.
    @param[out]      Ucs2FileName   Pointer to an array of CHAR16's, of size EXT4_NAME_MAX + 1.
 
-   @retval EFI_SUCCESS   The filename was succesfully retrieved and converted to UCS2.
-   @retval !EFI_SUCCESS  Failure.
+   @retval EFI_SUCCESS              The filename was succesfully retrieved and converted to UCS2.
+   @retval EFI_INVALID_PARAMETER    The filename is not valid UTF-8.
+   @retval !EFI_SUCCESS             Failure.
 **/
 EFI_STATUS
 Ext4GetUcs2DirentName (
@@ -174,10 +175,16 @@ Ext4RetrieveDirent (
        * need to form valid ASCII/UTF-8 sequences.
        */
       if (EFI_ERROR (Status)) {
-        // If we error out, skip this entry
-        // I'm not sure if this is correct behaviour, but I don't think there's a precedent here.
-        BlockOffset += Entry->rec_len;
-        continue;
+        if (Status == EFI_INVALID_PARAMETER) {
+          // If we error out due to a bad UTF-8 sequence (see Ext4GetUcs2DirentName), skip this entry.
+          // I'm not sure if this is correct behaviour, but I don't think there's a precedent here.
+          BlockOffset += Entry->rec_len;
+          continue;
+        }
+
+        // Other sorts of errors should just error out.
+        FreePool (Buf);
+        return Status;
       }
 
       if ((Entry->name_len == StrLen (Name)) &&
@@ -436,6 +443,7 @@ Ext4ReadDir (
   EXT4_FILE       *TempFile;
   BOOLEAN         ShouldSkip;
   BOOLEAN         IsDotOrDotDot;
+  CHAR16          DirentUcs2Name[EXT4_NAME_MAX + 1];
 
   DirIno     = File->Inode;
   Status     = EFI_SUCCESS;
@@ -505,6 +513,21 @@ Ext4ReadDir (
       continue;
     }
 
+    // Test if the dirent is valid utf-8. This is already done inside Ext4OpenDirent but EFI_INVALID_PARAMETER
+    // has the danger of its meaning being overloaded in many places, so we can't skip according to that.
+    // So test outside of it, explicitly.
+    Status = Ext4GetUcs2DirentName (&Entry, DirentUcs2Name);
+
+    if (EFI_ERROR (Status)) {
+      if (Status == EFI_INVALID_PARAMETER) {
+        // Bad UTF-8, skip.
+        Offset += Entry.rec_len;
+        continue;
+      }
+
+      goto Out;
+    }
+
     Status = Ext4OpenDirent (Partition, EFI_FILE_MODE_READ, &TempFile, &Entry, File);
 
     if (EFI_ERROR (Status)) {
diff --git a/Features/Ext4Pkg/Ext4Dxe/Ext4Dxe.h b/Features/Ext4Pkg/Ext4Dxe/Ext4Dxe.h
index 466e49523030..41779dad855f 100644
--- a/Features/Ext4Pkg/Ext4Dxe/Ext4Dxe.h
+++ b/Features/Ext4Pkg/Ext4Dxe/Ext4Dxe.h
@@ -944,11 +944,11 @@ Ext4StrCmpInsensitive (
    Retrieves the filename of the directory entry and converts it to UTF-16/UCS-2
 
    @param[in]      Entry   Pointer to a EXT4_DIR_ENTRY.
-   @param[out]      Ucs2FileName   Pointer to an array of CHAR16's, of size
-EXT4_NAME_MAX + 1.
+   @param[out]      Ucs2FileName   Pointer to an array of CHAR16's, of size EXT4_NAME_MAX + 1.
 
-   @retval EFI_SUCCESS   Unicode collation was successfully initialised.
-   @retval !EFI_SUCCESS  Failure.
+   @retval EFI_SUCCESS              The filename was succesfully retrieved and converted to UCS2.
+   @retval EFI_INVALID_PARAMETER    The filename is not valid UTF-8.
+   @retval !EFI_SUCCESS             Failure.
 **/
 EFI_STATUS
 Ext4GetUcs2DirentName (
-- 
2.39.0

[PATCH 3/3] Ext4Pkg: Fix and clarify handling regarding non-utf8 dir entries

[Pedro Falcato]

Previously, the handling was mixed and/or non-existent regarding non utf-8
dirent names. Clarify it.

Signed-off-by: Pedro Falcato <pedro.falcato@gmail.com>
Cc: Marvin Häuser <mhaeuser@posteo.de>
---
 Features/Ext4Pkg/Ext4Dxe/Directory.c | 37 ++++++++++++++++++++++------
 Features/Ext4Pkg/Ext4Dxe/Ext4Dxe.h   |  8 +++---
 2 files changed, 34 insertions(+), 11 deletions(-)

diff --git a/Features/Ext4Pkg/Ext4Dxe/Directory.c b/Features/Ext4Pkg/Ext4Dxe/Directory.c
index 6ed664fc632f..ba781bad968c 100644
--- a/Features/Ext4Pkg/Ext4Dxe/Directory.c
+++ b/Features/Ext4Pkg/Ext4Dxe/Directory.c
@@ -1,7 +1,7 @@
 /** @file
   Directory related routines
 
-  Copyright (c) 2021 Pedro Falcato All rights reserved.
+  Copyright (c) 2021 - 2023 Pedro Falcato All rights reserved.
 
   SPDX-License-Identifier: BSD-2-Clause-Patent
 **/
@@ -16,8 +16,9 @@
    @param[in]      Entry   Pointer to a EXT4_DIR_ENTRY.
    @param[out]      Ucs2FileName   Pointer to an array of CHAR16's, of size EXT4_NAME_MAX + 1.
 
-   @retval EFI_SUCCESS   The filename was succesfully retrieved and converted to UCS2.
-   @retval !EFI_SUCCESS  Failure.
+   @retval EFI_SUCCESS              The filename was succesfully retrieved and converted to UCS2.
+   @retval EFI_INVALID_PARAMETER    The filename is not valid UTF-8.
+   @retval !EFI_SUCCESS             Failure.
 **/
 EFI_STATUS
 Ext4GetUcs2DirentName (
@@ -174,10 +175,16 @@ Ext4RetrieveDirent (
        * need to form valid ASCII/UTF-8 sequences.
        */
       if (EFI_ERROR (Status)) {
-        // If we error out, skip this entry
-        // I'm not sure if this is correct behaviour, but I don't think there's a precedent here.
-        BlockOffset += Entry->rec_len;
-        continue;
+        if (Status == EFI_INVALID_PARAMETER) {
+          // If we error out due to a bad UTF-8 sequence (see Ext4GetUcs2DirentName), skip this entry.
+          // I'm not sure if this is correct behaviour, but I don't think there's a precedent here.
+          BlockOffset += Entry->rec_len;
+          continue;
+        }
+
+        // Other sorts of errors should just error out.
+        FreePool (Buf);
+        return Status;
       }
 
       if ((Entry->name_len == StrLen (Name)) &&
@@ -436,6 +443,7 @@ Ext4ReadDir (
   EXT4_FILE       *TempFile;
   BOOLEAN         ShouldSkip;
   BOOLEAN         IsDotOrDotDot;
+  CHAR16          DirentUcs2Name[EXT4_NAME_MAX + 1];
 
   DirIno     = File->Inode;
   Status     = EFI_SUCCESS;
@@ -505,6 +513,21 @@ Ext4ReadDir (
       continue;
     }
 
+    // Test if the dirent is valid utf-8. This is already done inside Ext4OpenDirent but EFI_INVALID_PARAMETER
+    // has the danger of its meaning being overloaded in many places, so we can't skip according to that.
+    // So test outside of it, explicitly.
+    Status = Ext4GetUcs2DirentName (&Entry, DirentUcs2Name);
+
+    if (EFI_ERROR (Status)) {
+      if (Status == EFI_INVALID_PARAMETER) {
+        // Bad UTF-8, skip.
+        Offset += Entry.rec_len;
+        continue;
+      }
+
+      goto Out;
+    }
+
     Status = Ext4OpenDirent (Partition, EFI_FILE_MODE_READ, &TempFile, &Entry, File);
 
     if (EFI_ERROR (Status)) {
diff --git a/Features/Ext4Pkg/Ext4Dxe/Ext4Dxe.h b/Features/Ext4Pkg/Ext4Dxe/Ext4Dxe.h
index 466e49523030..41779dad855f 100644
--- a/Features/Ext4Pkg/Ext4Dxe/Ext4Dxe.h
+++ b/Features/Ext4Pkg/Ext4Dxe/Ext4Dxe.h
@@ -944,11 +944,11 @@ Ext4StrCmpInsensitive (
    Retrieves the filename of the directory entry and converts it to UTF-16/UCS-2
 
    @param[in]      Entry   Pointer to a EXT4_DIR_ENTRY.
-   @param[out]      Ucs2FileName   Pointer to an array of CHAR16's, of size
-EXT4_NAME_MAX + 1.
+   @param[out]      Ucs2FileName   Pointer to an array of CHAR16's, of size EXT4_NAME_MAX + 1.
 
-   @retval EFI_SUCCESS   Unicode collation was successfully initialised.
-   @retval !EFI_SUCCESS  Failure.
+   @retval EFI_SUCCESS              The filename was succesfully retrieved and converted to UCS2.
+   @retval EFI_INVALID_PARAMETER    The filename is not valid UTF-8.
+   @retval !EFI_SUCCESS             Failure.
 **/
 EFI_STATUS
 Ext4GetUcs2DirentName (
-- 
2.39.0

Re: [edk2-devel] [PATCH 2/3] Ext4Pkg: Add documentation surrounding ext4 directory entries

[Pedro Falcato]

[-- Attachment #1: Type: text/plain, Size: 3575 bytes --]

Apologies for the duplicate send of these patches, small mistake using git
send-email *.patch ;)
Please only consider the patches that start with [PATCH N/3].

On Wed, Jan 11, 2023 at 11:59 PM Pedro Falcato via groups.io <pedro.falcato=
gmail.com@groups.io> wrote:

> Several questions have popped up regarding the ext4 directory entry
> layout and contents off-list. Attempt to clarify these issues by adding
> some explanatory comments.
>
> Signed-off-by: Pedro Falcato <pedro.falcato@gmail.com>
> Cc: Marvin Häuser <mhaeuser@posteo.de>
> ---
>  Features/Ext4Pkg/Ext4Dxe/Ext4Disk.h | 21 +++++++++++++++++++--
>  Features/Ext4Pkg/Ext4Dxe/Ext4Dxe.h  |  5 ++---
>  2 files changed, 21 insertions(+), 5 deletions(-)
>
> diff --git a/Features/Ext4Pkg/Ext4Dxe/Ext4Disk.h
> b/Features/Ext4Pkg/Ext4Dxe/Ext4Disk.h
> index 4fd91a423324..d0a455d0e572 100644
> --- a/Features/Ext4Pkg/Ext4Dxe/Ext4Disk.h
> +++ b/Features/Ext4Pkg/Ext4Dxe/Ext4Disk.h
> @@ -1,7 +1,7 @@
>  /** @file
>    Raw filesystem data structures
>
> -  Copyright (c) 2021 Pedro Falcato All rights reserved.
> +  Copyright (c) 2021 - 2023 Pedro Falcato All rights reserved.
>    SPDX-License-Identifier: BSD-2-Clause-Patent
>
>    Layout of an EXT2/3/4 filesystem:
> @@ -397,12 +397,29 @@ typedef struct _Ext4Inode {
>    UINT32       i_projid;
>  } EXT4_INODE;
>
> +#define EXT4_NAME_MAX  255
> +
>  typedef struct {
> +  // offset 0x0: inode number (if 0, unused entry, should skip.)
>    UINT32    inode;
> +  // offset 0x4: Directory entry's length.
> +  //             Note: rec_len >= name_len + EXT4_MIN_DIR_ENTRY_LEN and
> rec_len % 4 == 0.
>    UINT16    rec_len;
> +  // offset 0x6: Directory entry's name's length
>    UINT8     name_len;
> +  // offset 0x7: Directory entry's file type indicator
>    UINT8     file_type;
> -  CHAR8     name[255];
> +  // offset 0x8: name[name_len]: Variable length character array; not
> null-terminated.
> +  CHAR8     name[EXT4_NAME_MAX];
> +  // Further notes on names:
> +  // 1) We use EXT4_NAME_MAX here instead of flexible arrays for ease of
> use around the driver.
> +  //
> +  // 2) ext4 directories are defined, as the documentation puts it, as:
> +  // "a directory is more or less a flat file that maps an arbitrary byte
> string
> +  // (usually ASCII) to an inode number on the filesystem". So, they are
> not
> +  // necessarily encoded with ASCII, UTF-8, or any of the sort. We must
> treat it
> +  // as a bag of bytes. When interacting with EFI interfaces themselves
> (which expect UCS-2)
> +  // we skip any directory entry that is not valid UTF-8.
>  } EXT4_DIR_ENTRY;
>
>  #define EXT4_MIN_DIR_ENTRY_LEN  8
> diff --git a/Features/Ext4Pkg/Ext4Dxe/Ext4Dxe.h
> b/Features/Ext4Pkg/Ext4Dxe/Ext4Dxe.h
> index adf3c13f6ea9..466e49523030 100644
> --- a/Features/Ext4Pkg/Ext4Dxe/Ext4Dxe.h
> +++ b/Features/Ext4Pkg/Ext4Dxe/Ext4Dxe.h
> @@ -1,7 +1,7 @@
>  /** @file
>    Common header for the driver
>
> -  Copyright (c) 2021 - 2022 Pedro Falcato All rights reserved.
> +  Copyright (c) 2021 - 2023 Pedro Falcato All rights reserved.
>    SPDX-License-Identifier: BSD-2-Clause-Patent
>  **/
>
> @@ -31,8 +31,7 @@
>
>  #include "Ext4Disk.h"
>
> -#define SYMLOOP_MAX    8
> -#define EXT4_NAME_MAX  255
> +#define SYMLOOP_MAX  8
>  //
>  // We need to specify path length limit for security purposes, to prevent
> possible
>  // overflows and dead-loop conditions. Originally this limit is absent in
> FS design,
> --
> 2.39.0
>
>
>
> 
>
>
>

-- 
Pedro

[-- Attachment #2: Type: text/html, Size: 4400 bytes --]

Re: [PATCH 1/3] Ext4Pkg: Fix out-of-bounds read in Ext4ReadDir

[Marvin Häuser]

[-- Attachment #1: Type: text/plain, Size: 2060 bytes --]

Reviewed-by: Marvin Häuser <mhaeuser@posteo.de <mailto:mhaeuser@posteo.de>>

(Still two comments for next time)

> On 12. Jan 2023, at 00:59, Pedro Falcato <pedro.falcato@gmail.com> wrote:
> 
> Fix an out-of-bounds read inside CompareMem() when
> checking for "." or ".." by explicitly bounding name_len
> to [0, 2] beforehand.
> 
> Reported-by: Savva Mitrofanov <savvamtr@gmail.com>
> Fixes: 45e37d8533ca8 ("Ext4Pkg: Hide "." and ".." entries from Read() callers.")
> Cc: Marvin Häuser <mhaeuser@posteo.de>
> Signed-off-by: Pedro Falcato <pedro.falcato@gmail.com>
> ---
> Features/Ext4Pkg/Ext4Dxe/Directory.c | 12 +++++++-----
> 1 file changed, 7 insertions(+), 5 deletions(-)
> 
> diff --git a/Features/Ext4Pkg/Ext4Dxe/Directory.c b/Features/Ext4Pkg/Ext4Dxe/Directory.c
> index 4441e6d192b6..6ed664fc632f 100644
> --- a/Features/Ext4Pkg/Ext4Dxe/Directory.c
> +++ b/Features/Ext4Pkg/Ext4Dxe/Directory.c
> @@ -491,12 +491,14 @@ Ext4ReadDir (
>     // or a checksum at the end of the directory block.
>     // memcmp (and CompareMem) return 0 when the passed length is 0.
> 
> -    IsDotOrDotDot = Entry.name_len != 0 &&
> -                    (CompareMem (Entry.name, ".", Entry.name_len) == 0 ||
> -                     CompareMem (Entry.name, "..", Entry.name_len) == 0);
> +    // We must bound name_len as > 0 and <= 2 to avoid any out-of-bounds accesses or bad detection of

"bad detection" = false positive?

> +    // "." and "..".
> +    IsDotOrDotDot = Entry.name_len > 0 && Entry.name_len <= 2 &&
> +                    CompareMem (Entry.name, "..", Entry.name_len) == 0;
> 
> -    // When inode = 0, it's unused.
> -    ShouldSkip = Entry.inode == 0 || IsDotOrDotDot;
> +    // When inode = 0, it's unused. When name_len == 0, it's a nameless entry

Inconsistency between "=" and "==".

> +    // (which we should not expose to ReadDir).
> +    ShouldSkip = Entry.inode == 0 || Entry.name_len == 0 || IsDotOrDotDot;
> 
>     if (ShouldSkip) {
>       Offset += Entry.rec_len;
> -- 
> 2.39.0
> 


[-- Attachment #2: Type: text/html, Size: 3162 bytes --]

Re: [PATCH 2/3] Ext4Pkg: Add documentation surrounding ext4 directory entries

[Marvin Häuser]

[-- Attachment #1: Type: text/plain, Size: 3589 bytes --]

Well, this is still not Doxygen-friendly (requires three slashes) and it's still awkward (as in done nowhere else) to explicitly list the offset or name in the comment. In case neither is a concern:

Reviewed-by: Marvin Häuser <mhaeuser@posteo.de <mailto:mhaeuser@posteo.de>>

> On 12. Jan 2023, at 00:59, Pedro Falcato <pedro.falcato@gmail.com> wrote:
> 
> Several questions have popped up regarding the ext4 directory entry
> layout and contents off-list. Attempt to clarify these issues by adding
> some explanatory comments.
> 
> Signed-off-by: Pedro Falcato <pedro.falcato@gmail.com>
> Cc: Marvin Häuser <mhaeuser@posteo.de>
> ---
> Features/Ext4Pkg/Ext4Dxe/Ext4Disk.h | 21 +++++++++++++++++++--
> Features/Ext4Pkg/Ext4Dxe/Ext4Dxe.h  |  5 ++---
> 2 files changed, 21 insertions(+), 5 deletions(-)
> 
> diff --git a/Features/Ext4Pkg/Ext4Dxe/Ext4Disk.h b/Features/Ext4Pkg/Ext4Dxe/Ext4Disk.h
> index 4fd91a423324..d0a455d0e572 100644
> --- a/Features/Ext4Pkg/Ext4Dxe/Ext4Disk.h
> +++ b/Features/Ext4Pkg/Ext4Dxe/Ext4Disk.h
> @@ -1,7 +1,7 @@
> /** @file
>   Raw filesystem data structures
> 
> -  Copyright (c) 2021 Pedro Falcato All rights reserved.
> +  Copyright (c) 2021 - 2023 Pedro Falcato All rights reserved.
>   SPDX-License-Identifier: BSD-2-Clause-Patent
> 
>   Layout of an EXT2/3/4 filesystem:
> @@ -397,12 +397,29 @@ typedef struct _Ext4Inode {
>   UINT32       i_projid;
> } EXT4_INODE;
> 
> +#define EXT4_NAME_MAX  255
> +
> typedef struct {
> +  // offset 0x0: inode number (if 0, unused entry, should skip.)
>   UINT32    inode;
> +  // offset 0x4: Directory entry's length.
> +  //             Note: rec_len >= name_len + EXT4_MIN_DIR_ENTRY_LEN and rec_len % 4 == 0.
>   UINT16    rec_len;
> +  // offset 0x6: Directory entry's name's length
>   UINT8     name_len;
> +  // offset 0x7: Directory entry's file type indicator
>   UINT8     file_type;
> -  CHAR8     name[255];
> +  // offset 0x8: name[name_len]: Variable length character array; not null-terminated.
> +  CHAR8     name[EXT4_NAME_MAX];
> +  // Further notes on names:
> +  // 1) We use EXT4_NAME_MAX here instead of flexible arrays for ease of use around the driver.
> +  //
> +  // 2) ext4 directories are defined, as the documentation puts it, as:
> +  // "a directory is more or less a flat file that maps an arbitrary byte string
> +  // (usually ASCII) to an inode number on the filesystem". So, they are not
> +  // necessarily encoded with ASCII, UTF-8, or any of the sort. We must treat it
> +  // as a bag of bytes. When interacting with EFI interfaces themselves (which expect UCS-2)
> +  // we skip any directory entry that is not valid UTF-8.
> } EXT4_DIR_ENTRY;
> 
> #define EXT4_MIN_DIR_ENTRY_LEN  8
> diff --git a/Features/Ext4Pkg/Ext4Dxe/Ext4Dxe.h b/Features/Ext4Pkg/Ext4Dxe/Ext4Dxe.h
> index adf3c13f6ea9..466e49523030 100644
> --- a/Features/Ext4Pkg/Ext4Dxe/Ext4Dxe.h
> +++ b/Features/Ext4Pkg/Ext4Dxe/Ext4Dxe.h
> @@ -1,7 +1,7 @@
> /** @file
>   Common header for the driver
> 
> -  Copyright (c) 2021 - 2022 Pedro Falcato All rights reserved.
> +  Copyright (c) 2021 - 2023 Pedro Falcato All rights reserved.
>   SPDX-License-Identifier: BSD-2-Clause-Patent
> **/
> 
> @@ -31,8 +31,7 @@
> 
> #include "Ext4Disk.h"
> 
> -#define SYMLOOP_MAX    8
> -#define EXT4_NAME_MAX  255
> +#define SYMLOOP_MAX  8
> //
> // We need to specify path length limit for security purposes, to prevent possible
> // overflows and dead-loop conditions. Originally this limit is absent in FS design,
> -- 
> 2.39.0
> 


[-- Attachment #2: Type: text/html, Size: 4439 bytes --]

Re: [PATCH 3/3] Ext4Pkg: Fix and clarify handling regarding non-utf8 dir entries

[Marvin Häuser]

[-- Attachment #1: Type: text/plain, Size: 4579 bytes --]

Reviewed-by: Marvin Häuser <mhaeuser@posteo.de <mailto:mhaeuser@posteo.de>>

> On 12. Jan 2023, at 00:59, Pedro Falcato <pedro.falcato@gmail.com> wrote:
> 
> Previously, the handling was mixed and/or non-existent regarding non utf-8
> dirent names. Clarify it.
> 
> Signed-off-by: Pedro Falcato <pedro.falcato@gmail.com>
> Cc: Marvin Häuser <mhaeuser@posteo.de>
> ---
> Features/Ext4Pkg/Ext4Dxe/Directory.c | 37 ++++++++++++++++++++++------
> Features/Ext4Pkg/Ext4Dxe/Ext4Dxe.h   |  8 +++---
> 2 files changed, 34 insertions(+), 11 deletions(-)
> 
> diff --git a/Features/Ext4Pkg/Ext4Dxe/Directory.c b/Features/Ext4Pkg/Ext4Dxe/Directory.c
> index 6ed664fc632f..ba781bad968c 100644
> --- a/Features/Ext4Pkg/Ext4Dxe/Directory.c
> +++ b/Features/Ext4Pkg/Ext4Dxe/Directory.c
> @@ -1,7 +1,7 @@
> /** @file
>   Directory related routines
> 
> -  Copyright (c) 2021 Pedro Falcato All rights reserved.
> +  Copyright (c) 2021 - 2023 Pedro Falcato All rights reserved.
> 
>   SPDX-License-Identifier: BSD-2-Clause-Patent
> **/
> @@ -16,8 +16,9 @@
>    @param[in]      Entry   Pointer to a EXT4_DIR_ENTRY.
>    @param[out]      Ucs2FileName   Pointer to an array of CHAR16's, of size EXT4_NAME_MAX + 1.
> 
> -   @retval EFI_SUCCESS   The filename was succesfully retrieved and converted to UCS2.
> -   @retval !EFI_SUCCESS  Failure.
> +   @retval EFI_SUCCESS              The filename was succesfully retrieved and converted to UCS2.
> +   @retval EFI_INVALID_PARAMETER    The filename is not valid UTF-8.
> +   @retval !EFI_SUCCESS             Failure.
> **/
> EFI_STATUS
> Ext4GetUcs2DirentName (
> @@ -174,10 +175,16 @@ Ext4RetrieveDirent (
>        * need to form valid ASCII/UTF-8 sequences.
>        */
>       if (EFI_ERROR (Status)) {
> -        // If we error out, skip this entry
> -        // I'm not sure if this is correct behaviour, but I don't think there's a precedent here.
> -        BlockOffset += Entry->rec_len;
> -        continue;
> +        if (Status == EFI_INVALID_PARAMETER) {
> +          // If we error out due to a bad UTF-8 sequence (see Ext4GetUcs2DirentName), skip this entry.
> +          // I'm not sure if this is correct behaviour, but I don't think there's a precedent here.
> +          BlockOffset += Entry->rec_len;
> +          continue;
> +        }
> +
> +        // Other sorts of errors should just error out.
> +        FreePool (Buf);
> +        return Status;
>       }
> 
>       if ((Entry->name_len == StrLen (Name)) &&
> @@ -436,6 +443,7 @@ Ext4ReadDir (
>   EXT4_FILE       *TempFile;
>   BOOLEAN         ShouldSkip;
>   BOOLEAN         IsDotOrDotDot;
> +  CHAR16          DirentUcs2Name[EXT4_NAME_MAX + 1];
> 
>   DirIno     = File->Inode;
>   Status     = EFI_SUCCESS;
> @@ -505,6 +513,21 @@ Ext4ReadDir (
>       continue;
>     }
> 
> +    // Test if the dirent is valid utf-8. This is already done inside Ext4OpenDirent but EFI_INVALID_PARAMETER
> +    // has the danger of its meaning being overloaded in many places, so we can't skip according to that.
> +    // So test outside of it, explicitly.
> +    Status = Ext4GetUcs2DirentName (&Entry, DirentUcs2Name);
> +
> +    if (EFI_ERROR (Status)) {
> +      if (Status == EFI_INVALID_PARAMETER) {
> +        // Bad UTF-8, skip.
> +        Offset += Entry.rec_len;
> +        continue;
> +      }
> +
> +      goto Out;
> +    }
> +
>     Status = Ext4OpenDirent (Partition, EFI_FILE_MODE_READ, &TempFile, &Entry, File);
> 
>     if (EFI_ERROR (Status)) {
> diff --git a/Features/Ext4Pkg/Ext4Dxe/Ext4Dxe.h b/Features/Ext4Pkg/Ext4Dxe/Ext4Dxe.h
> index 466e49523030..41779dad855f 100644
> --- a/Features/Ext4Pkg/Ext4Dxe/Ext4Dxe.h
> +++ b/Features/Ext4Pkg/Ext4Dxe/Ext4Dxe.h
> @@ -944,11 +944,11 @@ Ext4StrCmpInsensitive (
>    Retrieves the filename of the directory entry and converts it to UTF-16/UCS-2
> 
>    @param[in]      Entry   Pointer to a EXT4_DIR_ENTRY.
> -   @param[out]      Ucs2FileName   Pointer to an array of CHAR16's, of size
> -EXT4_NAME_MAX + 1.
> +   @param[out]      Ucs2FileName   Pointer to an array of CHAR16's, of size EXT4_NAME_MAX + 1.
> 
> -   @retval EFI_SUCCESS   Unicode collation was successfully initialised.
> -   @retval !EFI_SUCCESS  Failure.
> +   @retval EFI_SUCCESS              The filename was succesfully retrieved and converted to UCS2.
> +   @retval EFI_INVALID_PARAMETER    The filename is not valid UTF-8.
> +   @retval !EFI_SUCCESS             Failure.
> **/
> EFI_STATUS
> Ext4GetUcs2DirentName (
> -- 
> 2.39.0
> 


[-- Attachment #2: Type: text/html, Size: 6968 bytes --]