search

TrenchBoot / trenchboot-issues

This repository is to centralize issues and development progress tracking for the TrenchBoot project.
3 stars 1 forks source link

Review Phase 1 TrenchBoot Intel TXT and TPM 1.2 support Rull Requests #9

Open BeataZdunczyk opened 1 year ago

BeataZdunczyk commented 1 year ago

Is your feature request related to a problem? Please describe.

This issue tracks the review, and merge process for the Pull Requests created to add TPM 1.2 support for Intel TXT in TrenchBoot GRUB2 and to implement Xen Secure Launch with Intel TXT support in Xen for TrenchBoot as Anti Evil Maid project.The problem is that TrenchBoot, which is used as an anti-evil maid solution in Qubes OS, does not currently support TPM 1.2 on Intel TXT path and does not have Xen Secure Launch with Intel TXT support. This limits the hardware compatibility for TrenchBoot and hinders its ability to protect against certain attacks.

Is your feature request related to a new idea or technology that would benefit the project? Please describe.

The feature request is to add TPM 1.2 support for Intel TXT in TrenchBoot GRUB2 and to implement Xen Secure Launch with Intel TXT support in Xen for TrenchBoot. This would allow TrenchBoot to support older Intel hardware with Intel TXT and launch Xen directly via DRTM on Intel hardware, improving its hardware compatibility and security.

Describe the solution you'd like

Following PRs merged:

  1. Add TPM 1.2 support for Intel TXT in TrenchBoot GRUB2: https://github.com/QubesOS/qubes-grub2/pull/13
  2. Implement Xen Secure Launch with Intel TXT support in Xen for TrenchBoot: https://github.com/QubesOS/qubes-vmm-xen/pull/160

Describe alternatives you've considered N/A

Additional context

This feature request is Phase 1 for TrenchBoot as Anti Evil Maid project, as outlined in the documentation: https://docs.dasharo.com/projects/trenchboot-aem/ and https://docs.dasharo.com/projects/trenchboot-aem-v2/

Relevant documentation you've consulted N/A

SergiiDmytruk commented 8 months ago

Rebased Xen patches (by now they include TPM 2.0 and processing of DRTM from SLRT) on aem-phase3-rebase of https://github.com/TrenchBoot/xen.

Commits looks a bit different now and most of the code differences are due to taking most of review comments into account (skipped those which might need discussion), especially the one about using a different prefix for SL-related symbols (I went with slaunch_ which was already used in one place and is also used in GRUB changes).

Commits: https://github.com/TrenchBoot/xen/compare/c23e698430...aem-phase3-rebase

Diff of diffs (includes "changes" like line number or hash differences):

diff -bu <( git diff --no-ext-diff cbb35e7..3dc5991) <( git diff --no-ext-diff c23e698..150a0a329a)

