Intel txt aem 2.06 testing #16

krystian-hebel · 2024-01-27T11:16:15Z

No description provided.

It does not make sense to have separate headers for separate static functions. Additionally, we have to add some constants with MSR addresses in subsequent patches. So, make one common place to store them. Signed-off-by: Daniel Kiper <daniel.kiper@oracle.com>

... to grub_rdmsr() and grub_wrmsr() respectively. New names are more obvious than older ones. Signed-off-by: Daniel Kiper <daniel.kiper@oracle.com>

Currently rdmsr and wrmsr commands have own MSR support detection code. This code is the same. So, it is duplicated. Additionally, this code cannot be reused by others. Hence, extract this code to a function and make it public. By the way, improve a code a bit. Additionally, use GRUB_ERR_BAD_DEVICE instead of GRUB_ERR_BUG to signal an error because errors encountered by this new routine are not bugs. Signed-off-by: Daniel Kiper <daniel.kiper@oracle.com>

...to avoid potential conflicts and confusion. Signed-off-by: Daniel Kiper <daniel.kiper@oracle.com>

Subsequent patches will use that constant. Signed-off-by: Daniel Kiper <daniel.kiper@oracle.com>

…acros Subsequent patches will use those macros and constant. Signed-off-by: Krystian Hebel <krystian.hebel@3mdeb.com>

The functions calculate lowest and highest available RAM addresses respectively. Both functions are needed to calculate PMR boundaries for Intel TXT secure launcher introduced by subsequent patches. Signed-off-by: Daniel Kiper <daniel.kiper@oracle.com>

...to avoid naming collision with TPM TIS and CRB driver introduced by subsequent patch. Signed-off-by: Daniel Kiper <daniel.kiper@oracle.com>

It will be used by Intel TXT secure launcher introduced by subsequent patches. Signed-off-by: Daniel Kiper <daniel.kiper@oracle.com> Signed-off-by: Krystian Hebel <krystian.hebel@3mdeb.com>

DemiMarie

Please rename all read and write functions to include the number of bits they access, so read64 instead of readq (and similar for other functions). Also please document the magic numbers used in the SLRT calls

docs/grub.texi

grub-core/lib/i386/relocator32.S

DemiMarie · 2024-02-04T20:18:12Z

grub-core/loader/i386/linux.c

+      prot_size = ALIGN_UP (prot_size, GRUB_TXT_PMR_ALIGN);
+
+      if (prot_size > GRUB_TXT_MLE_MAX_SIZE)
+	{
+	  err = GRUB_ERR_OUT_OF_RANGE;
+	  goto fail;
+	}


Should these be reordered to make sure ALIGN_UP cannot overflow?

DemiMarie · 2024-02-04T20:35:24Z

grub-core/loader/i386/linux.c

+      /* TODO the initrd image and size can have hi bits but for now assume always < 4G */
+      grub_slaunch_add_slrt_policy_entry (17,
+                                          GRUB_SLR_ET_RAMDISK,
+                                          /*flags=*/0,
+                                          boot_params->ramdisk_image,
+                                          boot_params->ramdisk_size,
+                                          "Measured Kernel initrd");


Should this be checked?

DemiMarie · 2024-02-04T20:35:57Z

