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

[Marvin Häuser]

As Pedro agreed, forgot to add

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

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

> On 27. Jan 2023, at 15:14, Marvin Häuser <mhaeuser at posteo.de> wrote:
> 
> 
> 
>> On 27. Jan 2023, at 15:09, Pedro Falcato <pedro.falcato at gmail.com> wrote:
>> 
>>> On Fri, Jan 27, 2023 at 10:04 AM Marvin Häuser <mhaeuser at posteo.de> wrote:
>>> 
>>> On 27. Jan 2023, at 10:29, Savva Mitrofanov <savvamtr at 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 at posteo.de>
>>>> Cc: Pedro Falcato <pedro.falcato at gmail.com>
>>>> Cc: Vitaly Cheptsov <vit9696 at protonmail.com>
>>>> Fixes: 89b2bb0db263 ("Ext4Pkg: Fix and clarify handling regarding non-utf8 dir entries")
>>>> Signed-off-by: Savva Mitrofanov <savvamtr at 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 at 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/1813853
Group Owner: devel+owner at edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [edk2-devel-archive at redhat.com]
-=-=-=-=-=-=-=-=-=-=-=-