```diff --- /dev/fd/63 2023-12-20 19:40:09.799219058 +0200 +++ /dev/fd/62 2023-12-20 19:40:09.799219058 +0200 @@ -1,6 +1,6 @@ diff --git a/.github/workflows/build.yml b/.github/workflows/build.yml new file mode 100644 -index 0000000000..cfb966b60d +index 0000000000..5bff1deb9b --- /dev/null +++ b/.github/workflows/build.yml @@ -0,0 +1,17 @@ @@ -17,7 +17,7 @@ + qubes-dom0-package: + uses: TrenchBoot/.github/.github/workflows/qubes-dom0-package.yml@master + with: -+ base-commit: 'cbb35e72802f3a285c382a995ef647b59e5caf2f' ++ base-commit: 'c23e698430f3381304643d59b1ae373e36a318d5' + patch-start: 1300 + qubes-component: 'vmm-xen' + spec-pattern: '/^Patch1202:/' @@ -38,10 +38,10 @@ xen.gz ~~~~~~ diff --git a/xen/arch/x86/Makefile b/xen/arch/x86/Makefile -index 177a2ff742..594093fcca 100644 +index 4eec765106..76606f7617 100644 --- a/xen/arch/x86/Makefile +++ b/xen/arch/x86/Makefile -@@ -55,6 +55,7 @@ obj-y += percpu.o +@@ -56,6 +56,7 @@ obj-y += percpu.o obj-y += physdev.o obj-$(CONFIG_COMPAT) += x86_64/physdev.o obj-y += psr.o @@ -49,7 +49,7 @@ obj-y += setup.o obj-y += shutdown.o obj-y += smp.o -@@ -63,6 +64,7 @@ obj-y += spec_ctrl.o +@@ -64,6 +65,7 @@ obj-y += spec_ctrl.o obj-y += srat.o obj-y += string.o obj-y += time.o @@ -93,10 +93,10 @@ for (acpiid = 0; acpiid < ARRAY_SIZE(x86_acpiid_to_apicid); acpiid++) diff --git a/xen/arch/x86/apic.c b/xen/arch/x86/apic.c -index 47e6e5fe41..b33e9070e8 100644 +index fcd8d5e8b2..2ac86a3774 100644 --- a/xen/arch/x86/apic.c +++ b/xen/arch/x86/apic.c -@@ -989,7 +989,7 @@ __next: +@@ -985,7 +985,7 @@ __next: */ if (boot_cpu_physical_apicid == -1U) boot_cpu_physical_apicid = get_apic_id(); @@ -129,7 +129,7 @@ $(LD) $(subst x86_64,i386,$(LDFLAGS_DIRECT)) -N -T $(filter %.lds,$^) -o $@ $< diff --git a/xen/arch/x86/boot/head.S b/xen/arch/x86/boot/head.S -index 0fb7dd3029..32f99dfc52 100644 +index 245c859dd7..2cd9a3a0b5 100644 --- a/xen/arch/x86/boot/head.S +++ b/xen/arch/x86/boot/head.S @@ -3,6 +3,7 @@ @@ -152,7 +152,7 @@ + .long 0x42b651cb /* UUID3 */ + .long 0x00000034 /* MLE header size */ + .long 0x00020002 /* MLE version 2.2 */ -+ .long (sl_stub_entry - start) /* Linear entry point of MLE (SINIT virt. address) */ ++ .long (slaunch_stub_entry - start) /* Linear entry point of MLE (SINIT virt. address) */ + .long 0x00000000 /* First valid page of MLE */ + .long 0x00000000 /* Offset within binary of first byte of MLE */ + .long 0x00000000 /* Offset within binary of last byte + 1 of MLE */ @@ -166,7 +166,7 @@ .section .init.rodata, "a", @progbits .Lbad_cpu_msg: .asciz "ERR: Not a 64-bit CPU!" -@@ -425,6 +445,31 @@ __pvh_start: +@@ -428,6 +448,31 @@ __pvh_start: #endif /* CONFIG_PVH_GUEST */ @@ -190,7 +190,7 @@ + * - APs must be brought up by MONITOR or GETSEC[WAKEUP], depending on + * which is supported by a given SINIT ACM + */ -+sl_stub_entry: ++slaunch_stub_entry: + movl $SLAUNCH_BOOTLOADER_MAGIC,%eax + + /* Fall through to Multiboot entry point. */ @@ -198,7 +198,7 @@ __start: cld cli -@@ -453,6 +498,10 @@ __start: +@@ -456,6 +501,10 @@ __start: /* Bootloaders may set multiboot{1,2}.mem_lower to a nonzero value. */ xor %edx,%edx @@ -209,13 +209,13 @@ /* Check for Multiboot2 bootloader. */ cmp $MULTIBOOT2_BOOTLOADER_MAGIC,%eax je .Lmultiboot2_proto -@@ -468,6 +517,27 @@ __start: +@@ -471,6 +520,27 @@ __start: cmovnz MB_mem_lower(%ebx),%edx jmp trampoline_bios_setup +.Lslaunch_proto: + /* Save information that TrenchBoot slaunch was used. */ -+ movl $1, sym_esi(sl_status) ++ movl $1, sym_esi(slaunch_active) + + /* Push arguments to stack and call txt_early_tests(). */ + push $sym_offs(__2M_rwdata_end) /* end of target image */ @@ -237,7 +237,7 @@ .Lmultiboot2_proto: /* Skip Multiboot2 information fixed part. */ lea (MB2_fixed_sizeof+MULTIBOOT2_TAG_ALIGN-1)(%ebx),%ecx -@@ -789,6 +859,14 @@ cmdline_parse_early: +@@ -792,6 +862,14 @@ cmdline_parse_early: reloc: .incbin "reloc.bin" @@ -325,7 +325,7 @@ /* Relocations for trampoline Real Mode segments. */ diff --git a/xen/arch/x86/boot/txt_early.c b/xen/arch/x86/boot/txt_early.c new file mode 100644 -index 0000000000..f94b2c6cc0 +index 0000000000..c52a600e8d --- /dev/null +++ b/xen/arch/x86/boot/txt_early.c @@ -0,0 +1,131 @@ @@ -369,7 +369,7 @@ + + /* Verify the value of the low PMR base. It should always be 0. */ + if (os_sinit->vtd_pmr_lo_base != 0) -+ txt_reset(SL_ERROR_LO_PMR_BASE); ++ txt_reset(SLAUNCH_ERROR_LO_PMR_BASE); + + /* + * Low PMR size should not be 0 on current platforms. There is an ongoing @@ -377,12 +377,12 @@ + * yet supported by the code. + */ + if (os_sinit->vtd_pmr_lo_size == 0) -+ txt_reset(SL_ERROR_LO_PMR_BASE); ++ txt_reset(SLAUNCH_ERROR_LO_PMR_BASE); + + /* Check if regions overlap. Treat regions with no hole between as error. */ + if (os_sinit->vtd_pmr_hi_size != 0 && + os_sinit->vtd_pmr_hi_base <= os_sinit->vtd_pmr_lo_size) -+ txt_reset(SL_ERROR_HI_PMR_BASE); ++ txt_reset(SLAUNCH_ERROR_HI_PMR_BASE); + + /* All regions accessed by 32b code must be below 4G. */ + if (os_sinit->vtd_pmr_hi_base + os_sinit->vtd_pmr_hi_size <= 0x100000000ull) @@ -397,17 +397,17 @@ + + /* Check if all of Xen before relocation is covered by PMR. */ + if (!is_in_pmr(os_sinit, load_base_addr, xen_size, check_high_pmr)) -+ txt_reset(SL_ERROR_LO_PMR_MLE); ++ txt_reset(SLAUNCH_ERROR_LO_PMR_MLE); + + /* Check if all of Xen after relocation is covered by PMR. */ + if (load_base_addr != tgt_base_addr && + !is_in_pmr(os_sinit, tgt_base_addr, xen_size, check_high_pmr)) -+ txt_reset(SL_ERROR_LO_PMR_MLE); ++ txt_reset(SLAUNCH_ERROR_LO_PMR_MLE); + + /* Check if MBI is covered by PMR. MBI starts with 'uint32_t total_size'. */ + if (!is_in_pmr(os_sinit, os_mle->boot_params_addr, + *(uint32_t *)os_mle->boot_params_addr, check_high_pmr)) -+ txt_reset(SL_ERROR_BUFFER_BEYOND_PMR); ++ txt_reset(SLAUNCH_ERROR_BUFFER_BEYOND_PMR); + + /* Check if TPM event log (if present) is covered by PMR. */ + /* @@ -431,7 +431,7 @@ + if (os_mle->evtlog_addr != 0 && os_mle->evtlog_size != 0 && + !is_in_pmr(os_sinit, os_mle->evtlog_addr, os_mle->evtlog_size, + check_high_pmr)) -+ txt_reset(SL_ERROR_BUFFER_BEYOND_PMR); ++ txt_reset(SLAUNCH_ERROR_BUFFER_BEYOND_PMR); + */ +} + @@ -451,7 +451,7 @@ + + if (txt_os_mle_data_size(txt_heap) < sizeof(*os_mle) || + txt_os_sinit_data_size(txt_heap) < sizeof(*os_sinit)) -+ txt_reset(SL_ERROR_GENERIC); ++ txt_reset(SLAUNCH_ERROR_GENERIC); + + os_mle = txt_os_mle_data_start(txt_heap); + os_sinit = txt_os_sinit_data_start(txt_heap); @@ -513,10 +513,10 @@ BUG /* start_secondary() shouldn't return. */ diff --git a/xen/arch/x86/cpu/mwait-idle.c b/xen/arch/x86/cpu/mwait-idle.c -index 5d77672f6b..890b6ba3a9 100644 +index ffdc6fb2fc..1124f2be3d 100644 --- a/xen/arch/x86/cpu/mwait-idle.c +++ b/xen/arch/x86/cpu/mwait-idle.c -@@ -1093,8 +1093,8 @@ static void __init ivt_idle_state_table_update(void) +@@ -1213,8 +1213,8 @@ static void __init ivt_idle_state_table_update(void) unsigned int cpu, max_apicid = boot_cpu_physical_apicid; for_each_present_cpu(cpu) @@ -528,10 +528,10 @@ case 0: case 1: /* 1 and 2 socket systems use default ivt_cstates */ diff --git a/xen/arch/x86/domain.c b/xen/arch/x86/domain.c -index 41e1e3f272..34d3631904 100644 +index aca9fa310c..9a8cc4e92e 100644 --- a/xen/arch/x86/domain.c +++ b/xen/arch/x86/domain.c -@@ -1559,7 +1559,7 @@ long do_vcpu_op(int cmd, unsigned int vcpuid, XEN_GUEST_HANDLE_PARAM(void) arg) +@@ -1538,7 +1538,7 @@ long do_vcpu_op(int cmd, unsigned int vcpuid, XEN_GUEST_HANDLE_PARAM(void) arg) break; cpu_id.phys_id = @@ -541,7 +541,7 @@ rc = -EFAULT; diff --git a/xen/arch/x86/e820.c b/xen/arch/x86/e820.c -index b653a19c93..03a0a86902 100644 +index c5911cf48d..f836fec189 100644 --- a/xen/arch/x86/e820.c +++ b/xen/arch/x86/e820.c @@ -11,6 +11,7 @@ @@ -556,7 +556,7 @@ rdmsrl(MSR_MTRRcap, mtrr_cap); rdmsrl(MSR_MTRRdefType, mtrr_def); -+ if ( sl_status ) ++ if ( slaunch_active ) + txt_restore_mtrrs(e820_verbose); + if ( e820_verbose ) @@ -577,22 +577,23 @@ diff --git a/xen/arch/x86/include/asm/intel_txt.h b/xen/arch/x86/include/asm/intel_txt.h new file mode 100644 -index 0000000000..bbf6f566ed +index 0000000000..8be44fae1a --- /dev/null +++ b/xen/arch/x86/include/asm/intel_txt.h -@@ -0,0 +1,397 @@ +@@ -0,0 +1,398 @@ ++#include ++ +/* + * TXT configuration registers (offsets from TXT_{PUB, PRIV}_CONFIG_REGS_BASE) + */ -+ -+#include -+ +#define TXT_PUB_CONFIG_REGS_BASE 0xfed30000 +#define TXT_PRIV_CONFIG_REGS_BASE 0xfed20000 + ++/* Number of pages for each config regs space. */ +#define NR_TXT_CONFIG_PAGES ((TXT_PUB_CONFIG_REGS_BASE - \ + TXT_PRIV_CONFIG_REGS_BASE) >> PAGE_SHIFT) + ++/* Offsets from pub/priv config space. */ +#define TXTCR_STS 0x0000 +#define TXTCR_ESTS 0x0008 +#define TXTCR_ERRORCODE 0x0030 @@ -621,39 +622,39 @@ + * TXT Specification + * Appendix I ACM Error Codes + */ -+#define SL_ERROR_GENERIC 0xc0008001 -+#define SL_ERROR_TPM_INIT 0xc0008002 -+#define SL_ERROR_TPM_INVALID_LOG20 0xc0008003 -+#define SL_ERROR_TPM_LOGGING_FAILED 0xc0008004 -+#define SL_ERROR_REGION_STRADDLE_4GB 0xc0008005 -+#define SL_ERROR_TPM_EXTEND 0xc0008006 -+#define SL_ERROR_MTRR_INV_VCNT 0xc0008007 -+#define SL_ERROR_MTRR_INV_DEF_TYPE 0xc0008008 -+#define SL_ERROR_MTRR_INV_BASE 0xc0008009 -+#define SL_ERROR_MTRR_INV_MASK 0xc000800a -+#define SL_ERROR_MSR_INV_MISC_EN 0xc000800b -+#define SL_ERROR_INV_AP_INTERRUPT 0xc000800c -+#define SL_ERROR_INTEGER_OVERFLOW 0xc000800d -+#define SL_ERROR_HEAP_WALK 0xc000800e -+#define SL_ERROR_HEAP_MAP 0xc000800f -+#define SL_ERROR_REGION_ABOVE_4GB 0xc0008010 -+#define SL_ERROR_HEAP_INVALID_DMAR 0xc0008011 -+#define SL_ERROR_HEAP_DMAR_SIZE 0xc0008012 -+#define SL_ERROR_HEAP_DMAR_MAP 0xc0008013 -+#define SL_ERROR_HI_PMR_BASE 0xc0008014 -+#define SL_ERROR_HI_PMR_SIZE 0xc0008015 -+#define SL_ERROR_LO_PMR_BASE 0xc0008016 -+#define SL_ERROR_LO_PMR_MLE 0xc0008017 -+#define SL_ERROR_INITRD_TOO_BIG 0xc0008018 -+#define SL_ERROR_HEAP_ZERO_OFFSET 0xc0008019 -+#define SL_ERROR_WAKE_BLOCK_TOO_SMALL 0xc000801a -+#define SL_ERROR_MLE_BUFFER_OVERLAP 0xc000801b -+#define SL_ERROR_BUFFER_BEYOND_PMR 0xc000801c -+#define SL_ERROR_OS_SINIT_BAD_VERSION 0xc000801d -+#define SL_ERROR_EVENTLOG_MAP 0xc000801e -+#define SL_ERROR_TPM_NUMBER_ALGS 0xc000801f -+#define SL_ERROR_TPM_UNKNOWN_DIGEST 0xc0008020 -+#define SL_ERROR_TPM_INVALID_EVENT 0xc0008021 ++#define SLAUNCH_ERROR_GENERIC 0xc0008001 ++#define SLAUNCH_ERROR_TPM_INIT 0xc0008002 ++#define SLAUNCH_ERROR_TPM_INVALID_LOG20 0xc0008003 ++#define SLAUNCH_ERROR_TPM_LOGGING_FAILED 0xc0008004 ++#define SLAUNCH_ERROR_REGION_STRADDLE_4GB 0xc0008005 ++#define SLAUNCH_ERROR_TPM_EXTEND 0xc0008006 ++#define SLAUNCH_ERROR_MTRR_INV_VCNT 0xc0008007 ++#define SLAUNCH_ERROR_MTRR_INV_DEF_TYPE 0xc0008008 ++#define SLAUNCH_ERROR_MTRR_INV_BASE 0xc0008009 ++#define SLAUNCH_ERROR_MTRR_INV_MASK 0xc000800a ++#define SLAUNCH_ERROR_MSR_INV_MISC_EN 0xc000800b ++#define SLAUNCH_ERROR_INV_AP_INTERRUPT 0xc000800c ++#define SLAUNCH_ERROR_INTEGER_OVERFLOW 0xc000800d ++#define SLAUNCH_ERROR_HEAP_WALK 0xc000800e ++#define SLAUNCH_ERROR_HEAP_MAP 0xc000800f ++#define SLAUNCH_ERROR_REGION_ABOVE_4GB 0xc0008010 ++#define SLAUNCH_ERROR_HEAP_INVALID_DMAR 0xc0008011 ++#define SLAUNCH_ERROR_HEAP_DMAR_SIZE 0xc0008012 ++#define SLAUNCH_ERROR_HEAP_DMAR_MAP 0xc0008013 ++#define SLAUNCH_ERROR_HI_PMR_BASE 0xc0008014 ++#define SLAUNCH_ERROR_HI_PMR_SIZE 0xc0008015 ++#define SLAUNCH_ERROR_LO_PMR_BASE 0xc0008016 ++#define SLAUNCH_ERROR_LO_PMR_MLE 0xc0008017 ++#define SLAUNCH_ERROR_INITRD_TOO_BIG 0xc0008018 ++#define SLAUNCH_ERROR_HEAP_ZERO_OFFSET 0xc0008019 ++#define SLAUNCH_ERROR_WAKE_BLOCK_TOO_SMALL 0xc000801a ++#define SLAUNCH_ERROR_MLE_BUFFER_OVERLAP 0xc000801b ++#define SLAUNCH_ERROR_BUFFER_BEYOND_PMR 0xc000801c ++#define SLAUNCH_ERROR_OS_SINIT_BAD_VERSION 0xc000801d ++#define SLAUNCH_ERROR_EVENTLOG_MAP 0xc000801e ++#define SLAUNCH_ERROR_TPM_NUMBER_ALGS 0xc000801f ++#define SLAUNCH_ERROR_TPM_UNKNOWN_DIGEST 0xc0008020 ++#define SLAUNCH_ERROR_TPM_INVALID_EVENT 0xc0008021 + +#define SLAUNCH_BOOTLOADER_MAGIC 0x4c534254 + @@ -662,7 +663,7 @@ + +#ifndef __ASSEMBLY__ + -+extern unsigned long sl_status; ++extern bool slaunch_active; + +extern char txt_ap_entry[]; +extern uint32_t trampoline_gdt[]; @@ -891,7 +892,7 @@ +{ + /* Check for size overflow. */ + if (base + size < base) -+ txt_reset(SL_ERROR_INTEGER_OVERFLOW); ++ txt_reset(SLAUNCH_ERROR_INTEGER_OVERFLOW); + + /* Low range always starts at 0, so its size is also end address. */ + if (base >= os_sinit->vtd_pmr_lo_base && @@ -901,7 +902,7 @@ + if (check_high && os_sinit->vtd_pmr_hi_size != 0) { + if (os_sinit->vtd_pmr_hi_base + os_sinit->vtd_pmr_hi_size < + os_sinit->vtd_pmr_hi_size) -+ txt_reset(SL_ERROR_INTEGER_OVERFLOW); ++ txt_reset(SLAUNCH_ERROR_INTEGER_OVERFLOW); + if (base >= os_sinit->vtd_pmr_hi_base && + base + size <= os_sinit->vtd_pmr_hi_base + os_sinit->vtd_pmr_hi_size) + return 1; @@ -978,11 +979,25 @@ +void tpm_process_drtm_policy(const multiboot_info_t *mbi); + +#endif /* __ASSEMBLY__ */ +diff --git a/xen/arch/x86/include/asm/mm.h b/xen/arch/x86/include/asm/mm.h +index d723c7c38f..989fb75038 100644 +--- a/xen/arch/x86/include/asm/mm.h ++++ b/xen/arch/x86/include/asm/mm.h +@@ -98,6 +98,9 @@ + #define _PGC_need_scrub _PGC_allocated + #define PGC_need_scrub PGC_allocated + ++/* How much of the directmap is prebuilt at compile time. */ ++#define PREBUILT_MAP_LIMIT (1 << L2_PAGETABLE_SHIFT) ++ + #ifndef CONFIG_BIGMEM + /* + * This definition is solely for the use in struct page_info (and diff --git a/xen/arch/x86/include/asm/processor.h b/xen/arch/x86/include/asm/processor.h -index 8e2816fae9..dbf5a53e53 100644 +index f8f757a8a8..592ff38bae 100644 --- a/xen/arch/x86/include/asm/processor.h +++ b/xen/arch/x86/include/asm/processor.h -@@ -130,6 +130,8 @@ struct cpuinfo_x86 { +@@ -137,6 +137,8 @@ struct cpuinfo_x86 { __u32 phys_proc_id; /* package ID of each logical CPU */ __u32 cpu_core_id; /* core ID of each logical CPU*/ __u32 compute_unit_id; /* AMD compute unit ID of each logical CPU */ @@ -991,7 +1006,7 @@ unsigned short x86_clflush_size; } __cacheline_aligned; -@@ -142,6 +144,8 @@ extern struct cpuinfo_x86 boot_cpu_data; +@@ -149,6 +151,8 @@ extern struct cpuinfo_x86 boot_cpu_data; extern struct cpuinfo_x86 cpu_data[]; #define current_cpu_data cpu_data[smp_processor_id()] @@ -1034,10 +1049,10 @@ extern bool unaccounted_cpus; diff --git a/xen/arch/x86/intel_txt.c b/xen/arch/x86/intel_txt.c new file mode 100644 -index 0000000000..502a9c35d3 +index 0000000000..75584b17a2 --- /dev/null +++ b/xen/arch/x86/intel_txt.c -@@ -0,0 +1,159 @@ +@@ -0,0 +1,155 @@ +#include +#include +#include @@ -1049,17 +1064,13 @@ + +static uint64_t __initdata txt_heap_base, txt_heap_size; + -+unsigned long __initdata sl_status; -+ -+#define PREBUILT_MAP_LIMIT (1 << L2_PAGETABLE_SHIFT) ++bool __initdata slaunch_active; + +int __init map_l2(unsigned long paddr, unsigned long size) +{ + unsigned long aligned_paddr = paddr & ~((1ULL << L2_PAGETABLE_SHIFT) - 1); + unsigned long pages = ((paddr + size) - aligned_paddr); -+ pages += (1ULL << L2_PAGETABLE_SHIFT) - 1; -+ pages &= ~((1ULL << L2_PAGETABLE_SHIFT) - 1); -+ pages >>= PAGE_SHIFT; ++ pages = ROUNDUP(pages, 1ULL << L2_PAGETABLE_SHIFT) >> PAGE_SHIFT; + + if ( (aligned_paddr + pages * PAGE_SIZE) <= PREBUILT_MAP_LIMIT ) + return 0; @@ -1226,10 +1237,10 @@ } diff --git a/xen/arch/x86/numa.c b/xen/arch/x86/numa.c -index 627ae8aa95..d4f311742e 100644 +index 322157fab7..393e4108c7 100644 --- a/xen/arch/x86/numa.c +++ b/xen/arch/x86/numa.c -@@ -321,14 +321,13 @@ custom_param("numa", numa_setup); +@@ -324,14 +324,13 @@ custom_param("numa", numa_setup); /* * Setup early cpu_to_node. * @@ -1251,7 +1262,7 @@ */ void __init init_cpu_to_node(void) { -@@ -337,7 +336,7 @@ void __init init_cpu_to_node(void) +@@ -340,7 +339,7 @@ void __init init_cpu_to_node(void) for ( i = 0; i < nr_cpu_ids; i++ ) { @@ -1261,7 +1272,7 @@ continue; node = apicid < MAX_LOCAL_APIC ? apicid_to_node[apicid] : NUMA_NO_NODE; diff --git a/xen/arch/x86/platform_hypercall.c b/xen/arch/x86/platform_hypercall.c -index a7341dc3d7..d39af7c2a2 100644 +index e7deee2268..a63751fff0 100644 --- a/xen/arch/x86/platform_hypercall.c +++ b/xen/arch/x86/platform_hypercall.c @@ -588,7 +588,7 @@ ret_t do_platform_op( @@ -1274,7 +1285,7 @@ ASSERT(g_info->apic_id != BAD_APICID); g_info->flags = 0; diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c -index e05189f649..d1c233868c 100644 +index 0c00ea875d..808a15c41d 100644 --- a/xen/arch/x86/setup.c +++ b/xen/arch/x86/setup.c @@ -55,6 +55,7 @@ @@ -1285,7 +1296,7 @@ /* opt_nosmp: If true, secondary processors are ignored. */ static bool __initdata opt_nosmp; -@@ -320,7 +321,7 @@ static void __init init_idle_domain(void) +@@ -316,7 +317,7 @@ static void __init init_idle_domain(void) void srat_detect_node(int cpu) { nodeid_t node; @@ -1294,7 +1305,7 @@ node = apicid < MAX_LOCAL_APIC ? apicid_to_node[apicid] : NUMA_NO_NODE; if ( node == NUMA_NO_NODE ) -@@ -347,7 +348,7 @@ static void __init normalise_cpu_order(void) +@@ -343,7 +344,7 @@ static void __init normalise_cpu_order(void) for_each_present_cpu ( i ) { @@ -1303,7 +1314,7 @@ min_diff = min_cpu = ~0u; /* -@@ -358,12 +359,12 @@ static void __init normalise_cpu_order(void) +@@ -354,12 +355,12 @@ static void __init normalise_cpu_order(void) j < nr_cpu_ids; j = cpumask_next(j, &cpu_present_map) ) { @@ -1318,7 +1329,7 @@ { min_diff = diff; min_cpu = j; -@@ -379,9 +380,9 @@ static void __init normalise_cpu_order(void) +@@ -375,9 +376,9 @@ static void __init normalise_cpu_order(void) /* Switch the best-matching CPU with the next CPU in logical order. */ j = cpumask_next(i, &cpu_present_map); @@ -1331,7 +1342,7 @@ } } -@@ -686,7 +687,7 @@ static void __init noreturn reinit_bsp_stack(void) +@@ -682,7 +683,7 @@ static void __init noreturn reinit_bsp_stack(void) /* Update SYSCALL trampolines */ percpu_traps_init(); @@ -1340,11 +1351,21 @@ rc = setup_cpu_root_pgt(0); if ( rc ) -@@ -1140,6 +1141,17 @@ void __init noreturn __start_xen(unsigned long mbi_p) +@@ -849,9 +850,6 @@ static struct domain *__init create_dom0(const module_t *image, + return d; + } + +-/* How much of the directmap is prebuilt at compile time. */ +-#define PREBUILT_MAP_LIMIT (1 << L2_PAGETABLE_SHIFT) +- + void __init noreturn __start_xen(unsigned long mbi_p) + { + char *memmap_type = NULL; +@@ -1170,6 +1168,17 @@ void __init noreturn __start_xen(unsigned long mbi_p) #endif } -+ if ( sl_status ) ++ if ( slaunch_active ) + { + /* Prepare for TXT-related code. */ + map_txt_mem_regions(); @@ -1358,20 +1379,20 @@ /* Sanitise the raw E820 map to produce a final clean version. */ max_page = raw_max_page = init_e820(memmap_type, &e820_raw); -@@ -1158,6 +1170,12 @@ void __init noreturn __start_xen(unsigned long mbi_p) +@@ -1188,6 +1197,12 @@ void __init noreturn __start_xen(unsigned long mbi_p) /* Create a temporary copy of the E820 map. */ memcpy(&boot_e820, &e820, sizeof(e820)); + /* Process all yet unmeasured DRTM entries after E820 initialization to not + * do this while memory is uncached (too slow). This must also happen before + * fields of Multiboot modules change their format below. */ -+ if ( sl_status ) ++ if ( slaunch_active ) + tpm_process_drtm_policy(mbi); + /* Early kexec reservation (explicit static start address). */ nr_pages = 0; for ( i = 0; i < e820.nr_map; i++ ) -@@ -1871,6 +1889,7 @@ void __init noreturn __start_xen(unsigned long mbi_p) +@@ -1901,6 +1916,7 @@ void __init noreturn __start_xen(unsigned long mbi_p) */ if ( !pv_shim ) { @@ -1379,7 +1400,7 @@ for_each_present_cpu ( i ) { /* Set up cpu_to_node[]. */ -@@ -1878,6 +1897,14 @@ void __init noreturn __start_xen(unsigned long mbi_p) +@@ -1908,6 +1924,14 @@ void __init noreturn __start_xen(unsigned long mbi_p) /* Set up node_to_cpumask based on cpu_to_node[]. */ numa_add_cpu(i); @@ -1426,7 +1447,7 @@ halt(); } diff --git a/xen/arch/x86/smpboot.c b/xen/arch/x86/smpboot.c -index b46fd9ab18..455fc4fadd 100644 +index 41d9454d53..f8d1e4c76d 100644 --- a/xen/arch/x86/smpboot.c +++ b/xen/arch/x86/smpboot.c @@ -39,6 +39,7 @@ @@ -1580,7 +1601,7 @@ startup_cpu_idle_loop(); } -+static int sl_wake_aps(unsigned long trampoline_rm) ++static int slaunch_wake_aps(unsigned long trampoline_rm) +{ + struct txt_sinit_mle_data *sinit_mle = + txt_sinit_mle_data_start(__va(read_txt_reg(TXTCR_HEAP_BASE))); @@ -1619,8 +1640,8 @@ if ( tboot_in_measured_env() && !tboot_wake_ap(phys_apicid, start_eip) ) return 0; -+ if ( sl_status ) -+ return sl_wake_aps(start_eip); ++ if ( slaunch_active ) ++ return slaunch_wake_aps(start_eip); + + /* + * Use destination shorthand for broadcasting IPIs during boot. @@ -1857,9 +1878,9 @@ + /* Not really used anywhere, but set it just in case. */ + set_cpu_state(0, CPU_STATE_ONLINE); - if ( opt_xpti_hwdom || opt_xpti_domu ) - { -@@ -1281,7 +1360,7 @@ void __cpu_disable(void) + set_nr_sockets(); + +@@ -1267,7 +1346,7 @@ void __cpu_disable(void) { int cpu = smp_processor_id(); @@ -1868,7 +1889,7 @@ local_irq_disable(); clear_local_APIC(); -@@ -1306,7 +1385,7 @@ void __cpu_die(unsigned int cpu) +@@ -1292,7 +1371,7 @@ void __cpu_die(unsigned int cpu) unsigned int i = 0; enum cpu_state seen_state; @@ -1877,7 +1898,7 @@ { BUG_ON(seen_state != CPU_STATE_DYING); mdelay(100); -@@ -1391,7 +1470,7 @@ int __cpu_up(unsigned int cpu) +@@ -1377,7 +1456,7 @@ int __cpu_up(unsigned int cpu) { int apicid, ret; @@ -1886,7 +1907,7 @@ return -ENODEV; if ( (!x2apic_enabled && apicid >= APIC_ALL_CPUS) || -@@ -1407,7 +1486,7 @@ int __cpu_up(unsigned int cpu) +@@ -1393,7 +1472,7 @@ int __cpu_up(unsigned int cpu) time_latch_stamps(); @@ -1896,10 +1917,10 @@ { cpu_relax(); diff --git a/xen/arch/x86/spec_ctrl.c b/xen/arch/x86/spec_ctrl.c -index 4e53056624..3d8721d912 100644 +index 0ff3c895ac..38375f44dd 100644 --- a/xen/arch/x86/spec_ctrl.c +++ b/xen/arch/x86/spec_ctrl.c -@@ -583,7 +583,7 @@ static bool __init check_smt_enabled(void) +@@ -601,7 +601,7 @@ static bool __init check_smt_enabled(void) * has a non-zero thread id component indicates that SMT is active. */ for_each_present_cpu ( cpu ) @@ -1909,10 +1930,10 @@ return false; diff --git a/xen/arch/x86/sysctl.c b/xen/arch/x86/sysctl.c -index 716525f72f..7792ce5302 100644 +index 42dc360ad6..de9b75c1ec 100644 --- a/xen/arch/x86/sysctl.c +++ b/xen/arch/x86/sysctl.c -@@ -90,7 +90,7 @@ static long cf_check smt_up_down_helper(void *data) +@@ -59,7 +59,7 @@ static long cf_check smt_up_down_helper(void *data) for_each_present_cpu ( cpu ) { /* Skip primary siblings (those whose thread id is 0). */ @@ -1922,10 +1943,42 @@ if ( !up && core_parking_remove(cpu) ) diff --git a/xen/arch/x86/tboot.c b/xen/arch/x86/tboot.c -index fe1abfdf08..a1559a0545 100644 +index fe1abfdf08..9730f12b06 100644 --- a/xen/arch/x86/tboot.c +++ b/xen/arch/x86/tboot.c -@@ -254,9 +254,9 @@ static int mfn_in_guarded_stack(unsigned long mfn) +@@ -14,6 +14,7 @@ + #include + #include + #include ++#include + #include + + /* tboot= */ +@@ -31,23 +32,6 @@ static vmac_t frametable_mac; /* MAC for frame table during S3 */ + static uint64_t __initdata txt_heap_base, __initdata txt_heap_size; + static uint64_t __initdata sinit_base, __initdata sinit_size; + +-/* +- * TXT configuration registers (offsets from TXT_{PUB, PRIV}_CONFIG_REGS_BASE) +- */ +- +-#define TXT_PUB_CONFIG_REGS_BASE 0xfed30000 +-#define TXT_PRIV_CONFIG_REGS_BASE 0xfed20000 +- +-/* # pages for each config regs space - used by fixmap */ +-#define NR_TXT_CONFIG_PAGES ((TXT_PUB_CONFIG_REGS_BASE - \ +- TXT_PRIV_CONFIG_REGS_BASE) >> PAGE_SHIFT) +- +-/* offsets from pub/priv config space */ +-#define TXTCR_SINIT_BASE 0x0270 +-#define TXTCR_SINIT_SIZE 0x0278 +-#define TXTCR_HEAP_BASE 0x0300 +-#define TXTCR_HEAP_SIZE 0x0308 +- + #define SHA1_SIZE 20 + typedef uint8_t sha1_hash_t[SHA1_SIZE]; + +@@ -254,9 +238,9 @@ static int mfn_in_guarded_stack(unsigned long mfn) for ( i = 0; i < nr_cpu_ids; i++ ) { @@ -1939,10 +1992,10 @@ return -1; diff --git a/xen/arch/x86/tpm.c b/xen/arch/x86/tpm.c new file mode 100644 -index 0000000000..bb27123571 +index 0000000000..139aaa7a0f --- /dev/null +++ b/xen/arch/x86/tpm.c -@@ -0,0 +1,1163 @@ +@@ -0,0 +1,1164 @@ +/* + * Copyright (c) 2022 3mdeb Sp. z o.o. All rights reserved. + * @@ -1974,12 +2027,15 @@ + +#include "boot/defs.h" +#include "include/asm/intel_txt.h" -+#include +#ifdef __va +#error "__va defined in non-paged mode!" +#endif +#define __va(x) _p(x) + ++/* ++ * The implementation is necessary if compiler chooses to not use an inline ++ * builtin. ++ */ +void *memset(void *dest, int c, size_t n) +{ + uint8_t *d = dest; @@ -1989,7 +2045,6 @@ + + return dest; +} -+ +void *memcpy(void *dest, const void *src, size_t n) +{ + const uint8_t *s = src; @@ -2033,6 +2088,7 @@ + +#define swap16(x) __builtin_bswap16(x) +#define swap32(x) __builtin_bswap32(x) ++#define memset(s, c, n) __builtin_memset(s, c, n) +#define memcpy(d, s, n) __builtin_memcpy(d, s, n) + +static inline volatile uint32_t tis_read32(unsigned reg) @@ -2729,12 +2785,10 @@ +{ + uint32_t rc; + unsigned i; -+ unsigned j; + struct tpm2_log_hashes supported_hashes = {0}; + + request_locality(loc); + -+ j = 0; + for ( i = 0; i < log_hashes->count; ++i ) { + const struct tpm2_log_hash *hash = &log_hashes->hashes[i]; + if ( !tpm_supports_hash(loc, hash) ) { @@ -3107,10 +3161,10 @@ +} +#endif diff --git a/xen/arch/x86/traps.c b/xen/arch/x86/traps.c -index 7207390118..ae19056ee0 100644 +index e65cc60041..7471da5d2e 100644 --- a/xen/arch/x86/traps.c +++ b/xen/arch/x86/traps.c -@@ -616,9 +616,9 @@ void show_stack_overflow(unsigned int cpu, const struct cpu_user_regs *regs) +@@ -623,9 +623,9 @@ void show_stack_overflow(unsigned int cpu, const struct cpu_user_regs *regs) unsigned long curr_stack_base = esp & ~(STACK_SIZE - 1); unsigned long esp_top, esp_bottom; ```

