Switch to using SLRT interface and process DRTM policy

[SergiiDmytruk]

No description provided.

[SergiiDmytruk]

CI seems to fail because of recent changes in https://github.com/QubesOS/qubes-vmm-xen. I think smp_cleanup_upstreaming was recently rebased to take this into account.

[krystian-hebel]

smp_cleanup_upstreaming was rebased just for upstreaming changes not directly related to AEM. I think that the conflict is caused by https://github.com/QubesOS/qubes-vmm-xen/blob/main/0512-xsa446.patch, I also had to do some manual work around it during rebasing.

I'm not sure what would be the best approach to deal with it. We could:

• include QubesOS-specific patches in aem, but this would make every bump on Qubes side much harder for us, requiring another rebase each time it happens,
• make CI do a rebase on-the-fly, resolving conflicts in the process (patches to patch patches on top of patches...),
• wait for https://github.com/QubesOS/qubes-vmm-xen to switch to Xen 4.18 (IIUC it was released last Friday so it may be soon, maybe @marmarek would know the expected bump date), which includes XSA fixes.

[SergiiDmytruk]

• include QubesOS-specific patches in aem, but this would make every bump on Qubes side much harder for us, requiring another rebase each time it happens,

Could apply only those patches which cause conflicts if that's possible.

• make CI do a rebase on-the-fly, resolving conflicts in the process (patches to patch patches on top of patches...),

I think format-patch + am essentially amounts to what git rebase would do (rebase generates its temporary patches a bit differently, but I don't think that would help), so it might result in a similar failure when conflicts can't be resolved automatically.

[SergiiDmytruk]

@krystian-hebel So, to move this and GRUB changes forward, measuring only MBI and measuring SLRT as described in the specification? If this turns out to be problematic, it's unlikely to be the only thing that wasn't done properly right away, so might be unwise to get stuck on it. It's not even large amount of code either way.

[krystian-hebel]

I'd say MBI, modules and SLRT as in sl_extend_slrt in patches sent to Linux:

/*
* In revision one of the SLRT, the only table that needs to be
* measured is the Intel info table. Everything else is meta-data,
* addresses and sizes. Note the size of what to measure is not set.
* The flag SLR_POLICY_IMPLICIT_SIZE leaves it to the measuring code
* to sort out.
*/

Patch 05/13 of that series also has different definitions for TXT_EVTYPE_* constants, we probably should align with those.

[SergiiDmytruk]

Patch 05/13 of that series also has different definitions for TXT_EVTYPE_* constants, we probably should align with those.

There is this comment in intel_txt.h:

/* TXT-defined use 0x4xx, TrenchBoot in Linux uses 0x5xx, use 0x6xx here. */

[SergiiDmytruk]

Also started handling DRTM entry's flags:

1. https://github.com/TrenchBoot/xen/compare/d820e3dac46047e7e81db6fdc30d72041e9d208e..6ec1f7b7ae9af57ca6abbb7729255aaf14fbcfa7#diff-5861d423276bd98c31fb2a501569110297cbe6488842e0359435a3af795c24aeR1040-R1042
2. https://github.com/TrenchBoot/xen/compare/d820e3dac46047e7e81db6fdc30d72041e9d208e..6ec1f7b7ae9af57ca6abbb7729255aaf14fbcfa7#diff-5861d423276bd98c31fb2a501569110297cbe6488842e0359435a3af795c24aeR1059-R1061
3. https://github.com/TrenchBoot/xen/compare/d820e3dac46047e7e81db6fdc30d72041e9d208e..6ec1f7b7ae9af57ca6abbb7729255aaf14fbcfa7#diff-5861d423276bd98c31fb2a501569110297cbe6488842e0359435a3af795c24aeR1099-R1106

[krystian-hebel]

Also started handling DRTM entry's flags: (...)

I think that check for SLR_POLICY_FLAG_MEASURED for modules (1. and 2.) should be removed. We don't use it in any way right now, but if we ever will (e.g. if some low-level module is consumed very early, earlier than verify_drtm_mbi_consistency() is called) it would make sense to measure it earlier and set mentioned flag without actually removing the module from MBI. We would still need to check whether a) module exists in MBI and b) has the same address and size.

As for 3rd link, the code should test if entries that are already measured are first entries in the table. Order of measurements matters, it should reflect the order of policy entries. So if an entry that is not yet measured is found, no further entry may have SLR_POLICY_FLAG_MEASURED set. Ideally, error message should mention that order of modules in grub.cfg should match that of policy, because IIUC this is how GRUB orders modules in MBI.

There is this comment in intel_txt.h: (...)

I know, I wrote it 🙂 This was before any specification was even planned and design decisions were still being made, so I wanted to avoid any potential conflicts. Now those types are somewhat defined, and most entries use the same type, which is different to what I planned back then. Having fewer types will make it easier to add support for other payloads/OSes/hypervisors without having to add new types, propagate them and potentially resolve any conflicts.

[SergiiDmytruk]

Updated constants and flags handling in https://github.com/TrenchBoot/xen/compare/6ec1f7b7ae9af57ca6abbb7729255aaf14fbcfa7..c8f93f26c569ab37177393c2ceedb0cd012da4d1#

[krystian-hebel]

I've tried booting this + GRUB on hardware and it seems to reboot somewhere in Xen, before anything is printed on the screen. I don't have a way of connecting to serial at the moment, so I can't debug any further.

[SergiiDmytruk]

What about GRUB changes + first two commits of this PR? That should work if bug doesn't show up without the last commit.

If it works and deployment of new code isn't too hard, it's possible to bisect changes by adding return somewhere in tpm_take_measurements() to narrow it down a bit (everything should work if that code doesn't run at all). Could be that GRUB provides incorrect data and Xen code is fine, knowing approximately how far things go should make code inspection easier either way.

[krystian-hebel]

What about GRUB changes + first two commits of this PR?

Same. I ended up adding infinite loop as the first Xen instruction and it still reboots, so the issue is most likely in GRUB. Unfortunately, it seems that either BIOS clears TXT.ERRORCODE on boot or reboot happens before SINIT. In any case, I'm moving the review to GRUB PR.

[krystian-hebel]

These should be all of the changes required to boot successfully.

[SergiiDmytruk]

There were lots of changes in various places, but hopefully they agree on each other.

I think verify_drtm_mbi_consistency() now needs to be called separately earlier (where tpm_take_measurements() was called before), but other than that changes should address the review. Will do this change tomorrow.

[SergiiDmytruk]

I think verify_drtm_mbi_consistency() now needs to be called separately earlier (where tpm_take_measurements() was called before), but other than that changes should address the review. Will do this change tomorrow.

That turned out to be unnecessary, modules are updated after DRTM is processed. I however turned tpm_take_measurements() into check_drtm_policy() and collected all checks related to DRTM entries there (from slr_get_policy() and tpm_process_drtm_policy()) as they were too far apart. See https://github.com/TrenchBoot/xen/compare/cc6e675973ecc45d3e99d0c9b1301f16d3912476..f74bb7a7e506d9beb2e8267a9d3b4572b968b62d