EmbeddedPkg: Add option to disable EFI Memory Attribute Protocol

[mariobalanica]

Description

Introduce a driver that allows disabling this protocol via an HII option. Default is enabled, which can also be overridden at build time by changing gEmbeddedTokenSpaceGuid.PcdMemoryAttributeEnabledDefault.

Some Linux distros (e.g. CentOS Stream 9) haven't updated the affected OS loader versions, so it would be better to provide users with a workaround in the meantime rather than breaking compatibility.

• [ ] Breaking change?
  • Breaking change - Will this cause a break in build or boot behavior?
  • Examples: Add a new library class or move a module to a different repo.
• [ ] Impacts security?
  • Security - Does the change have a direct security impact?
  • Examples: Crypto algorithm change or buffer overflow fix.
• [ ] Includes tests?
  • Tests - Does the change include any explicit test code?
  • Examples: Unit tests or integration tests.

[mariobalanica]

cc @hrw

[hrw]

Calling maintainers: @leiflindholm @ardbiesheuvel @changab

This code exists in EDK2 forks already and it would be nice to have it upstream.

[leiflindholm]

CI is failing because of non-7-bit characters in Mario's surname in the header copyright statement. Do we have a tag (or label) for ignoring ECC when it's being silly? @mdkinney @makubacki

[makubacki]

CI is failing because of non-7-bit characters in Mario's surname in the header copyright statement. Do we have a tag (or label) for ignoring ECC when it's being silly? @mdkinney @makubacki

I don't use ECC outside edk2 and I'm not very familiar with it, but I think we'd need a more permanent solution.

The EDK II C Coding Standards Specification Section 5.1.3 states: "Files may only contain the ASCII characters 0x0A, 0x0D, and 0x20 through 0x7E." This addition contains 0x103. I don't know the exact tool/compatibility problems that statement was defined to address in this project, but the first question would be, does it allow an exception?

• If universally allowed now, update the spec and ECC.
• If allowed under certain conditions, determine if the existing mechanisms using an ECC exception (MdePkg.ci.yaml has an example) is sufficient for error code 1008 and/or these files or a new heuristic such as allowing additional characters in comments, etc. is needed.
• If not allowed at all, then the file modification would need to be updated.

These paths would allow the same result to be obtained when running ECC against the code in local and server CI.

@mdkinney would have more background on the context for the coding standard direction and ECC specific guidance.

[mariobalanica]

I can trim my last name if that's a blocker, but I'm probably not going to be the last person who runs into this. It would be nice to lessen the rule to allow non-ASCII chars, at least in comments.

[leiflindholm]

@mariobalanica indeed. We accept individual contributors and it would be beyond silly to ask people to misspell their own names for a coding style. (There could also be interesting legal implications surrounding misrepresenting one's own name for the purpose of claiming copyright.)

I think a reasonable relaxation would be something like "in comments, for proper nouns".

However, in many cases, what gets caught out by this test is things like prettified quotation or non-standard iso8859 special characters, which are good to catch, even in comments. So ECC exception would probably be preferable to new heuristic.

The policy conversation should probably be moved to the mailing list, but @mdkinney, would you object to me to adding an ECC exception for "Bălănică" for EmbeddedPkg for now?

[makubacki]

The policy conversation should probably be moved to the mailing list, but @mdkinney, would you object to me to adding an ECC exception for "Bălănică" for EmbeddedPkg for now?

Since I'm hosting the Tianocore Tools & CI meeting later today, I've added this as an agenda item there to help make progress on a resolution - Tools and CI Meeting - Date 7/1 - #5843.

[leiflindholm]

Ah, it's fireworks week in the US. I'll submit the exception for now, and we can discuss when everyone's had time to recover. (My preference remains an override label though.)

[ardbiesheuvel]

Calling maintainers: @leiflindholm @ardbiesheuvel @changab

This code exists in EDK2 forks already and it would be nice to have it upstream.

I don't disagree but I am a bit disappointed.

Could we at least add a bit more context? I.e., that this is only needed for shim on arm64, which rolled out an untested version calling the EFI memory attributes protocol incorrectly, and that we are stuck with that version of shim because MS refuses to sign shim for x86 while the distros clean up the mess wrt NX support in the various loaders. IOW, nothing is stopping the distros from fixing shim on arm64 (given that it is not signed by MS anyway), but for unspecified reasons, they prefer to keep distributing a known broken version.

As you might be able to glean from the above, I am struggling to muster any sympathy and help out here. The distros are in a mess entirely of their own making, infecting the pristine arm64 ecosystem with a pile of hacks that are only needed for x86 PCs built by vendors that care only about the Windows sticker, and couldn't care less whether or not their EFI implementation is actually compliant.

So the fix is not to use shim. Shim shouldn't exist, and there was never a good reason to rely on shim on arm64.

[mariobalanica]

It's indeed tempting to just let OS vendors deal with it. However, I also believe that firmware shouldn't break OS booting overnight, even though the root issue is somewhere else. The burden is ultimately placed on users, which now have to spend time looking up the problem, ending up just downgrading to older firmware.

[makubacki]

Ah, it's fireworks week in the US. I'll submit the exception for now, and we can discuss when everyone's had time to recover. (My preference remains an override label though.)

That seems reasonable. Attendance was too light to make a decision.

[ardbiesheuvel]

Please document that the need to uninstall the EFI memory attributes protocol exists only on arm64, given that this is the only arch in EDK2 that implements it, and that the only known reason for this is shim, of which a version got shipped and signed by MS for x86 that invokes the protocol incorrectly, resulting in a crash. There is no technical reason to disable this security/robustness feature - it is only inertia by the distros, which already have a fixed version of shim at their disposal, but are refusing to package it for arm64 because MS are currently not willing to sign the x86 version using their 3rd party CA cert.

[mariobalanica]

Done.

[leiflindholm]

@mariobalanica I was about to merge, but now build fails due to a typo in the .dsc change: "EmdeddedPkg". I'd fix it up myself, but don't have edit access on the branch, could you update it plese?

[mariobalanica]

Fixed