Inteltxt support #160

TomaszAIR · 2023-02-08T09:37:08Z

Set of patches which adds Intel TXT support in Xen for TrenchBoot.

This is necessary to create Proof of Concept for TrenchBoot Anti Evil
Maid for QubesOS.

Due to the requirements of Intel TXT and how it is utilised, it is
impossible to use the Xen boot protocols defined in the UEFI or
Multiboot2 specifications. Those patches creates a custom Intel TXT
entry point for Xen which would hand-off to the standard Multiboot2
entry point and enable direct launch of Xen by GRUB via DRTM on Intel
hardware. Additionally there was no support for launching Xen with Intel
TXT other than Trusted Boot. Certain parts had to be ported from
Trusted Boot specific code to Xen native code:

constructing MLE header
waking up APs
restoring MTRRs
reserving the TXT memory
reenabling SMIs
handling TXT shutdown and S3 resume/suspend
TPM event log finding

See: https://lists.xenproject.org/archives/html/xen-devel/2022-10/msg01663.html
for details

Signed-off-by: Tomasz Żyjewski [email protected]

marmarek · 2023-02-08T13:31:51Z

It doesn't build on fc37-based dom0:
https://gitlab.com/QubesOS/qubes-vmm-xen/-/jobs/3733914032#L282

04:49:26,195 [executor:docker:91e433f5ab] output: DEBUG: ld -melf_i386  -N -T arch/x86/boot/build32.lds -o arch/x86/boot/tpm_early.lnk arch/x86/boot/tpm_early.o
04:49:26,195 [executor:docker:91e433f5ab] output: DEBUG: ld: arch/x86/boot/tpm_early.o: in function `tpm_hash_extend':
04:49:26,196 [executor:docker:91e433f5ab] output: DEBUG: tpm.c:(.text+0x43f): undefined reference to `memcpy'
04:49:26,196 [executor:docker:91e433f5ab] output: DEBUG: make[4]: *** [arch/x86/boot/Makefile:37: arch/x86/boot/tpm_early.lnk] Error 1

TomaszAIR · 2023-02-09T07:52:20Z

@marmarek Thank you for pointing that out. Will check this. To reproduce this error I need to change DIST_DOM0 to fc37 locally in builder.conf used when I built this package?

Also, should I set you as reviewer of this PR? Got also similar for grub2 QubesOS/qubes-grub2#13

marmarek · 2023-02-09T11:08:56Z

@marmarek Thank you for pointing that out. Will check this. To reproduce this error I need to change DIST_DOM0 to fc37 locally in builder.conf used when I built this package?

Yes, that should be enough.

Also, should I set you as reviewer of this PR? Got also similar for grub2 QubesOS/qubes-grub2#13

You can if you want, but no need to do that.

TomaszAIR · 2023-02-20T16:31:44Z

@marmarek sorry it took so long, rebased on latest main and with this change a56c58b the build should not fail anymore on fc37

marmarek · 2023-02-21T16:35:29Z

Changing initramfs results in changes to both PCR-17 and PCR-18, is that desired? Is it because initramfs size is implicitly measured into PCR-18, while initramfs content is measured into PCR-17?

marmarek · 2023-02-21T18:18:35Z

It indeed seems to be about the size - if just initramfs content is different but still the same size, then PCR-18 remains unchanged.

This isn't very problematic this way. More problematic would be intended change to PCR-18 also affecting PCR-17. I guess it's about measuring multiboot structure which includes initramfs size, right? Selective measurement is probably not worth the risk, but maybe I'm missing something?

TomaszAIR · 2023-02-23T13:33:08Z

@krystian-hebel I would say that you are the expert in this regard. Could you answer these questions?

krystian-hebel · 2023-02-23T14:44:17Z

I guess it's about measuring multiboot structure which includes initramfs size, right?

