As Pedro agreed, forgot to add

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

@Pedro feel free to change to UINT8 on push nevertheless. :)

It is not so important from my point of view, however, I corrected this in the referenced repository fork.

On 27. Jan 2023, at 15:09, Pedro Falcato <pedro.falcato@...> wrote:

On Fri, Jan 27, 2023 at 10:04 AM Marvin Häuser <mhaeuser@...> wrote:

On 27. Jan 2023, at 10:29, Savva Mitrofanov <savvamtr@...> wrote:

The directory entry name conventions forbid having null-terminator
symbols in its body and can lead to undefined behavior conditions
and crashes

Cc: Marvin Häuser <mhaeuser@...>
Cc: Pedro Falcato <pedro.falcato@...>
Cc: Vitaly Cheptsov <vit9696@...>
Fixes: 89b2bb0db263 ("Ext4Pkg: Fix and clarify handling regarding non-utf8 dir entries")
Signed-off-by: Savva Mitrofanov <savvamtr@...>
---
Features/Ext4Pkg/Ext4Dxe/Directory.c | 9 ++++++++-
1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/Features/Ext4Pkg/Ext4Dxe/Directory.c b/Features/Ext4Pkg/Ext4Dxe/Directory.c
index 0753a20b5377..465749c9b51d 100644
--- a/Features/Ext4Pkg/Ext4Dxe/Directory.c
+++ b/Features/Ext4Pkg/Ext4Dxe/Directory.c
@@ -28,9 +28,16 @@ Ext4GetUcs2DirentName (
{
CHAR8 Utf8NameBuf[EXT4_NAME_MAX + 1];
UINT16 *Str;
+ UINTN Index;

I *really* do not like UINTN in code that does not deal with buffer addresses and sizes. I'd change it to UINT8, but I'll leave it up to Pedro.

Considering this is a size (length of name) and it gets explicitly

Well, being a "size" is not sufficient, I explicitly referred to buffer sizes. Say, you get an arbitrarily long buffer from a caller, its size parameter should probably be UINTN, to be able to optimally leverage the platform memory and be flexible. This is not a buffer size, this is a bounded index of an architecture-agnostic data structure. I simply do not want behaviour that depends on the host architecture in architecture-agnostic code for no reason. We actually had multiple prior examples of bugs due to this, though this is obviously fine (thus not demanding it's changed).

bounded by name_len (a UINT8) I'm okay with it. But if for some reason
you need to submit a v4, please change.

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

--
Pedro

On Fri, Jan 27, 2023 at 10:04 AM Marvin Häuser <mhaeuser@...> wrote:

On 27. Jan 2023, at 10:29, Savva Mitrofanov <savvamtr@...> wrote:

The directory entry name conventions forbid having null-terminator
symbols in its body and can lead to undefined behavior conditions
and crashes

Cc: Marvin Häuser <mhaeuser@...>
Cc: Pedro Falcato <pedro.falcato@...>
Cc: Vitaly Cheptsov <vit9696@...>
Fixes: 89b2bb0db263 ("Ext4Pkg: Fix and clarify handling regarding non-utf8 dir entries")
Signed-off-by: Savva Mitrofanov <savvamtr@...>
---
Features/Ext4Pkg/Ext4Dxe/Directory.c | 9 ++++++++-
1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/Features/Ext4Pkg/Ext4Dxe/Directory.c b/Features/Ext4Pkg/Ext4Dxe/Directory.c
index 0753a20b5377..465749c9b51d 100644
--- a/Features/Ext4Pkg/Ext4Dxe/Directory.c
+++ b/Features/Ext4Pkg/Ext4Dxe/Directory.c
@@ -28,9 +28,16 @@ Ext4GetUcs2DirentName (
{
CHAR8 Utf8NameBuf[EXT4_NAME_MAX + 1];
UINT16 *Str;
+ UINTN Index;

I *really* do not like UINTN in code that does not deal with buffer addresses and sizes. I'd change it to UINT8, but I'll leave it up to Pedro.

Considering this is a size (length of name) and it gets explicitly
bounded by name_len (a UINT8) I'm okay with it. But if for some reason
you need to submit a v4, please change.

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

--
Pedro

On 27. Jan 2023, at 10:29, Savva Mitrofanov <savvamtr@...> wrote:

The directory entry name conventions forbid having null-terminator
symbols in its body and can lead to undefined behavior conditions
and crashes

Cc: Marvin Häuser <mhaeuser@...>
Cc: Pedro Falcato <pedro.falcato@...>
Cc: Vitaly Cheptsov <vit9696@...>
Fixes: 89b2bb0db263 ("Ext4Pkg: Fix and clarify handling regarding non-utf8 dir entries")
Signed-off-by: Savva Mitrofanov <savvamtr@...>
---
Features/Ext4Pkg/Ext4Dxe/Directory.c | 9 ++++++++-
1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/Features/Ext4Pkg/Ext4Dxe/Directory.c b/Features/Ext4Pkg/Ext4Dxe/Directory.c
index 0753a20b5377..465749c9b51d 100644
--- a/Features/Ext4Pkg/Ext4Dxe/Directory.c
+++ b/Features/Ext4Pkg/Ext4Dxe/Directory.c
@@ -28,9 +28,16 @@ Ext4GetUcs2DirentName (
{
CHAR8 Utf8NameBuf[EXT4_NAME_MAX + 1];
UINT16 *Str;
+ UINTN Index;

I *really* do not like UINTN in code that does not deal with buffer addresses and sizes. I'd change it to UINT8, but I'll leave it up to Pedro.

EFI_STATUS Status;

- CopyMem (Utf8NameBuf, Entry->name, Entry->name_len);
+ for (Index = 0; Index < Entry->name_len; ++Index) {
+ if (Entry->name[Index] == '\0') {
+ return EFI_INVALID_PARAMETER;
+ }
+
+ Utf8NameBuf[Index] = Entry->name[Index];
+ }

Utf8NameBuf[Entry->name_len] = '\0';

--
2.39.0

The directory entry name conventions forbid having null-terminator
symbols in its body and can lead to undefined behavior conditions
and crashes

Cc: Marvin Häuser <mhaeuser@...>
Cc: Pedro Falcato <pedro.falcato@...>
Cc: Vitaly Cheptsov <vit9696@...>
Fixes: 89b2bb0db263 ("Ext4Pkg: Fix and clarify handling regarding non-utf8 dir entries")
Signed-off-by: Savva Mitrofanov <savvamtr@...>
---
Features/Ext4Pkg/Ext4Dxe/Directory.c | 9 ++++++++-
1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/Features/Ext4Pkg/Ext4Dxe/Directory.c b/Features/Ext4Pkg/Ext4Dxe/Directory.c
index 0753a20b5377..465749c9b51d 100644
--- a/Features/Ext4Pkg/Ext4Dxe/Directory.c
+++ b/Features/Ext4Pkg/Ext4Dxe/Directory.c
@@ -28,9 +28,16 @@ Ext4GetUcs2DirentName (
{
CHAR8 Utf8NameBuf[EXT4_NAME_MAX + 1];
UINT16 *Str;
+ UINTN Index;
EFI_STATUS Status;

- CopyMem (Utf8NameBuf, Entry->name, Entry->name_len);
+ for (Index = 0; Index < Entry->name_len; ++Index) {
+ if (Entry->name[Index] == '\0') {
+ return EFI_INVALID_PARAMETER;
+ }
+
+ Utf8NameBuf[Index] = Entry->name[Index];
+ }

Utf8NameBuf[Entry->name_len] = '\0';

--
2.39.0