grub-core/loader/i386/linux.c

  if (! ctx.real_mode_target)
    return grub_error (GRUB_ERR_OUT_OF_MEMORY, "cannot allocate real mode pages");

  {
    grub_relocator_chunk_t ch;
    grub_size_t sz;

-    if (grub_add (ctx.real_size, efi_mmap_size, &sz))
+    if (grub_add (ctx.real_size, efi_mmap_size + ap_wake_block_size, &sz))


Use grub_add() here too?

grub-core/loader/i386/txt/verify.c

grub-core/loader/multiboot_elfxx.c

DemiMarie · 2024-02-05T22:19:05Z

grub-core/loader/multiboot_mbi2.c

+
+  for (i = 0, cur = modules; i < modcnt; i++, cur = cur->next)
+    {
+      grub_slaunch_add_slrt_policy_entry (17,


Where does 17 come from?

grub-core/loader/i386/txt/txt.c

grub-core/loader/i386/slaunch.c

grub-core/loader/i386/linux.c

DemiMarie · 2024-02-08T22:01:39Z

grub-core/loader/i386/txt/acmod.c

+  info_table_size = __builtin_offsetof (struct grub_txt_acm_info_table, length)
+		    + sizeof(ptr->length);


I thought stddef.h is available in freestanding code. On my system (GCC with Fedora 39) it is provided by the compiler, not libc, and it only provides definitions for macros, structs, and typedefs that are defined by the compiler.

DemiMarie · 2024-02-08T22:02:50Z

grub-core/loader/i386/txt/acmod.c

+  return proc_id_list;
+}
+
+static int


typedef _Bool bool? That said, I thought <stdbool.h> was available even in freestanding mode. You don’t get a libc in freestanding mode, but <stdbool.h> is the compiler’s job to provide, not libc’s.

grub-core/loader/i386/txt/acmod.c

grub-core/loader/i386/txt/verify.c

include/grub/i386/linux.h

include/grub/slr_table.h

DemiMarie · 2024-02-08T22:32:16Z

include/grub/slr_table.h

+  /* Can read the size field of next entry? */
+  if ( grub_add (addr, sizeof(*next), &addr) )
+    return NULL;
+
+  /* Does next element's header fit within the table? */
+  if (addr >= grub_slr_end_of_entries (table))
+    return NULL;
+
+  next = (struct grub_slr_entry_hdr *) (addr - sizeof(*next));
+
+  /* Does next element fit within the table? */
+  if (grub_slr_end_of_entries (table) - (addr - sizeof(*next)) < next->size)
+    return NULL;


Suggested change

/* Can read the size field of next entry? */

if ( grub_add (addr, sizeof(*next), &addr) )

return NULL;

/* Does next element's header fit within the table? */

if (addr >= grub_slr_end_of_entries (table))

return NULL;

next = (struct grub_slr_entry_hdr *) (addr - sizeof(*next));

/* Does next element fit within the table? */

if (grub_slr_end_of_entries (table) - (addr - sizeof(*next)) < next->size)

return NULL;

/* Does next element's header fit within the table? */

if (addr >= grub_slr_end_of_entries (table) - sizeof(*next))

return NULL;

next = (struct grub_slr_entry_hdr *) addr);

/* Does next element fit within the table? */

if (grub_slr_end_of_entries (table) - addr < next->size)

return NULL;

/* Does next element include a header? */

if (next->size < sizeof(*next))

return NULL;

krystian-hebel

@DemiMarie could you mark discussions as resolved wherever you're happy with the change?

grub-core/loader/i386/txt/acmod.c

grub-core/loader/i386/linux.c

krystian-hebel · 2024-02-09T14:57:06Z

grub-core/loader/i386/txt/acmod.c

+  info_table_size = __builtin_offsetof (struct grub_txt_acm_info_table, length)
+		    + sizeof(ptr->length);


You're right, I've mistook it with stdint.h, which in theory is available since C99, but compilers still have problems with it.

In any case, GRUB seems to avoid standard headers in core code, except for stdarg.h. They are used in OS utilities and libraries, but not in the bootloader itself, so I don't feel comfortable including it just for this.

krystian-hebel · 2024-02-09T14:58:12Z

grub-core/loader/i386/txt/acmod.c

+  return proc_id_list;
+}
+
+static int


Same reasoning as above.

include/grub/i386/linux.h

include/grub/slr_table.h

DemiMarie

Looks good!

docs/grub.texi

grub-core/lib/i386/relocator32.S

include/grub/slr_table.h

DemiMarie · 2024-02-09T19:01:51Z

include/grub/slr_table.h

+  if (!entry) /* Start from the beginning */
+    entry = (struct grub_slr_entry_hdr *)((grub_uint8_t *) table + sizeof(*table));


That falls into the “input is known trusted here” case.

DemiMarie · 2024-02-09T19:02:58Z

include/grub/i386/msr.h

+#define GRUB_MSR_EFER_LME		(1<<8)     /* Enable Long Mode/IA-32e */
+#define GRUB_MSR_EFER_LMA		(1<<10)    /* Long Mode/IA-32e Active */


For some reason I could not find this in my copy, probably because I was not looking at the right place. Consider this resolved.

DemiMarie · 2024-02-09T19:46:31Z

grub-core/loader/i386/slaunch.c

+GRUB_MOD_INIT (slaunch)
+{
+  cmd_slaunch = grub_register_command ("slaunch", grub_cmd_slaunch,
+				       NULL, N_("Enable secure launcher"));
+  cmd_slaunch_module = grub_register_command ("slaunch_module", grub_cmd_slaunch_module,
+					      NULL, N_("Load secure launcher module from file"));
+  cmd_slaunch_state = grub_register_command ("slaunch_state", grub_cmd_slaunch_state,
+					     NULL, N_("Display secure launcher state"));


This can be resolved.

DemiMarie · 2024-02-09T19:47:00Z

grub-core/loader/i386/txt/acmod.c

+  info_table_size = __builtin_offsetof (struct grub_txt_acm_info_table, length)
+		    + sizeof(ptr->length);


That’s fine, you can resolve this.

DemiMarie · 2024-02-09T19:47:20Z

grub-core/loader/i386/txt/acmod.c

+  return proc_id_list;
+}
+
+static int


