OvmfPkg: Create SP800155 HOBs from QemuFwCfgFile

[deeglaze]

Signed firmware measurements are allowed to be passed along to in the TCG and CC event logs according to the TCG PC Client Platform Firware Profile. The event logs include events that Tcg2Dxe reads from appropriately GUIDed HOBs, so allow etc/sp800155evts to pass along events that the VMM sees fit to provide.

The VMM may provide reference measurements through UEFI variables that it references from the SP800-155 event3 structure given the appropriate RIM locator type, or via URL, etc.

After the HOBs are read from fw_cfg, they are verified for internal consistency. The SEV-SNP/TDX threat model includes attacks from the VMM, so without this checking, the host VMM could corrupt data.

Description

• Breaking change? No
• Impacts security? May improve security by providing reference measurement metadata through a standard channel.
• Includes tests? No

How This Was Tested

Internally on Google's vmm integration test framework.

Integration Instructions

1. Provide etc/sp800155evts through Qemu with contents in ovmf_x64_csm_debug.fd.evts. This evts file includes 2 events. One references a UEFI variable and the other references a cloud storage bucket that is not yet publicly readable.
2. Create a fake efivarfs mount point, say mkdir -p /tmp/efivars and put the ovmf_x64_csm_debug.fd.signed contents in /tmp/efivars/FirmwareRIM-a2858e46-a37f-456a-8c79-0c1fe48b65ff
3. read the binary bios measurements from user space, and validate that the event is present in its expected unmeasured state. User space code to do said extraction can be done with the gce-tcb-endorsement CLI tool's extract command (cli binary released as gcetcbendorsement):

gcetcbendorsement extract --efivarfs=/tmp/efivars --out=endorsement.binarypb

For extra good measure that the contents are as expected, you may run

gcetcbendorsement verify endorsement.binarypb

The verify command will attempt to download https://pki.goog/cloud_integrity/GCE-cc-tcb-root_1.crt to verify the signature. A local path to the root certificate may also be provided with --root_cert.

[deeglaze]

/cc @kraxel @jyao1

[deeglaze]

I've opened https://bugzilla.tianocore.org/show_bug.cgi?id=4782 regarding the false positive in PatchCheck.

[kraxel]

From a design point of view I think the HOB should be created by OVMF instead of being passed in from the VMM.

Looking at the fields in tdTCG_Sp800_155_PlatformId_Event3 most of it isn't new.