sl_status is now a bool and called slaunch_active. By the way, I noticed that x86/boot/txt_early: add early TXT tests and restore MBI pointer commit adds declaration of (now) slaunch_active without definition, so Xen doesn't compile. Don't know if it's worth fixing by adding essentially empty intel_txt.c in that commit.

Also, there should be no tabulation in the changes anymore.

BeataZdunczyk commented 8 months ago

@SergiiDmytruk great news! Thank you for the comparison of the diff of diffs, it is helpful. @krystian-hebel please take a look.

krystian-hebel commented 7 months ago

sl_status is now a bool

I'm not sure how I feel about this change. To the best of my knowledge, size of bool is not defined. There is BOOL_WIDTH in limits.h since C23, I haven't found anything similar for earlier standards. This may be important because in assembly we assume that it is 32b.

On top of that, we will soon be implementing AMD counterpart, so we may need a way of differentiating between those two, which would require more bits than a single bool provides.

Xen doesn't compile. Don't know if it's worth fixing by adding essentially empty intel_txt.c in that commit.

There was a recent discussion about how Xen can't be easily bisected because of problems like this, so please fix it.

SergiiDmytruk commented 7 months ago

I'm not sure how I feel about this change. To the best of my knowledge, size of bool is not defined. There is BOOL_WIDTH in limits.h since C23, I haven't found anything similar for earlier standards. This may be important because in assembly we assume that it is 32b.

