1. Patchew
  2. EDK2
  3. [edk2-devel] [edk2-platforms][PATCH v3 00/11] Ext4Pkg: Code correctness and security improvements
  4. View patch

[edk2-devel] [edk2-platforms][PATCH v3 11/11] Ext4Pkg: Filter out directory entry names containing \0 as invalid

Savva Mitrofanov posted 11 patches 3 years ago
There is a newer version of this series
[edk2-devel] [edk2-platforms][PATCH v3 11/11] Ext4Pkg: Filter out directory entry names containing \0 as invalid
Posted by Savva Mitrofanov 3 years ago 
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@posteo.de>
Cc: Pedro Falcato <pedro.falcato@gmail.com>
Cc: Vitaly Cheptsov <vit9696@protonmail.com>
Fixes: 89b2bb0db263 ("Ext4Pkg: Fix and clarify handling regarding non-utf8 dir entries")
Signed-off-by: Savva Mitrofanov <savvamtr@gmail.com>
---
 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



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#99147): https://edk2.groups.io/g/devel/message/99147
Mute This Topic: https://groups.io/mt/96562701/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] [edk2-platforms][PATCH v3 11/11] Ext4Pkg: Filter out directory entry names containing \0 as invalid
Posted by Marvin Häuser 3 years ago 
On 27. Jan 2023, at 10:29, Savva Mitrofanov <savvamtr@gmail.com> 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@posteo.de>
> Cc: Pedro Falcato <pedro.falcato@gmail.com>
> Cc: Vitaly Cheptsov <vit9696@protonmail.com>
> Fixes: 89b2bb0db263 ("Ext4Pkg: Fix and clarify handling regarding non-utf8 dir entries")
> Signed-off-by: Savva Mitrofanov <savvamtr@gmail.com>
> ---
> 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
> 



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#99149): https://edk2.groups.io/g/devel/message/99149
Mute This Topic: https://groups.io/mt/96562701/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] [edk2-platforms][PATCH v3 11/11] Ext4Pkg: Filter out directory entry names containing \0 as invalid
Posted by Savva Mitrofanov 3 years ago 
It is not so important from my point of view, however, I corrected this in the referenced repository fork.

> On 27 Jan 2023, at 16:04, Marvin Häuser <mhaeuser@posteo.de> wrote:
> 
> On 27. Jan 2023, at 10:29, Savva Mitrofanov <savvamtr@gmail.com> 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@posteo.de>
>> Cc: Pedro Falcato <pedro.falcato@gmail.com>
>> Cc: Vitaly Cheptsov <vit9696@protonmail.com>
>> Fixes: 89b2bb0db263 ("Ext4Pkg: Fix and clarify handling regarding non-utf8 dir entries")
>> Signed-off-by: Savva Mitrofanov <savvamtr@gmail.com>
>> ---
>> 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
>> 
> 



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#99291): https://edk2.groups.io/g/devel/message/99291
Mute This Topic: https://groups.io/mt/96562701/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] [edk2-platforms][PATCH v3 11/11] Ext4Pkg: Filter out directory entry names containing \0 as invalid
Posted by Pedro Falcato 3 years ago 
On Fri, Jan 27, 2023 at 10:04 AM Marvin Häuser <mhaeuser@posteo.de> wrote:
>
> On 27. Jan 2023, at 10:29, Savva Mitrofanov <savvamtr@gmail.com> 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@posteo.de>
> > Cc: Pedro Falcato <pedro.falcato@gmail.com>
> > Cc: Vitaly Cheptsov <vit9696@protonmail.com>
> > Fixes: 89b2bb0db263 ("Ext4Pkg: Fix and clarify handling regarding non-utf8 dir entries")
> > Signed-off-by: Savva Mitrofanov <savvamtr@gmail.com>
> > ---
> > 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@gmail.com>

-- 
Pedro


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#99159): https://edk2.groups.io/g/devel/message/99159
Mute This Topic: https://groups.io/mt/96562701/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] [edk2-platforms][PATCH v3 11/11] Ext4Pkg: Filter out directory entry names containing \0 as invalid
Posted by Marvin Häuser 3 years ago 

> On 27. Jan 2023, at 15:09, Pedro Falcato <pedro.falcato@gmail.com> wrote:
> 
> On Fri, Jan 27, 2023 at 10:04 AM Marvin Häuser <mhaeuser@posteo.de> wrote:
>> 
>> On 27. Jan 2023, at 10:29, Savva Mitrofanov <savvamtr@gmail.com> 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@posteo.de>
>>> Cc: Pedro Falcato <pedro.falcato@gmail.com>
>>> Cc: Vitaly Cheptsov <vit9696@protonmail.com>
>>> Fixes: 89b2bb0db263 ("Ext4Pkg: Fix and clarify handling regarding non-utf8 dir entries")
>>> Signed-off-by: Savva Mitrofanov <savvamtr@gmail.com>
>>> ---
>>> 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@gmail.com>
> 
> -- 
> Pedro



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#99162): https://edk2.groups.io/g/devel/message/99162
Mute This Topic: https://groups.io/mt/96562701/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] [edk2-platforms][PATCH v3 11/11] Ext4Pkg: Filter out directory entry names containing \0 as invalid
Posted by Marvin Häuser 3 years ago 
As Pedro agreed, forgot to add

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

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

> On 27. Jan 2023, at 15:14, Marvin Häuser <mhaeuser@posteo.de> wrote:
> 
> 
> 
>> On 27. Jan 2023, at 15:09, Pedro Falcato <pedro.falcato@gmail.com> wrote:
>> 
>>> On Fri, Jan 27, 2023 at 10:04 AM Marvin Häuser <mhaeuser@posteo.de> wrote:
>>> 
>>> On 27. Jan 2023, at 10:29, Savva Mitrofanov <savvamtr@gmail.com> 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@posteo.de>
>>>> Cc: Pedro Falcato <pedro.falcato@gmail.com>
>>>> Cc: Vitaly Cheptsov <vit9696@protonmail.com>
>>>> Fixes: 89b2bb0db263 ("Ext4Pkg: Fix and clarify handling regarding non-utf8 dir entries")
>>>> Signed-off-by: Savva Mitrofanov <savvamtr@gmail.com>
>>>> ---
>>>> 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@gmail.com>
>> 
>> -- 
>> Pedro
> 



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#99297): https://edk2.groups.io/g/devel/message/99297
Mute This Topic: https://groups.io/mt/96562701/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

Patchew 2.1-devel-b67e4c2

© 2016 - 2026 Red Hat, Inc.