Incorrect parsing of some firmware

[yeggor]

Hi. We found some firmwares that is not correctly handled by UEFITool (CC: @xorpse). There are two cases:

• the firmware is not processed correctly because of this condition (Lenovo firmwares from attached zip): https://github.com/LongSoft/UEFITool/blob/6e5cc3bccc16f36f1edca0dffd18549fcc5c22d5/common/ffsparser.cpp#L1493 below is the possible fix (if the size retrieved by the parser is 0, we still allow you to check the rest of the firmware):

  if (fileSize == 0) {
    fileSize = 0xFFFFFFFF;
    msg(usprintf("%s: file header parsing failed with invalid size", __FUNCTION__), index);
  }

• part of the firmware with modules goes to the BpdtStore (BlueField firmwares from attached zip): https://github.com/LongSoft/UEFITool/blob/6e5cc3bccc16f36f1edca0dffd18549fcc5c22d5/common/ffsparser.cpp#L1377

uefitool_tests.zip

[NikolajSchlej]

Tolerating zero-sized files will certainly be out-of-spec, but we already allow some OOS shenanigans. Replacing the size with 0xFFFFFFFF, however, is certainly not a good idea, because it will inevitably introduce overflows and memory corruptions where none were possible before. I think it's ultimately better to threat any file with fileSize < sizeof(FFS_FILE_HEADER) as if they have fileSize == sizeof(FFS_FILE_HEADER).

As for the other files, that bug is known and indeed needs a workaround.

[NikolajSchlej]

Investigated some more, this is the result: https://twitter.com/NikolajSchlej/status/1578304283803783168 TL;DR: FFSv3 large file inside an FFSv2 volume. Asked the spec wording to be updated, will add a workaround into getFileSize.

[NikolajSchlej]

Not even an FFSv3 large file, but Lenovo invention that uses uint32_t instead of spec-compliant uint64_t. I will detect that quirk by a file GUID, because so far that's enough, but if they double down on using that new format in otherwise spec-compliant FFSv2 volumes, we'll need a better workaround.

[yeggor]

Thanks for the investigation. Looks really interesting.

Replacing the size with 0xFFFFFFFF, however, is certainly not a good idea

My bad, maximum expected size can be 0xFFFFFF.

[NikolajSchlej]

Lenovo issue should now be fixed by this commit: https://github.com/LongSoft/UEFITool/commit/acc913769bf473fdeb4485c480a5a6eee8c43a31 I decided to detect the quirk based on a combination of FFSv2 Rev2 volumes (where FFS_ATTRIB_LARGE_FILE is defined a reserved unset bit) and that exact bit being set (because Lenovo did set it after all). Could end up wrong, but works well enough for now.

[yeggor]

Thank you. It works fine for me.

[NikolajSchlej]

Alright, time for a proper analysis of the BlueField ones. Those don't parse properly because they are all mistaken for having BPDT partition table with 20k+ entries that span way past the end of the file. Can be fixed by better separation of valid BPDT partition table headers from random data resembling them.

[NikolajSchlej]

@platomav, need a bit of your expertise here. This is only applicable to IFWI 2.0 cases, where we have to detect BPDT partition table headers in raw images. So far I've tried to detect them based on Signature being BPDT_GREEN_SIGNATURE or BPDT_YELLOW_SIGNATURE, and Version being 1, but it turns out that's not enough (this is #180 all over again, just for a different kind of possible raw area entry). Question is: have you seen IfwiVersion to be anything non-zero in any IFWI 2.0 image? Everything I've tried (about 20 images from various places) has it 0, so I'm contemplating on adding that to the heuristics. Another possibility is to check the sanity of NumEntries fields and bail if there are >= 50 or some other sane value.

[platomav]

@NikolajSchlej I had many issues over the years with IFWI detection at ME Analyzer but in the end I settled with this regex which covers all use cases and has not given me FP or FN (AFAIK):

# Intel Engine/Graphics firmware Boot Partition Descriptor Table pattern (BPDT)
bpdt_pat = re.compile(br'\xAA\x55[\x00\xAA]\x00.\x00[\x01\x02][\x00\x01].{16}(.\x00.\x00.{3}\x00.{3}\x00){3}', re.DOTALL)

Basically:

• Use characteristics from the BPDT Header

  • 4/4 bytes for Green|Yellow possible signatures
  • 1/2 bytes for DescCount, we won't see more than 256 entries
  • 0x1 byte for BPDTVersion (value 1 or 2)
  • 0x1 byte for BPDTFlags (value 0 or 1, the latter for data redundancy)
  • Skip the rest 0x16 (Checksum, IFWIVersion, FITVersion)

• Use characteristics from the first BPDT Entries (e.g. first 2-3)

  • Only 1/2 bytes for Type, there are way less than 256
  • Only 1/2 bytes for Flags, only 4 bits were defined
  • Only 3/4 bytes for Offset, we won't see entries starting from Header + 16MB
  • Only 3/4 bytes for Size, we won't see entries larger than 16MB

Regarding IFWIVersion: the name of that field is unfortunate. It actually stands for "Unique mark from build server". Vendors may sometimes add whatever they want there. I've seen some of them using the entire uint32 for a build marking (e.g. YYYYMMDD) or something else specific to them. For example, I found this sample in my library with a quick search:

Here are some resources from MEA which I think will be helpful for your (annoying, I know) quest:

• https://github.com/platomav/MEAnalyzer/blob/99fe9ebda23316c2e9cad5ade64714d4d564898c/MEA.py#L631
• https://github.com/platomav/MEAnalyzer/blob/99fe9ebda23316c2e9cad5ade64714d4d564898c/MEA.py#L674
• https://github.com/platomav/MEAnalyzer/blob/99fe9ebda23316c2e9cad5ade64714d4d564898c/MEA.py#L732
• https://github.com/platomav/MEAnalyzer/blob/99fe9ebda23316c2e9cad5ade64714d4d564898c/MEA.py#L10595
• https://github.com/platomav/MEAnalyzer/blob/99fe9ebda23316c2e9cad5ade64714d4d564898c/MEA.py#L10846

[NikolajSchlej]

Thanks a ton, @platomav.

Decided not to parse anything out of the entries (just yet), because current improvements done in https://github.com/LongSoft/UEFITool/commit/7e5e02b4b4ae93bbd488a6596a9474afe31c78fa are already enough to fix the BlueField issues we see here. Will return to it if we find images that will require even more fine-grained detection.

@yeggor, I've verified that everything attached is now being parsed as expected, so I'm closing this as complete. Please open another issue if you seen something else like this.

[yeggor]

Checked, looks good. Thanks a lot!