I see. Not worth blocking merge.

grub-core/loader/i386/txt/acmod.c

include/grub/i386/slaunch.h

krystian-hebel · 2024-02-13T17:04:54Z

grub-core/loader/i386/txt/verify.c

+                 N_("TXT heap is not configured correctly"));
+
+  bios_size = grub_txt_bios_data_size (txt_heap);
+  /* We support versions >= 4, but bios_data->mle_flags is in versions >= 5. */


Great, SNB_IVB_SINIT_20190708_PW.bin is version 4 but it has mle_flags. Fu Thanks, Intel.

Some of the commands declared in header files will be implemented in the follow-up commits. Signed-off-by: Ross Philipson <ross.philipson@oracle.com> Signed-off-by: Daniel Kiper <daniel.kiper@oracle.com> Signed-off-by: Krystian Hebel <krystian.hebel@3mdeb.com>

DemiMarie · 2024-02-13T22:01:54Z

@DemiMarie could you mark discussions as resolved wherever you're happy with the change?

GitHub isn’t allowing it, sadly.

DemiMarie

Looks okay

krystian-hebel · 2024-02-20T19:07:10Z

Those two sneaky errors were breaking boot on Optiplex... I'll redo the same tests on HP just in case before clearing debug output and finally preparing patches for qubes-grub2

Provide definitions of structures and basic functions for constructing and parsing of SLRT. Signed-off-by: Ross Philipson <ross.philipson@oracle.com> Signed-off-by: Sergii Dmytruk <sergii.dmytruk@3mdeb.com>

Signed-off-by: Ross Philipson <ross.philipson@oracle.com> Signed-off-by: Daniel Kiper <daniel.kiper@oracle.com>

Signed-off-by: Ross Philipson <ross.philipson@oracle.com> Signed-off-by: Daniel Kiper <daniel.kiper@oracle.com> Signed-off-by: Michał Żygowski <michal.zygowski@3mdeb.com> Signed-off-by: Krystian Hebel <krystian.hebel@3mdeb.com>

Signed-off-by: Ross Philipson <ross.philipson@oracle.com> Signed-off-by: Daniel Kiper <daniel.kiper@oracle.com> Signed-off-by: Krystian Hebel <krystian.hebel@3mdeb.com>

Signed-off-by: Michał Żygowski <michal.zygowski@3mdeb.com> Signed-off-by: Krystian Hebel <krystian.hebel@3mdeb.com>

Signed-off-by: Ross Philipson <ross.philipson@oracle.com> Signed-off-by: Sergii Dmytruk <sergii.dmytruk@3mdeb.com>

Signed-off-by: Ross Philipson <ross.philipson@oracle.com> Signed-off-by: Daniel Kiper <daniel.kiper@oracle.com> Signed-off-by: Krystian Hebel <krystian.hebel@3mdeb.com>

GRUB_MULTIBOOT(get_mbi_size) doesn't look like an accurate source of the final size, more like a minimal memory buffer size. Signed-off-by: Sergii Dmytruk <sergii.dmytruk@3mdeb.com>

The code makes sure that MBI entry goes first in DRTM, so the payload can measure it first on launch. SLRT table is allocated on the heap first, size for it is reserved inside TXT heap by TXT code and data is later copied into its final place. Signed-off-by: Sergii Dmytruk <sergii.dmytruk@3mdeb.com> Signed-off-by: Michał Żygowski <michal.zygowski@3mdeb.com> Signed-off-by: Tomasz Żyjewski <tomasz.zyjewski@3mdeb.com> Signed-off-by: Krystian Hebel <krystian.hebel@3mdeb.com>

krystian-hebel · 2024-02-20T20:19:58Z

No regression on HP, debug output cleared, fixes squashed to appropriate commits. Running per-commit build test (for bisectability) and will send patches tomorrow.

Reviewed on TrenchBoot/grub#16 Signed-off-by: Krystian Hebel <krystian.hebel@3mdeb.com>

marmarek · 2024-02-22T17:07:36Z

@DemiMarie can you send me signed message with commit ID which you approve (can be pasted clearsigned here, or via email)?

SergiiDmytruk · 2024-08-20T21:33:18Z