• The Platform* fields can be filled from smbios data (type 1 table) passed by the VMM.
• The Firmware* fields can be filled from PcdFirmwareVendor + PcdFirmwareVersionString.
• For RimLocator + PlatformCertLocator we probably need some way to pass them from VMM to OVMF (for Event3 support, Event2 doesn't need them).

Independent from the above the code has a bunch of problems:

• The handlng of multiple HOBs in the fw_cfg file is awkward.
• IMHO the code should not ASSERT but instead simply ignore an invalid fw_cfg file.
• The HOB GUID is not checked, so the VMM can insert /any/ EFI_HOB_TYPE_GUID_EXTENSION HOB, which in turn opens up a number of attack vectors.

[deeglaze]

Oh right an additional problem is in the event string lengths themselves. That would mean fully interpreting the struct and therefore knowing its exact beginning and end. Would you be okay with just a concatenation of PlatformIdEvent structs?

[kraxel]

Oh right an additional problem is in the event string lengths themselves. That would mean fully interpreting the struct and therefore knowing its exact beginning and end.

Good point. When interpreting the struct anyway it's also possible to check the Firmware* fields and PcdFirmware* match.

Would you be okay with just a concatenation of PlatformIdEvent structs?

Looks good to me.

The fw_cfg file should be in opt/org.tianocode/ btw.

[deeglaze]

We don't provide smbios data in our hypervisor, so I'm not fully confident in my ability to test cross-checking.

At first I added a whole event interpretation for every field, but then thought better of it just for simplicity. If we're interpreting the data, we should interpret as little as we need to. I've just stuck with a stream of uint16-sized strings that must sum to the size of the FwCfg file (including the size of the sizes).

If someone wants to add further validation when SMBIOS is present, that seems like an acceptable follow-up.

[kraxel]

At first I added a whole event interpretation for every field, but then thought better of it just for simplicity. If we're interpreting the data, we should interpret as little as we need to. I've just stuck with a stream of uint16-sized strings that must sum to the size of the FwCfg file (including the size of the sizes).

Hmm? While most of tdTCG_Sp800_155_PlatformId_Event2 is strings with size, there are a number of additional fields: Sigranture, VendorId, ReferenceManifestGuid, FirmwareManufacturerId. Event3 additionally has RimLocatorType and PlatformCertLocatorType. Note the strings are UINT8 not UINT16. Also note the size for some strings is UINT8 while for others it is UINT32. I think there is no way around parsing the struct completely.

I'd also suggest to move the code to a new source file.

[deeglaze]

@kraxel As long as you have the event length a priori, you don't need to parse the event to determine the length, which is why I made the format a file with size S with contents uint16str ..., where a uint16str is struct { UINT16 Size; UINT8 Data[Size]; }, such that the combined total size of the uintr16str sums to S.

The event gets stored in a HOB, which has its own HobLength which Tcg2Dxe uses for determining EventSize. It's only the event log reader that needs to parse the contents if they indeed care about what it contains.

I've moved the logic to PlatformId.c and PlatformId.h.

[kraxel]

@kraxel As long as you have the event length a priori, you don't need to parse the event to determine the length, which is why I made the format a file with size S with contents uint16str ..., where a uint16str is struct { UINT16 Size; UINT8 Data[Size]; }, such that the combined total size of the uintr16str sums to S.

Hmm. I liked the approach without the header more. Sure, the drawback is that you have to parse the complete event to figure what the size is. The advantage is that you can apply some checks, such as checking FirmwareVersion and PcdFirmwareVersionString being identical. Log the event fields for debugging purposes. Also it is possible to store a raw event as-is in sp800155evts.

[deeglaze]

I've gone back to interpreting the whole event, and this time I'm storing the different pointers in a local struct for those debug messages you alluded to, and perhaps extra checking if someone wants to add that.

[kraxel]

I've gone back to interpreting the whole event, and this time I'm storing the different pointers in a local struct for those debug messages you alluded to, and perhaps extra checking if someone wants to add that.

Thanks, looks much better to me.

Minor nit: The strings are supposed to be NULL-terminated according to the comments. I'd suggest to verify this is actually the case. With that in place you can drop also your hand-rolled AsciiToUcs2 function and call AsciiStrToUnicodeStrS instead. For logging you can skip even that and simply use the %a printf format directive.

A common prefix for the DEBUG messages would be good I think, to make clear what the context is and to allow for an easy grep in the logfile.

[deeglaze]

I've added a fixup commit because I noticed an outdated claim in the PlatformId.h header.

[lgao4]

Jiewen, could you help confirm the change in MdePkg TCG Platform definition?

[deeglaze]

@jyao1 for ease of review https://trustedcomputinggroup.org/wp-content/uploads/PC-Client-Platform-Firmware-Profile-Version-1.06-Revision-52_pub.pdf has the event definition on page 167

[ardbiesheuvel]

@mdkinney @makubacki there seems to be a problem with this PR: even though I don't object to the contents of the third patch that Dionna added, it is kind of strange that doing so did not invalidate my review. I think this is a rather fundamental problem, given that I may rely on other maintainers' Reviewed-by before setting the 'push' label, but it is not guaranteed that the version they reviewed is the same version that gets merged in the end.

Could we clear all reviews when the PR changes?

[makubacki]

Could we clear all reviews when the PR changes?

There is a branch setting to do that:

We decided to leave that unchecked to balance the overhead and time for each reviewer to re-review each push to each PR. It can especially slow down overall PR completion with required reviewers. In past work, we've found most updates are minor, in response to direct feedback or the contributor calls out that a re-review is needed. Similarly, Reviewed-by tags would often be carried forward in patch series revisions without a similar guarantee the patch did not change.

I don't have a strong opinion and the policy is available if there's a preference to use it.

[deeglaze]

@jyao1 Any comments? Would like to move this forward if possible.

[jyao1]

Usually we use one PR to resolve one issue only.
As such, I highly recommend splitting it to 2 PR, since they are totally irrelevant and they sit in different packages.

One is only for dup field removal in MdePkg – I can approve it immediately.
The other is to add OVMF support – I need more time to review and feedback. I will post the feedback in-line.

[jyao1]

After the HOBs are read from fw_cfg, they are verified for internal consistency. The SEV-SNP/TDX threat model includes attacks from the VMM, so without this checking, the host VMM could corrupt data.

Yes, that is my biggest worry on this patch.
Would you please share what kind of security validation you have done, to ensure the function can deal with all different data corruption? For example, fuzzing?
Is that possible to share your test code, or check in as part of unit test in EDKII?

[deeglaze]

Thank you, @jyao1, for helping this PR be so much better.

@lgao4 @mdkinney Any further comments?