Improve Intel Microcode detection pattern

[platomav]

The current Intel microcode pattern seems to be weak/broad and causes a lot of false positives. This is just one example:

As a suggestion, the one I use at MCExtractor is probably as close to "robust" as possible:

# Intel - HeaderRev 01, Year 1993-2021, Day 01-31, Month 01-12, CPUID xxxxxx00, LoaderRev 00-01, PlatformIDs 000000xx, DataSize xxxxxx00, TotalSize xxxxxx00, Reserved1
pat_icpu = re.compile(br'\x01\x00{3}.{4}(([\x00-\x21]\x20)|([\x93-\x99]\x19))[\x01-\x31][\x01-\x12].{3}\x00.{4}[\x01\x00]\x00{3}.\x00{3}.{3}\x00.{3}\x00{13}', re.DOTALL)

[NikolajSchlej]

With #181 in mind, it becomes a serious parsing issue.

[platomav]

Serious but easily solvable at least by improving the pattern ;)

[NikolajSchlej]

182 and #183 duped here.

[NikolajSchlej]

Should be fixed in 95c8381, please verify and close, if so.

[platomav]

Alright, my two samples do show properly now. However, I have some thoughts:

• Unfortunately you cannot check if DataSize and TotalSize are 1024 multiples. For DataSize, it is not POR and in fact fails at most microcodes. TotalSize should work (POR) but alas, there are examples from 2007 and 2017 in which Intel has made mistakes and it is not 1K aligned. For example, microcode cpu906E9_plat2A_ver0000005E_2017-04-06_PRD_89811134 has a TotalSize of 0x17220 which has a non zero modulo 1024.
• I think checking/allowing Years 2030+ is overkill considering that these are at least 11 years away and at that point who knows what might have changed. In the meantime though, we might be getting false positives. Up until 2029 is good enough IMHO.

The first point by itself should be skipping a lot of valid microcodes at the current commit, based on my experience. If you feel like it will make false positives appear again, here is a list of some extra things I do in MCE to avoid them:

• The last 3 bytes of Header Revision are always 0.
• The last 3 bytes of Loader Revision are always 0.
• The MSByte of DataSize and Total Size is always 0 because we won't see >= 16MB microcodes any time soon. Maximum size so far is 115KB.
• The MSByte of CPUID is always 0 because Intel won't need such a big number any time soon. Current is 000Axxxx.

[NikolajSchlej]

DataSize is 1024 multiple

The code says it's a 4 multiple, which is a spec requirement I haven't ever seen breached. If you find such a file, please attach it.

TotalSize is 1024 multiple

This is a specification requirement, and if Intel failed to follow it, it's sad news.

• [x] Remove check for TotalSize % 1024 == 0

checking/allowing Years 2030+ is overkill

It is a bit of an overkill, sure, but it only adds 10/20 possible values to the other 65k+ impossible ones, so it won't affect the chance for a false positives too much, while providing some future-proofing for free.

The last 3 bytes of Header Revision are always 0.

• [x] Add check Revision < 0x100

The last 3 bytes of Loader Revision are always 0.

• [x] Add check LoaderRevision < 0x100

The MSByte of DataSize, TotalSize and CPUID is always 0

• [x] Add check DataSize, TotalSize < 0x1000000

I don't want to touch CPUID yet, because Intel might start using that upper bit tomorrow without announce.

[platomav]

The code says it's a 4 multiple, which is a spec requirement I haven't ever seen breached

Yes modulo 4 returns 0 at both DataSize and TotalSize. It is a very weak check compared to 1024 but better than nothing I suppose.

I don't want to touch CPUID yet, because Intel might start using that upper bit tomorrow without announce

Ok, it is up to each tool maintainer. The thing with the modding/research community is that it always reacts to what is currently available so future proofing has severe limits. There are so many things that Intel could do, such as upping the Header & Loader revision after 24 years or starting to use the reserve fields all of a sudden. All such cases (CPUID, Revision, Reserved etc) will lead to the same result: no detection of a given microcode. So my own ideology is to build for what's available and future proof moderately. Anyway, UT should be fine with the recent improvements. ;)

[NikolajSchlej]

Please verify with 64e1aa1.

[vit9696]

@NikolajSchlej I am not sure it is correct to expect MCU revision to be less than 0x100[1]. If you check the latest pdf with MCU revisions [2], they have many values far larger. Unless these are different numbers, I am afraid the check should be removed.

[1] https://github.com/LongSoft/UEFITool/commit/64e1aa18b87f28596a09747f86c454b2179ee4a3#diff-c600a9cef5af10105ff45bab78599a49R1285

[2] https://www.intel.com/content/dam/www/public/us/en/documents/corporate-information/SA00233-microcode-update-guidance.pdf

[NikolajSchlej]

This came from the fact that we call different revisions a different thing, the "Header Revision" you meant is called "Version" in UEFITool, and that one is hardcoded to 1 anyway. I'll remove that line now.

[NikolajSchlej]

3507698 should fix that now.

[platomav]

Indeed, that applies to Header Revision and Loader Revision for the MSByte check, not (bare) Revision/Version.

One small issue remains. The check must be to ignore the microcode if TotalSize < DataSize but not <= because from 1995-2006 they were the same since the "Extra" undocumented header did not yet exist. Also useful for the microcodes with 0 sizes (which means 0x800 per Intel POR). The current commit ignores all 1995-2006 microcodes based on my checks in MCE.

For reference, list of 5/1921 microcodes which are not 1K TotalSize aligned:

