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

[Marvin Häuser]

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

(Still two comments for next time)

> On 12. Jan 2023, at 00:59, Pedro Falcato <pedro.falcato at 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 at gmail.com>
> Fixes: 45e37d8533ca8 ("Ext4Pkg: Hide "." and ".." entries from Read() callers.")
> Cc: Marvin Häuser <mhaeuser at posteo.de>
> Signed-off-by: Pedro Falcato <pedro.falcato at 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
> 



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#98513): https://edk2.groups.io/g/devel/message/98513
Mute This Topic: https://groups.io/mt/96212631/1813853
Group Owner: devel+owner at edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [edk2-devel-archive at redhat.com]
-=-=-=-=-=-=-=-=-=-=-=-


-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://listman.redhat.com/archives/edk2-devel-archive/attachments/20230114/e0086ffd/attachment-0001.htm>