Cleanup: @krystian-hebel, so this got upstreamed and we won't need this PR and branches anymore?

krystian-hebel · 2024-08-26T11:53:09Z

@krystian-hebel, so this got upstreamed and we won't need this PR and branches anymore?

FSVO upstreamed, it was implemented in Qubes GRUB repo. Can be closed.

krystian-hebel force-pushed the intel-txt-aem-2.06-testing branch 3 times, most recently from fa228ba to 2a10c64 Compare January 29, 2024 15:55

marmarek mentioned this pull request Jan 31, 2024

Inteltxt support QubesOS/qubes-grub2#13

Merged

Daniel Kiper and others added 9 commits January 31, 2024 12:56

i386/msr: Rename grub_msr_read() and grub_msr_write()

462af28

... to grub_rdmsr() and grub_wrmsr() respectively. New names are more obvious than older ones. Signed-off-by: Daniel Kiper <daniel.kiper@oracle.com>

i386/memory: Rename PAGE_SHIFT to GRUB_PAGE_SHIFT

7d5c059

...to avoid potential conflicts and confusion. Signed-off-by: Daniel Kiper <daniel.kiper@oracle.com>

i386/memory: Rename PAGE_SIZE to GRUB_PAGE_SIZE and make it global

881d587

Subsequent patches will use that constant. Signed-off-by: Daniel Kiper <daniel.kiper@oracle.com>

i386/memory: Define GRUB_PAGE_MASK constant and GRUB_PAGE_{UP,DOWN} m…

d9f97aa

…acros Subsequent patches will use those macros and constant. Signed-off-by: Krystian Hebel <krystian.hebel@3mdeb.com>

i386/tpm: Rename tpm module to tpm_verifier

01e5b00

...to avoid naming collision with TPM TIS and CRB driver introduced by subsequent patch. Signed-off-by: Daniel Kiper <daniel.kiper@oracle.com>

i386/tpm: Add TPM TIS and CRB driver

c51d252

It will be used by Intel TXT secure launcher introduced by subsequent patches. Signed-off-by: Daniel Kiper <daniel.kiper@oracle.com> Signed-off-by: Krystian Hebel <krystian.hebel@3mdeb.com>

krystian-hebel force-pushed the intel-txt-aem-2.06-testing branch from 2a10c64 to 66004ae Compare January 31, 2024 11:57

krystian-hebel changed the base branch from intel-txt-aem-2.06 to intel-txt-aem-base January 31, 2024 11:57

DemiMarie suggested changes Feb 5, 2024

View reviewed changes

krystian-hebel mentioned this pull request Feb 6, 2024

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

Open

DemiMarie reviewed Feb 8, 2024

View reviewed changes

krystian-hebel commented Feb 9, 2024

View reviewed changes

DemiMarie approved these changes Feb 9, 2024

View reviewed changes

krystian-hebel commented Feb 13, 2024

View reviewed changes

krystian-hebel force-pushed the intel-txt-aem-2.06-testing branch from efdb40d to d57eb43 Compare February 13, 2024 19:32

DemiMarie approved these changes Feb 16, 2024

View reviewed changes

DemiMarie approved these changes Feb 19, 2024

View reviewed changes

DemiMarie reviewed Feb 20, 2024

View reviewed changes

rossphilipson added 2 commits February 20, 2024 21:06

i386/txt: Initial commit of the Secure Launch Resource Table (SLRT)

6833012

Provide definitions of structures and basic functions for constructing and parsing of SLRT. Signed-off-by: Ross Philipson <ross.philipson@oracle.com> Signed-off-by: Sergii Dmytruk <sergii.dmytruk@3mdeb.com>

i386/txt: Add Intel TXT definitions header file

e49d73b

Signed-off-by: Ross Philipson <ross.philipson@oracle.com> Signed-off-by: Daniel Kiper <daniel.kiper@oracle.com>

rossphilipson and others added 8 commits February 20, 2024 21:06

i386/txt: Add Intel TXT core implementation

4597667

Signed-off-by: Ross Philipson <ross.philipson@oracle.com> Signed-off-by: Daniel Kiper <daniel.kiper@oracle.com> Signed-off-by: Michał Żygowski <michal.zygowski@3mdeb.com> Signed-off-by: Krystian Hebel <krystian.hebel@3mdeb.com>

i386/txt: Add Intel TXT ACM module support

0ce784f

Signed-off-by: Ross Philipson <ross.philipson@oracle.com> Signed-off-by: Daniel Kiper <daniel.kiper@oracle.com> Signed-off-by: Krystian Hebel <krystian.hebel@3mdeb.com>