Exactly. PCR18 contains (among others) measurement of Multiboot2's MBI, which contains size of all modules, including initramfs. Same thing would happen if kernel's size changes, or even if some additional commands are run in GRUB before booting that require allocating memory and kernel/initramfs lands in different place as a consequence. If external, untrusted devices can be memory mapped, this may be relevant. Another important thing that lives in MBI is kernel's command line, I think I don't have to explain what security implications of changing it without measurement would be.

On Intel platforms, PCR18 also contains some other data, as described in TXT specification, I don't remember what exactly. AMD platforms would have full control over PCR18 content, so with current approach it would contain only MBI. To make it a bit more isolated we could move MBI measurement to another PCR (19+), but for reasons mentioned in the previous paragraph it still should be included in any attestation decision.

Selective measurement is probably not worth the risk, but maybe I'm missing something?

This would require parsing (using) structure that is not yet measured (trusted), which may open a way for TOCTOU attacks. This parser would also have to closely follow logic that is used by OS's parser, especially around corner cases like e.g. what happens if there are 2 or more command line tags.

TomaszAIR · 2023-03-08T12:58:47Z

@marmarek WDYT about above? Should we work more about these patches?

TomaszAIR · 2023-03-28T06:36:25Z

@marmarek How can we help to get this changes merged?

Pokisiekk · 2023-03-28T13:22:21Z

I'm trying to install the packages and confirm their correctness according to blog post, but I've encountered a problem:

The sudo qubes-dom0-update --enablerepo=qubes-dom0-current-testing command ends with an error (I also tried with --skip-broken):

When trying to install any package in dom0, I get the same error as reported here: QubesOS/qubes-issues#6885, so I can't install missing dependencies.

I'm working on an image from https://openqa.qubes-os.org/tests/55506#downloads, where I once completely reproduced the results from a blog post with earlier versions of packages.

I would like to ask for help, what should I do in this situation?

Pokisiekk · 2023-03-30T14:14:25Z

The reason for the issues from the #160 (comment) was that I received the wrong packages.

I went back to reproduce the results from the blog post and after completing step 6 form Installing Xen and GRUB packages section, I wanted to boot into the system, but it was no longer possible.

I have only one entry in Grub: Qubes, with Xen 4.17.0 and Linux 5.15.81-1.fc32, after selecting which the action is not always the same:

Turning on the platform and leaving it alone and waiting 5s seconds in Grub Menu freezes the platform, nothing else happens.
Turning on the platform, clicking ESC to enter boot menu, selecting the QubesOS disk and then clicking Enter in the Grub Menu before the timer expires will also freeze the platform.
Turning on the platform and clicking Enter in the Grub Menu before the timeout causes the restart of the platform.

meithecatte

1304-arch-x86-intel_txt.c-fixes-after-testing.patch

This is definitely not ready for review, please clean up.

1301-xen-arch-x86-boot-Add-MLE-header-and-file-for-new-en.patch

1302-xen-arch-x86-reserve-TXT-memory.patch

TomaszAIR · 2023-04-21T12:43:22Z

Changed to draft because we want to use CI to check if changes compiled correctly

TomaszAIR · 2023-04-26T07:46:28Z

Looks like CI ended with success. @meithecatte I kindly re-request for review.

@krystian-hebel cc

BeataZdunczyk · 2023-08-01T09:22:12Z

@marmarek, @meithecatte We'd really appreciate your review and feedback on these two PRs:

As these PRs are closing phase one, your input is crucial in determining their merge status. If any blockers exist, please let us know so we can address them together.

pietrushnic · 2023-08-16T12:06:53Z

@marmarta @meithecatte when you will have time to take a look at those patches?

HW42

This is a first batch of comments. Focused on things that make sense on it's own. I'm still reading up on Intel TXT, so review of more involved things are still in progress.

1301-x86-include-asm-intel_txt.h-constants-and-accessors-.patch

1309-x86-boot-choose-AP-stack-based-on-APIC-ID.patch

1310-x86-smpboot.c-TXT-AP-bringup.patch

1311-x86-setup.c-don-t-use-XSM-policy-and-ucode-updates-w.patch

HW42 · 2023-11-13T11:49:51Z

