Regression in parsing FFS volume header with AppleCRC32 and AppleUsedSpace non-standard entries in A66 and A67

[kocoman2]

In the past with the old version (59), I was able to extract the vbios (for osx) and the vbt (for ms windows) of the MBP102_0106_B0A_LOCKED.scap file (dl at https://github.com/gdbinit/firmware_vault/tree/master/EFI/MacBookPro) (for put in mbp101 macbookpro10,1 with disabled nvidia and enabled igpu), but with the current version 67 (don't know when it started..) it can't do it anymore.

for example if u use the old version like NE 59 and search for "bios_data_block" u can find it, but its not found when u use 67.. the 77ad7fdb-xxxx still exists but it does not expand the Volume Image Section like how the 59 does before..

[NikolajSchlej]

Most likely a regression after this commit: https://github.com/LongSoft/UEFITool/commit/d9e1fe58599f0abb83786be58b64f52c8dbe50f0 CC @yeggor

[NikolajSchlej]

Yep, A65 works as expected, will be fixed in A68.

[kocoman2]

when is the A68 cycle release ?

[NikolajSchlej]

In 2-3 weeks.

[NikolajSchlej]

@yeggor, could you please provide clarification for this check:

    // volumeHeader->ExtHeaderOffset should be aligned to 4 bytes
    if (volumeHeader->ExtHeaderOffset % 4) {
        msg(usprintf("%s: ExtHeaderOffset %04Xh (%hu) is not aligned by 4 bytes", __FUNCTION__, volumeHeader->ExtHeaderOffset, volumeHeader->ExtHeaderOffset));
        return U_INVALID_VOLUME;
    }

It is currently buggy as is because in FFSv2 Revision 1 volumes that field was Reserved, but can contain any bytes, so the field only has meaning after checking that Resivision is greater than 1, as it is done is all other places in the same function.

However, I don't really see a good reason for this check, i.e. is that "must be aligned to 4" a PI spec requirement?

[yeggor]

@yeggor, could you please provide clarification for this check:

    // volumeHeader->ExtHeaderOffset should be aligned to 4 bytes
    if (volumeHeader->ExtHeaderOffset % 4) {
        msg(usprintf("%s: ExtHeaderOffset %04Xh (%hu) is not aligned by 4 bytes", __FUNCTION__, volumeHeader->ExtHeaderOffset, volumeHeader->ExtHeaderOffset));
        return U_INVALID_VOLUME;
    }

It is currently buggy as is because in FFSv2 Revision 1 volumes that field was Reserved, but can contain any bytes, so the field only has meaning after checking that Resivision is greater than 1, as it is done is all other places in the same function.

However, I don't really see a good reason for this check, i.e. is that "must be aligned to 4" a PI spec requirement?

hi, the check was added to get rid of the following libFuzzer's UndefinedBehavior finding:

$ ./ffsparser_fuzzer crash-3cc848b650549660a77d498c7a6b50b756ca9b2f                                                                                         new_engine
ffsparser_fuzzer(68650,0x1fdd95e00) malloc: nano zone abandoned due to inability to reserve vm space.
INFO: Running with entropic power schedule (0xFF, 100).
INFO: Seed: 3844664712
INFO: Loaded 1 modules   (45799 inline 8-bit counters): 45799 [0x101318440, 0x101323727),
INFO: Loaded 1 PC tables (45799 PCs): 45799 [0x101323728,0x1013d6598),
./ffsparser_fuzzer: Running 1 inputs 1 time(s) each.
Running: crash-3cc848b650549660a77d498c7a6b50b756ca9b2f
/Users/yv/github/uefitool/common/ffsparser.cpp:1168:120: runtime error: reference binding to misaligned address 0x60b000000261 for type 'const EFI_GUID' (aka 'const EFI_GUID_'), which requires 4 byte alignment
0x60b000000261: note: pointer points here
 01 00 00  01 aa 55 00 00 00 00 01  00 00 00 01 d7 d7 d7 01  6c 00 00 00 00 00 00 00  5f 46 56 48 11
              ^
SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior /Users/yv/github/uefitool/common/ffsparser.cpp:1168:120 in
==68650== ERROR: libFuzzer: deadly signal
    #0 0x1019bfea4 in __sanitizer_print_stack_trace+0x28 (libclang_rt.asan_osx_dynamic.dylib:arm64+0x5bea4) (BuildId: 4947f3677e4435f39b5765e7dbc19bf732000000200000000100000000000b00)
    #1 0x101221140 in fuzzer::PrintStackTrace() FuzzerUtil.cpp:210
    #2 0x101203600 in fuzzer::Fuzzer::CrashCallback() FuzzerLoop.cpp:233
    #3 0x1a2deea20 in _sigtramp+0x34 (libsystem_platform.dylib:arm64+0x3a20) (BuildId: f80c6971c08031f5ab6ebe01311154af32000000200000000100000000040d00)
    #4 0x92438001a2dbfc24  (<unknown module>)
    #5 0x256e8001a2ccdae4  (<unknown module>)
    #6 0x212b0001019d89b4  (<unknown module>)
    #7 0x1019d8120 in __sanitizer::Die()+0xcc (libclang_rt.asan_osx_dynamic.dylib:arm64+0x74120) (BuildId: 4947f3677e4435f39b5765e7dbc19bf732000000200000000100000000000b00)
    #8 0x1019ea9d4 in __ubsan_handle_type_mismatch_v1_abort+0x24 (libclang_rt.asan_osx_dynamic.dylib:arm64+0x869d4) (BuildId: 4947f3677e4435f39b5765e7dbc19bf732000000200000000100000000000b00)
    #9 0x100f8b3a8 in FfsParser::parseVolumeHeader(UByteArray const&, unsigned int, UModelIndex const&, UModelIndex&) ffsparser.cpp:1168
    #10 0x100f7be08 in FfsParser::parseRawArea(UModelIndex const&) ffsparser.cpp:915
    #11 0x100f7a060 in FfsParser::parseGenericImage(UByteArray const&, unsigned int, UModelIndex const&, UModelIndex&) ffsparser.cpp:139
    #12 0x100f691b0 in FfsParser::performFirstPass(UByteArray const&, UModelIndex&) ffsparser.cpp:124
    #13 0x100f687d4 in FfsParser::parse(UByteArray const&) ffsparser.cpp:92
    #14 0x100ed081c in LLVMFuzzerTestOneInput ffsparser_fuzzer.cpp:28
    #15 0x101204a4c in fuzzer::Fuzzer::ExecuteCallback(unsigned char const*, unsigned long) FuzzerLoop.cpp:617
    #16 0x1011f0b70 in fuzzer::RunOneTest(fuzzer::Fuzzer*, char const*, unsigned long) FuzzerDriver.cpp:324
    #17 0x1011f5e18 in fuzzer::FuzzerDriver(int*, char***, int (*)(unsigned char const*, unsigned long)) FuzzerDriver.cpp:860
    #18 0x1012224b8 in main FuzzerMain.cpp:20
    #19 0x1a2a67f24  (<unknown module>)
    #20 0xf5567ffffffffffc  (<unknown module>)

NOTE: libFuzzer has rudimentary signal handlers.
      Combine libFuzzer with AddressSanitizer or similar for better crash reports.
SUMMARY: libFuzzer: deadly signal

But I realized now that I overdid it

[NikolajSchlej]

The whole alignment of GUID type is unfortunate, because it's definitely not aligned to anything in the UEFI image, being raw 16 bytes, The fact it's defined that U32-U16-U16-U8x8 only means it needs to be 4-byte aligned in C++, but the whole UEFI is written in C where no such requirement exists. Closing this as resolved.