i386/txt: Add Intel TXT verification routines

b3a5c6f

Signed-off-by: Ross Philipson <ross.philipson@oracle.com> Signed-off-by: Daniel Kiper <daniel.kiper@oracle.com> Signed-off-by: Krystian Hebel <krystian.hebel@3mdeb.com>

i386/txt: Initialize TPM 1.2 event log in TXT heap

f13b0f9

Signed-off-by: Michał Żygowski <michal.zygowski@3mdeb.com> Signed-off-by: Krystian Hebel <krystian.hebel@3mdeb.com>

i386/txt: switch to using Secure Launch Resource Table interface

f5ce8a7

Signed-off-by: Ross Philipson <ross.philipson@oracle.com> Signed-off-by: Sergii Dmytruk <sergii.dmytruk@3mdeb.com>

i386/slaunch: Add secure launch framework and commands

cf61e4e

Signed-off-by: Ross Philipson <ross.philipson@oracle.com> Signed-off-by: Daniel Kiper <daniel.kiper@oracle.com> Signed-off-by: Krystian Hebel <krystian.hebel@3mdeb.com>

multiboot: make GRUB_MULTIBOOT(make_mbi) return MBI's size

4e6cb41

GRUB_MULTIBOOT(get_mbi_size) doesn't look like an accurate source of the final size, more like a minimal memory buffer size. Signed-off-by: Sergii Dmytruk <sergii.dmytruk@3mdeb.com>

krystian-hebel force-pushed the intel-txt-aem-2.06-testing branch from 9dc582a to 58c9a06 Compare February 20, 2024 20:07

krystian-hebel added a commit to 3mdeb/qubes-grub2 that referenced this pull request Feb 21, 2024

TXT support: update after review

98414de

Reviewed on TrenchBoot/grub#16 Signed-off-by: Krystian Hebel <krystian.hebel@3mdeb.com>

DemiMarie approved these changes Feb 22, 2024

View reviewed changes

SergiiDmytruk mentioned this pull request Aug 20, 2024

Clean up branches in TrenchBoot repositories TrenchBoot/trenchboot-issues#48

Open

krystian-hebel closed this Aug 26, 2024

krystian-hebel deleted the intel-txt-aem-2.06-testing branch August 26, 2024 11:53

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Intel txt aem 2.06 testing #16

Intel txt aem 2.06 testing #16

krystian-hebel commented Jan 27, 2024

DemiMarie left a comment

DemiMarie Feb 4, 2024

DemiMarie Feb 4, 2024

DemiMarie Feb 4, 2024

DemiMarie Feb 5, 2024

DemiMarie Feb 8, 2024

DemiMarie Feb 8, 2024

DemiMarie Feb 8, 2024

krystian-hebel left a comment

krystian-hebel Feb 9, 2024

krystian-hebel Feb 9, 2024

DemiMarie left a comment

DemiMarie Feb 9, 2024

DemiMarie Feb 9, 2024

DemiMarie Feb 9, 2024

DemiMarie Feb 9, 2024

DemiMarie Feb 9, 2024

krystian-hebel Feb 13, 2024

DemiMarie commented Feb 13, 2024

DemiMarie left a comment

krystian-hebel commented Feb 20, 2024

krystian-hebel commented Feb 20, 2024

marmarek commented Feb 22, 2024

SergiiDmytruk commented Aug 20, 2024 •

edited

Loading

krystian-hebel commented Aug 26, 2024

		info_table_size = __builtin_offsetof (struct grub_txt_acm_info_table, length)
		+ sizeof(ptr->length);

		if (!entry) /* Start from the beginning */
		entry = (struct grub_slr_entry_hdr )((grub_uint8_t ) table + sizeof(*table));

		#define GRUB_MSR_EFER_LME (1<<8) /* Enable Long Mode/IA-32e */
		#define GRUB_MSR_EFER_LMA (1<<10) /* Long Mode/IA-32e Active */

Intel txt aem 2.06 testing #16

Intel txt aem 2.06 testing #16

Conversation

krystian-hebel commented Jan 27, 2024

DemiMarie 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

krystian-hebel left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DemiMarie 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

DemiMarie commented Feb 13, 2024

DemiMarie left a comment

Choose a reason for hiding this comment

krystian-hebel commented Feb 20, 2024

krystian-hebel commented Feb 20, 2024

marmarek commented Feb 22, 2024

SergiiDmytruk commented Aug 20, 2024 • edited Loading

krystian-hebel commented Aug 26, 2024

SergiiDmytruk commented Aug 20, 2024 •

edited

Loading