I think naming of things in a global namespace (constants in headers, global variables, etc.) deserve some thought. This is not blocking for usage in Qubes but a good preparation for getting it upstream.

I have seen:

https://lore.kernel.org/all/[email protected]/:

A quick note on terminology. The larger open source project itself is called
TrenchBoot, which is hosted on Github (links below). The kernel feature enabling
the use of Dynamic Launch technology is referred to as "Secure Launch" within
the kernel code. As such the prefixes sl_/SL_ or slaunch/SLAUNCH will be seen
in the code. The stub code discussed above is referred to as the SL stub.

(A similar note should go in the Xen patches somewhere)

I'm not sure why you avoid the Trenchboot name here. I think sl/slaunch is a bit non-ideal choice given that in the same context it could also mean "Secure Loader" (AMD specific) or "static launch" (as in SRTM instead of DRTM), but I guess that ship has sailed.

Consistently using slaunch as prefix at least makes further conflicts less likely since it's not so short like sl.

trenchboot_sl would be pretty clear but maybe too long.

Via @marmarek other ideas from some Xen developers:

sl seems a bit too short to me; but we have loads of 3-letter abbreviations (e.g., p2m, ept, etc). Could you add another letter to your acronym? Like "tsl" for "Trenchboot Secure Launch"?
seclaunch_ maybe, if variable names don't get too long

Again, this isn't blocking, but might be worth to think about.

Edit: missed firth paragraph when copy+pasting.

1301-x86-include-asm-intel_txt.h-constants-and-accessors-.patch

BeataZdunczyk · 2024-02-15T17:03:13Z

@meithecatte, @HW42 could you please review the changes by @SergiiDmytruk?
@marmarek cc

1301-x86-boot-add-MLE-header-and-new-entry-point.patch

HW42 · 2024-05-06T07:08:23Z

@SergiiDmytruk, @krystian-hebel: First of all sorry for the long delay on this task. I'm aware that in the meantime you added some fixes and new functionality on your https://github.com/TrenchBoot/xen repo. I already looked at some of them. I see that in the aem-4.17.4 branch you even already rebased them to Xen 4.17.4. Can you please update this PR. GitHub's review features is, in my experience, not good at matching comments between revisions of a PR, so it would be good to have this before adding more comments.

SergiiDmytruk · 2024-05-06T13:29:55Z

@HW42, I guess the next step here depends on how this will get merged:

If there is a chance of getting this set of commits in after minor corrections, we could do the review changes and continue here taking rebase into account, then send the rest of the patches (AMD part so far) on top of it and don't get back to this set of patches (even though some new commits move code from them around).
If the next round of review requires looking at the whole set of changes basically from scratch, then it might be better to rework commit history, close this PR and open a new one in its place.

HW42 · 2024-05-06T14:43:53Z

@SergiiDmytruk: I think that having a separate set of patches here, and let that diverge from what you plan to upstream is very undesirable. Especially for such a big set of patches. So please update the PR with the latest version you have. If you look at the history here we already had such force pushes. Yes, this makes review harder, but in my opinion clearly outweighs a diverging set of patches.

If you feel like GitHub PR comments here got too messy, you can create a new PR, as you suggested. Works for me either way.

SergiiDmytruk · 2024-05-12T21:52:41Z

Squashed 6 commits from aem-4.17.4 branch into some of the previous ones to get aem-phase4-rebase, addressed the latest review comments and rebased the branch here.

HW42

Commenting in small batches after having lost a unfinished review. Sorry in advance for the multiple notifications.

HW42 · 2024-08-07T13:16:18Z

1300-x86-include-asm-intel_txt.h-constants-and-accessors-.patch

+
+/* The same set of registers is exposed twice (with different permissions) and
+ * they are allocated continuously with page alignment. */
+#define NR_TXT_CONFIG_PAGES ((TXT_PUB_CONFIG_REGS_BASE - \


All users of this constant seems to do * PAGE_SIZE, so maybe change this to be the size in bytes?

