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

["Marvin Häuser" <mhaeuser@posteo.de>]

[-- 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 --]