I did the change because it was suggested in the review, can revert (I even have aem-phase3-rebase-before-sl_status-rename branch locally). I think bool is 1 byte and I didn't notice that assembly treats the variable as 4 bytes. Can add a static assert for the size if bool will remain.

On top of that, we will soon be implementing AMD counterpart, so we may need a way of differentiating between those two, which would require more bits than a single bool provides.

Is there a need to pack it in a single variable? It can be slaunch_active and slaunch_type.

There was a recent discussion about how Xen can't be easily bisected because of problems like this, so please fix it.

Will do.

krystian-hebel commented 7 months ago

Is there a need to pack it in a single variable? It can be slaunch_active and slaunch_type.

Actually, this may be even better. Often tasks have to be done for all vendors, and SLRT should make it similar between them, we just have to get its address differently. In fact, everything that has to be done on AMD that isn't required for Intel is already done without slaunch_type.

SergiiDmytruk commented 7 months ago

Change of the commit which wasn't building: https://github.com/TrenchBoot/xen/compare/6bbfc3c871143bf0bc0adf778b6cd8d40b64a14c..390826d5a1650a585d02f9761d60284a8b612915 It's the same where assembly needed to be updated.

Branch difference: https://github.com/TrenchBoot/xen/compare/150a0a329a76676484c878a542089eaf2b286d6a..0c8fec9a0638f61996b6e0f35c48ef35f1837c8b