https://github.com/TrenchBoot/xen/compare/dd475a4a76..aem-phase4-rebase#diff-dd4611d055ace3b3c87ffe90aaf480b0d4a0e4098adb6de5026e3144a4821b09L9

https://github.com/TrenchBoot/xen/compare/dd475a4a76..aem-phase4-rebase#diff-31035528a674298c3fdbc80ca9452ac5a422984a24386556a0729ee18ab93aebL18

https://github.com/TrenchBoot/xen/compare/dd475a4a76..aem-phase4-rebase#diff-31035528a674298c3fdbc80ca9452ac5a422984a24386556a0729ee18ab93aebL56

https://github.com/TrenchBoot/xen/compare/dd475a4a76..aem-phase4-rebase#diff-31035528a674298c3fdbc80ca9452ac5a422984a24386556a0729ee18ab93aebL56

HW42 · 2024-08-07T13:16:48Z

1300-x86-include-asm-intel_txt.h-constants-and-accessors-.patch

+ * Functions to extract data from the Intel TXT Heap Memory. The layout
+ * of the heap is as follows:
+ *  +---------------------------------+
+ *  | Size Bios Data table (uint64_t) |


Nitpick: Size of ...

https://github.com/TrenchBoot/xen/compare/dd475a4a76..aem-phase4-rebase#diff-dd4611d055ace3b3c87ffe90aaf480b0d4a0e4098adb6de5026e3144a4821b09R237-R253

HW42 · 2024-08-07T13:19:12Z

1302-x86-boot-txt_early-add-early-TXT-tests-and-restore-M.patch

+     * yet supported by the code.
+     */
+    if (os_sinit->vtd_pmr_lo_size == 0)
+        txt_reset(SLAUNCH_ERROR_LO_PMR_BASE);


For high PMR you defined SLAUNCH_ERROR_HI_PMR_SIZE, was this meant to be SLAUNCH_ERROR_LO_PMR_SIZE? (Probably doesn't matter much if you distinguish those cases, but noticed that you already defined an error code for the other region's size)

https://github.com/TrenchBoot/xen/compare/dd475a4a76..aem-phase4-rebase#diff-dd4611d055ace3b3c87ffe90aaf480b0d4a0e4098adb6de5026e3144a4821b09R63-R74

https://github.com/TrenchBoot/xen/compare/dd475a4a76..aem-phase4-rebase#diff-584be1f5aaa2f0679e692f6344b2e03670316e2ebdb7f4ba1dc4fbfb513d196cR67

HW42 · 2024-08-07T13:22:06Z

1302-x86-boot-txt_early-add-early-TXT-tests-and-restore-M.patch

+    txt_heap = _p(read_txt_reg(TXTCR_HEAP_BASE));
+
+    if (txt_os_mle_data_size(txt_heap) < sizeof(*os_mle) ||
+        txt_os_sinit_data_size(txt_heap) < sizeof(*os_sinit))


If I'm reading this right, those checks are of by sizeof(uint64_t). I would suggest to let txt_*_data_size return the size of the memory area without the size header (this obviously needs adaption of the math in txt_*_data_start.

Good catch. https://github.com/TrenchBoot/xen/compare/dd475a4a76..aem-phase4-rebase#diff-dd4611d055ace3b3c87ffe90aaf480b0d4a0e4098adb6de5026e3144a4821b09R257-R304

HW42 · 2024-08-07T13:23:42Z

1303-include-xen-slr_table.h-Secure-Launch-Resource-Table.patch

+
+slr_add_entry() and slr_init_table() were omitted to not have issues
+with memcpy() usage (it comes from different places for different
+translation units).


Side note: With having prefix the other stuff with slaunch, slr_ is now a bit inconsistent. But probably better to keep it this way, since then the identifiers match your "Secure Launch Specification".

HW42 · 2024-08-07T13:25:34Z

1304-xen-arch-x86-reserve-TXT-memory.patch

+
+    map_l2(txt_heap_base, txt_heap_size);
+
+    find_evt_log(&evt_log_addr, &evt_log_size);


