Bug in Ext4Pkg: ASSERT() triggered when processing a corrupted superblock

[stokescat]

During fuzzing of Ext4Dxe, an ASSERT() in Ext4ReadFile() was triggered while processing a corrupted filesystem image. The crash is reproducible with a libFuzzer-generated input. Stack trace:

[ext4] Error ==5562== ERROR: libFuzzer: deadly signal
    #0 0x0000004e6b65 in __sanitizer_print_stack_trace (/home/pavel/fuzz/OpenCorePkg/Utilities/TestExt4Dxe/TestExt4Dxe+0x4e6b65) (BuildId: bb76deb00b707c5b55be5fbebc76f05b7624f167)
    #1 0x00000043aa0c in fuzzer::PrintStackTrace() (/home/pavel/fuzz/OpenCorePkg/Utilities/TestExt4Dxe/TestExt4Dxe+0x43aa0c) (BuildId: bb76deb00b707c5b55be5fbebc76f05b7624f167)
    #2 0x00000041ec77 in fuzzer::Fuzzer::CrashCallback() (/home/pavel/fuzz/OpenCorePkg/Utilities/TestExt4Dxe/TestExt4Dxe+0x41ec77) (BuildId: bb76deb00b707c5b55be5fbebc76f05b7624f167)
    #3 0x7f6c0ac2744f  (/lib64/libc.so.6+0x1a44f) (BuildId: dae6ae6929d69dca842288f5300af5a33d1bdcd7)
    #4 0x7f6c0ac807d3 in __pthread_kill_implementation (/lib64/libc.so.6+0x737d3) (BuildId: dae6ae6929d69dca842288f5300af5a33d1bdcd7)
    #5 0x7f6c0ac2739d in gsignal (/lib64/libc.so.6+0x1a39d) (BuildId: dae6ae6929d69dca842288f5300af5a33d1bdcd7)
    #6 0x7f6c0ac0e901 in abort (/lib64/libc.so.6+0x1901) (BuildId: dae6ae6929d69dca842288f5300af5a33d1bdcd7)
    #7 0x0000006a278d in CpuBreakpoint /home/pavel/fuzz/OpenCorePkg/Utilities/TestExt4Dxe/../../User/Library/UserMisc.c:17:3
    #8 0x0000005b0c88 in DebugAssert /home/pavel/fuzz/audk/MdePkg/Library/UefiDebugLibConOut/DebugLib.c:232:7
    #9 0x0000005773f2 in Ext4ReadFile /home/pavel/fuzz/audk/Ext4Pkg/Ext4Dxe/File.c:521:4
    #10 0x000000535613 in FSFuzzTest1 /home/pavel/fuzz/OpenCorePkg/Utilities/TestExt4Dxe/FsFuzzTest.c:378:11
    #11 0x00000053220c in FsFuzzTest /home/pavel/fuzz/OpenCorePkg/Utilities/TestExt4Dxe/FsFuzzTest.c:853:2
    #12 0x00000051dde8 in TestExt4Dxe /home/pavel/fuzz/OpenCorePkg/Utilities/TestExt4Dxe/TestExt4Dxe.c:181:3
    #13 0x00000051d7f7 in LLVMFuzzerTestOneInput /home/pavel/fuzz/OpenCorePkg/Utilities/TestExt4Dxe/TestExt4Dxe.c:222:3
    #14 0x00000042074a in fuzzer::Fuzzer::ExecuteCallback(unsigned char const*, unsigned long) (/home/pavel/fuzz/OpenCorePkg/Utilities/TestExt4Dxe/TestExt4Dxe+0x42074a) (BuildId: bb76deb00b707c5b55be5fbebc76f05b7624f167)
    #15 0x00000041fa49 in fuzzer::Fuzzer::RunOne(unsigned char const*, unsigned long, bool, fuzzer::InputInfo*, bool, bool*) (/home/pavel/fuzz/OpenCorePkg/Utilities/TestExt4Dxe/TestExt4Dxe+0x41fa49) (BuildId: bb76deb00b707c5b55be5fbebc76f05b7624f167)
    #16 0x0000004217b5 in fuzzer::Fuzzer::MutateAndTestOne() (/home/pavel/fuzz/OpenCorePkg/Utilities/TestExt4Dxe/TestExt4Dxe+0x4217b5) (BuildId: bb76deb00b707c5b55be5fbebc76f05b7624f167)
    #17 0x0000004222c5 in fuzzer::Fuzzer::Loop(std::vector<fuzzer::SizedFile, std::allocator<fuzzer::SizedFile>>&) (/home/pavel/fuzz/OpenCorePkg/Utilities/TestExt4Dxe/TestExt4Dxe+0x4222c5) (BuildId: bb76deb00b707c5b55be5fbebc76f05b7624f167)
    #18 0x00000040e2d2 in fuzzer::FuzzerDriver(int*, char***, int (*)(unsigned char const*, unsigned long)) (/home/pavel/fuzz/OpenCorePkg/Utilities/TestExt4Dxe/TestExt4Dxe+0x40e2d2) (BuildId: bb76deb00b707c5b55be5fbebc76f05b7624f167)
    #19 0x00000043b616 in main (/home/pavel/fuzz/OpenCorePkg/Utilities/TestExt4Dxe/TestExt4Dxe+0x43b616) (BuildId: bb76deb00b707c5b55be5fbebc76f05b7624f167)
    #20 0x7f6c0ac10447 in __libc_start_call_main (/lib64/libc.so.6+0x3447) (BuildId: dae6ae6929d69dca842288f5300af5a33d1bdcd7)
    #21 0x7f6c0ac1050a in __libc_start_main@GLIBC_2.2.5 (/lib64/libc.so.6+0x350a) (BuildId: dae6ae6929d69dca842288f5300af5a33d1bdcd7)
    #22 0x000000401ac4 in _start (/home/pavel/fuzz/OpenCorePkg/Utilities/TestExt4Dxe/TestExt4Dxe+0x401ac4) (BuildId: bb76deb00b707c5b55be5fbebc76f05b7624f167)

NOTE: libFuzzer has rudimentary signal handlers.
      Combine libFuzzer with AddressSanitizer or similar for better crash reports.
SUMMARY: libFuzzer: deadly signal
MS: 2 InsertRepeatedBytes-CMP- DE: "\000\000\000\000"-; base unit: 2d7c42beb17edb545b7bc029810763fbf6f623ba
artifact_prefix='./'; Test unit written to ./crash-4fcde7f49db84ce692a16f5bdc1f04f2b528546d

Frame #9 points to the ASSERT() in Ext4ReadFile().

Root cause:
In the corrupted image, the superblock field s_rev_level is set to 0, so Ext4OpenSuperblock() treats the filesystem as GOOD_OLD_REV (ext2). However, the rest of the superblock fields correspond to ext4, including s_inode_size=256.

In the GOOD_OLD_REV branch, the driver forcibly sets Partition->InodeSize=EXT4_GOOD_OLD_INODE_SIZE (128) and ignores s_inode_size from the superblock. As a result, incorrect inode table offsets are used later, invalid inode data is read, and this leads to ASSERT() in Ext4ReadFile().

[mikebeaton]

The GOOD_OLD_REV branch seems to be correct, given its (broken) input. At the ASSERT, we should probably just return EFI_DEVICE_ERROR if the file is not openable, instead? It looks as if there may be various non-handled ASSERTs in this driver.