krystian-hebel commented 7 months ago

Changes look good now, and it seems to work on minimal Xen+Alpine setup, I think https://github.com/QubesOS/qubes-vmm-xen/pull/160 can be updated with those now.

SergiiDmytruk commented 7 months ago

I think QubesOS/qubes-vmm-xen#160 can be updated with those now.

To avoid a potential deadlock: I'm not the one who should be updating it, right?

SergiiDmytruk commented 7 months ago

Updated https://github.com/QubesOS/qubes-vmm-xen/pull/160 and commented on all review comments which were addressed or which I knew how to answer. There are some more left to answer, some to do (like define constants and add log messages). @krystian-hebel:

  1. You might already know an answer to https://github.com/QubesOS/qubes-vmm-xen/pull/160#discussion_r1390993407.
  2. Don't know if https://github.com/QubesOS/qubes-vmm-xen/pull/160#discussion_r1390955038 will be addressed in GRUB.
  3. https://github.com/QubesOS/qubes-vmm-xen/pull/160#discussion_r1390953518, is this why there is volatile on read_txt_reg()?
  4. Not sure if we should replace hash implementations per https://github.com/QubesOS/qubes-vmm-xen/pull/160#discussion_r1390984560
  5. Is https://github.com/QubesOS/qubes-vmm-xen/pull/160#discussion_r1390977814 safe because MTRRs are measured?
  6. Do we want to fail in cases like https://github.com/QubesOS/qubes-vmm-xen/pull/160#discussion_r1390976897? If SL isn't used, that code shouldn't be called and if it's used, conditions must always succeed? So maybe use BUG macro?