Missing NULL check of return value.

https://github.com/TrenchBoot/xen/compare/dd475a4a76..aem-phase4-rebase#diff-1d266d1dff9daf02e019100b9c620a099561c98811a05a0b7581363b3c00ac39R65-R66

HW42 · 2024-08-07T13:26:19Z

1304-xen-arch-x86-reserve-TXT-memory.patch

+           txt_heap_base + txt_heap_size);
+    e820_change_range_type(&e820_raw, txt_heap_base,
+                           txt_heap_base + txt_heap_size,
+                           E820_RAM, E820_RESERVED);


There's reserve_e820_ram as a short cut for E820_RAM, E820_RESERVED

https://github.com/TrenchBoot/xen/compare/dd475a4a76..aem-phase4-rebase#diff-31035528a674298c3fdbc80ca9452ac5a422984a24386556a0729ee18ab93aebR38

https://github.com/TrenchBoot/xen/compare/dd475a4a76..aem-phase4-rebase#diff-31035528a674298c3fdbc80ca9452ac5a422984a24386556a0729ee18ab93aebR51

https://github.com/TrenchBoot/xen/compare/dd475a4a76..aem-phase4-rebase#diff-1d266d1dff9daf02e019100b9c620a099561c98811a05a0b7581363b3c00ac39R92

HW42 · 2024-08-07T13:27:15Z

1304-xen-arch-x86-reserve-TXT-memory.patch

+           txt_heap_base + txt_heap_size);
+    e820_change_range_type(&e820_raw, txt_heap_base,
+                           txt_heap_base + txt_heap_size,
+                           E820_RAM, E820_RESERVED);


Missing return check of here and below. To be fair existing Xen code seems to be inconsistent about it, too.

https://github.com/TrenchBoot/xen/compare/dd475a4a76..aem-phase4-rebase#diff-31035528a674298c3fdbc80ca9452ac5a422984a24386556a0729ee18ab93aebR31-R61

https://github.com/TrenchBoot/xen/compare/dd475a4a76..aem-phase4-rebase#diff-1d266d1dff9daf02e019100b9c620a099561c98811a05a0b7581363b3c00ac39R92

HW42 · 2024-08-07T13:29:25Z

1305-x86-intel_txt.c-restore-boot-MTRRs.patch

+    os_mle_size = txt_os_mle_data_size(__va(txt_heap_base));
+    os_mle = txt_os_mle_data_start(__va(txt_heap_base));
+
+    if ( os_mle_size < sizeof(*os_mle) )


Off by sizeof(uint64_t), see comment above.

The same https://github.com/TrenchBoot/xen/compare/dd475a4a76..aem-phase4-rebase#diff-dd4611d055ace3b3c87ffe90aaf480b0d4a0e4098adb6de5026e3144a4821b09R257-R304

HW42 · 2024-08-07T13:33:12Z

1306-x86-sha1.c-add-file.patch

+
+File comes from [1] and is licensed under MIT License. Only enough
+changes to make it compile under Xen and to swap endianness of result
+were made to the original file.


After having a look at it, this should to be pretty easy to get lib/crypto/sha{1,256}.c from Linux into Xen. The used macros already exist in Xen, or did I miss something.

Even for less critical cases like here (hashing public data), I think we should use a well reviewed and maintained implementation.

Signed-off-by: Tomasz Żyjewski <[email protected]>

Signed-off-by: Tomasz Żyjewski <[email protected]> Signed-off-by: Sergii Dmytruk <[email protected]>

TomaszAIR force-pushed the inteltxt-support branch from 4ca5894 to a56c58b Compare February 20, 2023 16:30

meithecatte suggested changes Apr 3, 2023

View reviewed changes

1301-xen-arch-x86-boot-Add-MLE-header-and-file-for-new-en.patch Outdated Show resolved Hide resolved

1302-xen-arch-x86-reserve-TXT-memory.patch Outdated Show resolved Hide resolved

BeataZdunczyk mentioned this pull request Apr 3, 2023

