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.web10.120670.1673716416367555005 for ; Sat, 14 Jan 2023 09:13:36 -0800 Authentication-Results: mx.groups.io; dkim=pass header.i=@posteo.de header.s=2017 header.b=OXijef5q; 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 4131E240113 for ; Sat, 14 Jan 2023 18:13:34 +0100 (CET) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=posteo.de; s=2017; t=1673716414; bh=UXLFpkjNDBCx37DQ2AXelqLBlMrgYdAwoL4WcWWvjZo=; h=From:Subject:Date:Cc:To:From; b=OXijef5qtrCbD3kodOI7mo+uW/u0iGgR9Vfj3mavwcDHLr2ilnEhJ6NHJbizvVBOC RwQ200qZ5vHLYAtnAlSGr3NE7ELSS0hfu5JTNDPBinSP+AL1CyhNjdL1o7MULKpN01 ljftw03T0PPynFve5K4IYTNJFJML1/3Yj9LQGbes5wOgNSMCeKQOjjbEKq1uivjaEB vMs1Kmpn75h7yuUsQ5TY9OZ08MNA3bvNi9K+H71UMnJesMQZjkPFelgTTnyLChlnNc MJ/+75k/c/QZYCv9bmJk2mFgrG+qtaCi+8PmENMzEaV7TJ1gcRxPTUm6bYJsutGl3u gXuhH1tyIa2bA== Received: from customer (localhost [127.0.0.1]) by submission (posteo.de) with ESMTPSA id 4NvPy55Z6Fz6tnv; Sat, 14 Jan 2023 18:13:33 +0100 (CET) From: =?UTF-8?B?TWFydmluIEjDpHVzZXI=?= Message-Id: <5757B126-28E1-47EF-A3A7-078753924D30@posteo.de> Mime-Version: 1.0 (Mac OS X Mail 16.0 \(3731.300.101.1.3\)) Subject: Re: [PATCH 3/3] Ext4Pkg: Fix and clarify handling regarding non-utf8 dir entries Date: Sat, 14 Jan 2023 17:13:23 +0000 In-Reply-To: <20230111235920.252317-6-pedro.falcato@gmail.com> Cc: devel@edk2.groups.io To: Pedro Falcato References: <20230111235920.252317-1-pedro.falcato@gmail.com> <20230111235920.252317-6-pedro.falcato@gmail.com> Content-Type: multipart/alternative; boundary="Apple-Mail=_C4B09F20-9D2D-4150-8B03-38AF9848CE7D" --Apple-Mail=_C4B09F20-9D2D-4150-8B03-38AF9848CE7D Content-Transfer-Encoding: quoted-printable Content-Type: text/plain; charset=utf-8 Reviewed-by: Marvin H=C3=A4user > > On 12. Jan 2023, at 00:59, Pedro Falcato = wrote: >=20 > Previously, the handling was mixed and/or non-existent regarding non = utf-8 > dirent names. Clarify it. >=20 > Signed-off-by: Pedro Falcato > Cc: Marvin H=C3=A4user > --- > Features/Ext4Pkg/Ext4Dxe/Directory.c | 37 ++++++++++++++++++++++------ > Features/Ext4Pkg/Ext4Dxe/Ext4Dxe.h | 8 +++--- > 2 files changed, 34 insertions(+), 11 deletions(-) >=20 > 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 >=20 > - Copyright (c) 2021 Pedro Falcato All rights reserved. > + Copyright (c) 2021 - 2023 Pedro Falcato All rights reserved. >=20 > 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. >=20 > - @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 +=3D Entry->rec_len; > - continue; > + if (Status =3D=3D 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 +=3D Entry->rec_len; > + continue; > + } > + > + // Other sorts of errors should just error out. > + FreePool (Buf); > + return Status; > } >=20 > if ((Entry->name_len =3D=3D StrLen (Name)) && > @@ -436,6 +443,7 @@ Ext4ReadDir ( > EXT4_FILE *TempFile; > BOOLEAN ShouldSkip; > BOOLEAN IsDotOrDotDot; > + CHAR16 DirentUcs2Name[EXT4_NAME_MAX + 1]; >=20 > DirIno =3D File->Inode; > Status =3D EFI_SUCCESS; > @@ -505,6 +513,21 @@ Ext4ReadDir ( > continue; > } >=20 > + // 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 =3D Ext4GetUcs2DirentName (&Entry, DirentUcs2Name); > + > + if (EFI_ERROR (Status)) { > + if (Status =3D=3D EFI_INVALID_PARAMETER) { > + // Bad UTF-8, skip. > + Offset +=3D Entry.rec_len; > + continue; > + } > + > + goto Out; > + } > + > Status =3D Ext4OpenDirent (Partition, EFI_FILE_MODE_READ, = &TempFile, &Entry, File); >=20 > 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 >=20 > @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. >=20 > - @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 ( > --=20 > 2.39.0 >=20 --Apple-Mail=_C4B09F20-9D2D-4150-8B03-38AF9848CE7D Content-Transfer-Encoding: quoted-printable Content-Type: text/html; charset=utf-8 Reviewed-by: = Marvin H=C3=A4user <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=C3=A4user = <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 =             &n= bsp;The filename was succesfully retrieved and converted to UCS2.
+ =   @retval EFI_INVALID_PARAMETER    The filename = is not valid UTF-8.
+   @retval !EFI_SUCCESS =             Fa= ilure.
**/
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 +=3D = Entry->rec_len;
- =        continue;
+ =        if (Status =3D=3D = 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 +=3D = Entry->rec_len;
+ =          continue;
+ =        }
+
+ =        // Other sorts of errors = should just error out.
+ =        FreePool (Buf);
+ =        return Status;
=       }

=       if ((Entry->name_len =3D=3D = StrLen (Name)) &&
@@ -436,6 +443,7 @@ Ext4ReadDir (
=   EXT4_FILE       *TempFile;
=   BOOLEAN =         ShouldSkip;
=   BOOLEAN =         IsDotOrDotDot;
+ =  CHAR16 =          DirentUcs2Name[EXT4_= NAME_MAX + 1];

  DirIno     =3D = File->Inode;
  Status     =3D = 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 =3D Ext4GetUcs2DirentName (&Entry, = DirentUcs2Name);
+
+    if (EFI_ERROR (Status)) = {
+      if (Status =3D=3D = EFI_INVALID_PARAMETER) {
+ =        // Bad UTF-8, skip.
+ =        Offset +=3D = Entry.rec_len;
+ =        continue;
+ =      }
+
+ =      goto Out;
+ =    }
+
    Status =3D = 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 =             &n= bsp;The filename was succesfully retrieved and converted to UCS2.
+ =   @retval EFI_INVALID_PARAMETER    The filename = is not valid UTF-8.
+   @retval !EFI_SUCCESS =             Fa= ilure.
**/
EFI_STATUS
Ext4GetUcs2DirentName (
-- =
2.39.0


= --Apple-Mail=_C4B09F20-9D2D-4150-8B03-38AF9848CE7D--