Closed SergiiDmytruk closed 9 months ago
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:
aem
, but this would make every bump on Qubes side much harder for us, requiring another rebase each time it happens,
- 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.
@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.
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.
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. */
Also started handling DRTM entry's flags:
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 :slightly_smiling_face: 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.
Updated constants and flags handling in https://github.com/TrenchBoot/xen/compare/6ec1f7b7ae9af57ca6abbb7729255aaf14fbcfa7..c8f93f26c569ab37177393c2ceedb0cd012da4d1#
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.
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.
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.
I think
verify_drtm_mbi_consistency()
now needs to be called separately earlier (wheretpm_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
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.