Review Phase 1 TrenchBoot Intel TXT and TPM 1.2 support Rull Requests TrenchBoot/trenchboot-issues#9

Open

TomaszAIR force-pushed the inteltxt-support branch from a56c58b to 98e5787 Compare April 21, 2023 12:42

TomaszAIR changed the title ~~Inteltxt support~~ Draft: Inteltxt support Apr 21, 2023

TomaszAIR force-pushed the inteltxt-support branch 5 times, most recently from 0ab3116 to a46e3e7 Compare April 25, 2023 14:57

TomaszAIR changed the title ~~Draft: Inteltxt support~~ Inteltxt support Apr 26, 2023

TomaszAIR requested a review from meithecatte April 26, 2023 07:45

HW42 requested review from HW42 and removed request for meithecatte November 13, 2023 10:44

HW42 reviewed Nov 13, 2023

View reviewed changes

1301-x86-include-asm-intel_txt.h-constants-and-accessors-.patch Outdated Show resolved Hide resolved

SergiiDmytruk force-pushed the inteltxt-support branch 3 times, most recently from 04208ab to 5bf7fa7 Compare January 7, 2024 22:51

krystian-hebel reviewed Feb 23, 2024

View reviewed changes

1301-x86-boot-add-MLE-header-and-new-entry-point.patch Outdated Show resolved Hide resolved

SergiiDmytruk force-pushed the inteltxt-support branch from 5bf7fa7 to d7feb43 Compare February 28, 2024 13:50

This was referenced Feb 28, 2024

x86/boot: generate value of size field in MLE header TrenchBoot/xen#9

Merged

i386/txt: do not update MLE header TrenchBoot/grub#18

Merged

HW42 mentioned this pull request Apr 10, 2024

Review TrenchBoot's Xen patches QubesOS/qubes-issues#9105

Open

3 tasks

SergiiDmytruk force-pushed the inteltxt-support branch 2 times, most recently from fbe20ec to 8e209dc Compare May 12, 2024 21:44

HW42 reviewed Aug 7, 2024

View reviewed changes

TomaszAIR added 2 commits August 11, 2024 01:10

xen.spec.in: update using variables in comments

cd37551

Signed-off-by: Tomasz Żyjewski <[email protected]>

Add patches for TrenchBoot DRTM support

413d4c8

Signed-off-by: Tomasz Żyjewski <[email protected]> Signed-off-by: Sergii Dmytruk <[email protected]>

SergiiDmytruk force-pushed the inteltxt-support branch from 8e209dc to 413d4c8 Compare August 10, 2024 22:14

HW42 changed the base branch from main to release4.2 August 21, 2024 13:49

Inteltxt support #160

Are you sure you want to change the base?

Inteltxt support #160

Conversation

TomaszAIR commented Feb 8, 2023

marmarek commented Feb 8, 2023

TomaszAIR commented Feb 9, 2023

marmarek commented Feb 9, 2023

TomaszAIR commented Feb 20, 2023

marmarek commented Feb 21, 2023

marmarek commented Feb 21, 2023

TomaszAIR commented Feb 23, 2023

krystian-hebel commented Feb 23, 2023

TomaszAIR commented Mar 8, 2023

TomaszAIR commented Mar 28, 2023

Pokisiekk commented Mar 28, 2023

Pokisiekk commented Mar 30, 2023

meithecatte left a comment

Choose a reason for hiding this comment

TomaszAIR commented Apr 21, 2023

TomaszAIR commented Apr 26, 2023

BeataZdunczyk commented Aug 1, 2023

pietrushnic commented Aug 16, 2023

HW42 left a comment

Choose a reason for hiding this comment

HW42 commented Nov 13, 2023 • edited Loading

BeataZdunczyk commented Feb 15, 2024

HW42 commented May 6, 2024

SergiiDmytruk commented May 6, 2024

HW42 commented May 6, 2024

SergiiDmytruk commented May 12, 2024

HW42 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

HW42 commented Nov 13, 2023 •

edited

Loading