krystian-hebel commented 7 months ago

@SergiiDmytruk I replied to the first 4 comments you listed.

As for 5, it depends on what layer of security that question is about. They are measured, and they won't be less secure than it would be without TB because of that. Xen can choose to ignore it and set it's own MTRRs (perhaps it already does, haven't checked), but by restoring them at this point we get decent execution speed. Booting from UC is slooooow.

For 6, if BUG prints something that would be visible for end users, definitely use it. Otherwise this would still be silent, but most likely something else would break if TXT heap or SINIT can't be found, so maybe that would also be good reason for using BUG. If nothing else, this would at least make code clearer by having one less indentation level.

SergiiDmytruk commented 7 months ago

Thanks, @krystian-hebel.

Integrated your comments for 1 and 2 to corresponding commits.

For 6, if BUG prints something that would be visible for end users, definitely use it. Otherwise this would still be silent, but most likely something else would break if TXT heap or SINIT can't be found, so maybe that would also be good reason for using BUG. If nothing else, this would at least make code clearer by having one less indentation level.

It prints source code location and halts, so I guess it'll do.

Didn't update the PR yet, but pushed the changes. Their difference: https://github.com/TrenchBoot/xen/compare/0c8fec9a0638f61996b6e0f35c48ef35f1837c8b..37d1ba6086f103010cab4e90e83eb9f1033894ae

SergiiDmytruk commented 7 months ago

(Looks like last time I wrote a comment here I confused its preview for an actual comment and closed the browser. Not much was lost anyway.)

Last changes (constants and a comment): https://github.com/TrenchBoot/xen/compare/37d1ba6086f103010cab4e90e83eb9f1033894ae..81ac532e6c77290b6ecf6e4caf76915f350ca6d3

I think this addresses all outstanding comments on https://github.com/QubesOS/qubes-vmm-xen/pull/160 (several without reply are covered by a reply to a similar comment nearby).

krystian-hebel commented 6 months ago

Review of GRUB has been moved to https://github.com/TrenchBoot/grub/pull/16. Original approach from https://github.com/QubesOS/qubes-grub2/pull/13 proved to be impractical - review of patches to patches, indirect building, and most annoyingly, GH had problems with showing so many comments at once, some of us were greeted by unicorn of death instead of PR page: image

krystian-hebel commented 6 months ago

After review and a lot of debugging https://github.com/TrenchBoot/grub/pull/16 was finally positively tested and reviewed. New set of patches was created and sent back to ~the land of unicorns~ https://github.com/QubesOS/qubes-grub2/pull/13.