cpu106C0_plat01_ver00000005_2007-06-12_PRD_86074EFC cpu106C0_plat01_ver00000006_2007-07-27_PRD_3829B920 cpu106C0_plat01_ver00000007_2007-08-24_PRD_923CDFA3 cpu106C1_plat01_ver00000103_2007-08-23_PRD_8487EA26 cpu906E9_plat2A_ver0000005E_2017-04-06_PRD_89811134

[NikolajSchlej]

Fixed in 8bddbe7.

[platomav]

Built UT 8bddbe7, validated my initial and a few more varying samples and everything seems to be in order. This issue is now fixed! :)

[vit9696]

In fact, it still does not make sense to me. Both UpdateVersion and LoaderVersion are required to be set to 1. This is public knowledge from EDK II [1].

In my opinion, we should change this block:

    if (ucodeHeader->LoaderRevision > 0xFF) {
        return FALSE;
    }

to

    if (ucodeHeader->UpdateVersion != 1 || ucodeHeader->LoaderRevision != 1) {
        return FALSE;
    }

And replace structure members to UpdateVersion and LoaderVersion accordingly to avoid that mess with revision.

[1] https://github.com/tianocore/edk2/blob/edk2-stable201808/IntelFsp2Pkg/FspSecCore/Ia32/FspApiEntryT.nasm#L214-L220

[vit9696]

Should be properly resolved as of https://github.com/LongSoft/UEFITool/commit/967375243cb14d977a1c1660d6bda29538583255.

[platomav]

Personally I disagree with both changes.

• The current structure names make no sense and are misleading. The first dword should be called Header Revision (HeaderRevision) and the second Update Revision (UpdateRevision) or just Revision if you want to imply that it relates to the patch data, but certainly not the other way around.
• As has been discussed above, UEFITool (and any tool that deals with micrcodes) needs to have some future proofing. For example, allowing values 1 to 10 for HeaderRevision and LoaderRevision does give some leeway to UEFITool without any meaningful risk of false positives. The extremes, such as only 1 or all values from 1 to 0xFFFFFFFF, can be dangerous. The previous commits from Nikolaj allowed a (generous) leeway of 255 revisions while making sure that the other 3 bytes must not be populated. As he said above, these can be realistically overkill but it's better not to cut it too close.

[vit9696]

@platomav,

Regarding headers you will most likely have to accept the reality written by Intel. It makes not much sense to me as well, but it has been like this for years:

• First DWORD is UCODE format. It has always been named UpdateVersion or just Version. Always 1.
• Second DWORD is UCODE revision. That's the thing read from MSR_IA32_BIOS_SIGN_ID (8Bh), and it actually represents microcode revision as present in all known documents and operating systems, not just Intel. This value is arbitrary, e.g. Xeon CPUs may have E00000Dh.
• Sixth DWORD is UCODe loader format. This value has always been named LoaderVersion or similarly. Always 1.

The logic is very simple. Internal format detail is called versions, external format detail is called revisions.

As for values, given how much time it stayed like that, it is safe to assume that the change in the numbering will result in severe format changes with potential update of the fields. In my opinion, it is unreasonable to try to guess what Intel does before at least one change happens.

[NikolajSchlej]

Let's find a microcode file that doesn't follow that stricter assumption and fix the tool for it. I tried to to relax that check a bit, because it's actually not that important (date in BCD will rule most of the false-positives out), but if @vit9696 wants it strict, I'm ok with it.

[platomav]

From Intel 64 and IA-32 Architectures Software Developer’s Manual Vol 3A, Ch 9.11.1:

The logic is very simple. Internal format detail is called versions, external format detail is called revisions

Sure but most people don't know the difference and tend to use either one. Even Intel as you can see from the "Loader Revision" above. Either way, that's a technicality. My objection was with the term "Update" instead of "Header" at the first and second dword. Even if Intel was calling it "Update Version" instead of "Header Version" (which they're not at the official documentation for MCU), it doesn't mean that we should use the wrong term as well.

This value is arbitrary, e.g. Xeon CPUs may have E00000Dh

No, not really, just not documented officially. For example, the first bit (signed dword) shows whether the microcode is Production or Pre-Production. I've also noticed that 2 bytes are the actual revision and the other two are "flags" or similar.

In my opinion, it is unreasonable to try to guess what Intel does before at least one change happens

https://github.com/LongSoft/UEFITool/issues/180#issuecomment-528491981

[platomav]

Anyway, naming things aside, the current code works so it's just fine leaving it like that. Thanks for the feedback vit. ;)

[vit9696]

Intel SDM and Intel Reference Code (which is now largely open source) are written by completely different people, and the former is, frankly said, full of out of date or misunderstood information. For example, not so long ago I faced I had to go through CPU frequency detection on Intel Xeon W and Xeon Scalable, and trust me, SDM helped least. You can also find many good examples with register names being named differently in SDM and UEFI code.

I did a recheck on the names, and it is terribly messed up in the code as well. Perhaps, what we could do here is to stick to authentic Intel source file with the latest copyright year. I will probably go with Microcode.h from EDK II[1] and update the names once again tomorrow. For the good to the discussion it mostly matches your preferred naming, for the bad.

As for versioning, currently all firmwares verify first and sixth dword against 1 very close to reset vector, so I will probably stick to the current code. What Intel does from there on is unlikely to be a trivial change, and I would rather refrain from guessing, as they could even put some 0xF0000001 "extended" version and feel happy.

[1] https://github.com/tianocore/edk2/blob/edk2-stable201908/MdePkg/Include/Register/Intel/Microcode.h#L52