Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[NOT FOR MERGE] Intel txt aem 2.06 rebased #20

Closed
wants to merge 31 commits into from
Closed

[NOT FOR MERGE] Intel txt aem 2.06 rebased #20

wants to merge 31 commits into from

Conversation

krystian-hebel
Copy link
Collaborator

This PR is made to show and test SKINIT changes on top of what was sent to QubesOS/qubes-grub2#13. After review intel-txt-aem-2.06-rebased should be used to overwrite intel-txt-aem-2.06, and -testing can be removed.

@krystian-hebel krystian-hebel force-pushed the intel-txt-aem-2.06-rebased branch 2 times, most recently from f96f544 to 67d7167 Compare April 26, 2024 14:16
Daniel Kiper and others added 28 commits April 26, 2024 16:22
@krystian-hebel
i386/msr: Merge rdmsr.h and wrmsr.h into msr.h
0e20cf1 
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 <[email protected]>
@krystian-hebel
i386/msr: Rename grub_msr_read() and grub_msr_write()
69d6d82 
... to grub_rdmsr() and grub_wrmsr() respectively. New names are more
obvious than older ones.

Signed-off-by: Daniel Kiper <[email protected]>
@krystian-hebel
i386/msr: Extract and improve MSR support detection code
e349ad1 
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 <[email protected]>
@krystian-hebel
i386/memory: Rename PAGE_SHIFT to GRUB_PAGE_SHIFT
68e3344 
...to avoid potential conflicts and confusion.

Signed-off-by: Daniel Kiper <[email protected]>
@krystian-hebel
i386/memory: Rename PAGE_SIZE to GRUB_PAGE_SIZE and make it global
89a387c 
Subsequent patches will use that constant.

Signed-off-by: Daniel Kiper <[email protected]>
@krystian-hebel
i386/memory: Define GRUB_PAGE_MASK constant and GRUB_PAGE_{UP,DOWN} m…
a65ac35 
…acros

Subsequent patches will use those macros and constant.

Signed-off-by: Krystian Hebel <[email protected]>
@krystian-hebel
mmap: Add grub_mmap_get_lowest() and grub_mmap_get_highest()
19c1993 
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 <[email protected]>
@krystian-hebel
i386/tpm: Rename tpm module to tpm_verifier
4213c39 
...to avoid naming collision with TPM TIS and CRB driver introduced
by subsequent patch.

Signed-off-by: Daniel Kiper <[email protected]>
@krystian-hebel
i386/tpm: Add TPM TIS and CRB driver
137e1c0 
It will be used by Intel TXT secure launcher introduced
by subsequent patches.

Signed-off-by: Daniel Kiper <[email protected]>
Signed-off-by: Krystian Hebel <[email protected]>
@rossphilipson @krystian-hebel
i386/slaunch: Add basic platform support for secure launch
7d89e5d 
Some of the commands declared in header files will be implemented in
the follow-up commits.

Signed-off-by: Ross Philipson <[email protected]>
Signed-off-by: Daniel Kiper <[email protected]>
Signed-off-by: Krystian Hebel <[email protected]>
@rossphilipson @krystian-hebel
i386/txt: Initial commit of the Secure Launch Resource Table (SLRT)
1ec91a6 
Provide definitions of structures and basic functions for constructing
and parsing of SLRT.

Signed-off-by: Ross Philipson <[email protected]>
Signed-off-by: Sergii Dmytruk <[email protected]>
@rossphilipson @krystian-hebel
i386/txt: Add Intel TXT definitions header file
d8a9355 
Signed-off-by: Ross Philipson <[email protected]>
Signed-off-by: Daniel Kiper <[email protected]>
@rossphilipson @krystian-hebel
i386/txt: Add Intel TXT core implementation
9736d05 
Signed-off-by: Ross Philipson <[email protected]>
Signed-off-by: Daniel Kiper <[email protected]>
Signed-off-by: Michał Żygowski <[email protected]>
Signed-off-by: Krystian Hebel <[email protected]>
@rossphilipson @krystian-hebel
i386/txt: Add Intel TXT ACM module support
f00c4fc 
Signed-off-by: Ross Philipson <[email protected]>
Signed-off-by: Daniel Kiper <[email protected]>
Signed-off-by: Krystian Hebel <[email protected]>
@rossphilipson @krystian-hebel
i386/txt: Add Intel TXT verification routines
fdf9662 
Signed-off-by: Ross Philipson <[email protected]>
Signed-off-by: Daniel Kiper <[email protected]>
Signed-off-by: Krystian Hebel <[email protected]>
@miczyg1 @krystian-hebel
i386/txt: Initialize TPM 1.2 event log in TXT heap
b1adb3a 
Signed-off-by: Michał Żygowski <[email protected]>
Signed-off-by: Krystian Hebel <[email protected]>
@rossphilipson @krystian-hebel
i386/txt: switch to using Secure Launch Resource Table interface
e2fcca0 
Signed-off-by: Ross Philipson <[email protected]>
Signed-off-by: Sergii Dmytruk <[email protected]>
@rossphilipson @krystian-hebel
i386/slaunch: Add secure launch framework and commands
1ee2f71 
Signed-off-by: Ross Philipson <[email protected]>
Signed-off-by: Daniel Kiper <[email protected]>
Signed-off-by: Krystian Hebel <[email protected]>
@SergiiDmytruk @krystian-hebel
multiboot: make GRUB_MULTIBOOT(make_mbi) return MBI's size
e360ef6 
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 <[email protected]>
@miczyg1 @krystian-hebel
multiboot2: Implement TXT slaunch support
594fe47 
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 <[email protected]>
Signed-off-by: Michał Żygowski <[email protected]>
Signed-off-by: Tomasz Żyjewski <[email protected]>
Signed-off-by: Krystian Hebel <[email protected]>
@SergiiDmytruk @krystian-hebel
Add GitHub actions for building QubesOS packages
c6bd2ee 
Signed-off-by: Sergii Dmytruk <[email protected]>
@SergiiDmytruk @krystian-hebel
.github/workflows/build.yml: run when a tag is pushed
60d94af 
Signed-off-by: Sergii Dmytruk <[email protected]>
@SergiiDmytruk @krystian-hebel
.github/workflows/build.yml: drop pull_request_target
8be0756 
This still runs CI twice in a PR.

Signed-off-by: Sergii Dmytruk <[email protected]>
@krystian-hebel
.github/workflows/build.yml: bump base-commit
a485120 
After QubesOS/qubes-grub2#13 got merged, some
of the commits were duplicated, causing build system to fail. Skip
those commits and add the rest starting at the next available
patch-start number.

Signed-off-by: Krystian Hebel <[email protected]>
@krystian-hebel
i386/skinit: Add AMD SKINIT implementation
61e3170 
Signed-off-by: Krystian Hebel <[email protected]>
Signed-off-by: Sergii Dmytruk <[email protected]>
@krystian-hebel
i386/slaunch: Add support for AMD SKINIT
c9a17ff 
This updates `struct grub_slr_entry_dl_info`:
 * by adding `dlme_base` and `dlme_size` which weren't necessary for
   Intel TXT because the same information is passed via OS2SINIT data
 * by changing `dlme_entry` from `grub_uint64_t` to `grub_uint32_t`
   because that should be enough for an offset

Signed-off-by: Krystian Hebel <[email protected]>
Signed-off-by: Sergii Dmytruk <[email protected]>
@krystian-hebel
multiboot2: support AMD SKINIT
8dc7c67 
Signed-off-by: Krystian Hebel <[email protected]>
Signed-off-by: Sergii Dmytruk <[email protected]>
@SergiiDmytruk @krystian-hebel
i386/slaunch: improve error recovery of "slaunch_module" command
b1083c1 
Update `slaunch_module` global only if new module passed all checks.

Closes #11.

Signed-off-by: Sergii Dmytruk <[email protected]>
@SergiiDmytruk @krystian-hebel
i386/txt: do not update MLE header
99b6c86 
It's measured and we better measure the binary as is without changing it
on the fly.

Signed-off-by: Sergii Dmytruk <[email protected]>
@SergiiDmytruk @krystian-hebel
i386/skinit: don't allocate TPM event log low
f6dfae5 
Do allocation in the same way as it's done for TXT (preferring high
addresses), otherwise Xen loads Dom0 kernel over the TPM even log and
Linux ends up panicking when it detects conflict with e820 map in
Xen-specific code (why wouldn't Xen check it?  who knows).

Signed-off-by: Sergii Dmytruk <[email protected]>
@krystian-hebel
Copy link
Collaborator Author

krystian-hebel commented Apr 26, 2024 •
edited
Loading

To see real delta between those branches: https://github.com/TrenchBoot/grub/compare/intel-txt-aem-2.06-testing..intel-txt-aem-2.06-rebased, changes presented by GH in Files changed aren't exact because I had to move commits that added CI on top of base-commit.

SergiiDmytruk
SergiiDmytruk reviewed Apr 27, 2024
View reviewed changes
Copy link
Member

@SergiiDmytruk SergiiDmytruk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looked at the changes after not working with them for a few weeks and found some things that could be improved while doing the rebase. Also ran openQA test on Supermicro M11 with the artifacts produced by CI and it passed successfully.

grub-core/loader/i386/skinit.c
/* Contrary to the TXT, on AMD we do not have vendor-provided blobs in
* reserved memory, we are using normal RAM */
err = grub_relocator_alloc_chunk_align (rel, &ch, 0,
0xffffffff - GRUB_SKINIT_SLB_SIZE,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess someone during future review could point out that UP_TO_TOP32() should be used here as above, could update right away.

grub-core/loader/i386/skinit.c
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This could be squashed into i386/skinit: Add AMD SKINIT implementation.

grub-core/loader/i386/skinit.c
0xffffffff - GRUB_SKINIT_SLB_SIZE,
GRUB_SKINIT_SLB_SIZE,
GRUB_SKINIT_SLB_ALIGN,
GRUB_RELOCATOR_PREFERENCE_LOW, 1);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Given that GRUB_RELOCATOR_PREFERENCE_LOW caused us problems for TPM log, any benefits of using GRUB_RELOCATOR_PREFERENCE_HIGH here as well? In that case 0x1000000 was used for a base, so maybe there is no danger here assuming availability of aligned 64KiB below 16MiB boundary.

grub-core/loader/i386/skinit.c

grub_dprintf ("slaunch", "broadcasting INIT\r\n");
*apic = 0x000c0500; // INIT, all excluding self
grub_dprintf ("slaunch", "grub_tpm_relinquish_locality\r\n");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IIRC this print might be helpful for the delay that it introduces, but the message could probably be edited to say something like "sent INIT broadcasts".

grub-core/loader/i386/slaunch.c
if (slp == SLP_NONE)
return grub_error (GRUB_ERR_BAD_ARGUMENT, N_("secure launch not enabled"));

if (slp > SLP_AMD_SKINIT)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There should probably be SLP_END with the slp >= SLP_END check here, because what I've done doesn't look nice.

include/grub/i386/skinit.h
#define GRUB_SKINIT_SLB_SIZE 0x10000
#define GRUB_SKINIT_SLB_ALIGN 0x10000

#ifndef ASM_FILE
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this file is never included in an asm-file (directly or indirectly), so this check and defining constants after includes are unnecessary.

@miczyg1
grub-core/loader/multiboot_elfxx.c: Fix calculating MLE size
85ccfdc 
To avoid mismatch between OS SINIT data MLE size and MLE size from
MLE header, take the mle_size from MLE header and do not align it
to PMR size, which is 2MB.

Signed-off-by: Michał Żygowski <[email protected]>
@SergiiDmytruk
Copy link
Member

SergiiDmytruk commented Aug 20, 2024 •
edited
Loading

Cleanup: @krystian-hebel, I think the comments were basically taken care of in tb-2.12-57-linux-amd where it's needed for upstreaming, so the branches can be renamed as stated in the OP.

@SergiiDmytruk SergiiDmytruk mentioned this pull request Aug 20, 2024
Open
@krystian-hebel krystian-hebel deleted the branch intel-txt-aem-2.06-testing August 26, 2024 11:53
@krystian-hebel
Copy link
Collaborator Author

@krystian-hebel, I think the comments were basically taken care of in tb-2.12-57-linux-amd where it's needed for upstreaming, so the branches can be renamed as stated in the OP.

Agreed.

@krystian-hebel krystian-hebel deleted the intel-txt-aem-2.06-rebased branch August 26, 2024 11:55
@krystian-hebel krystian-hebel restored the intel-txt-aem-2.06-rebased branch August 26, 2024 12:03
@krystian-hebel
Copy link
Collaborator Author

(had to restore branch to switch target on #23, removing again)

@krystian-hebel krystian-hebel deleted the intel-txt-aem-2.06-rebased branch August 26, 2024 12:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@SergiiDmytruk SergiiDmytruk SergiiDmytruk left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@krystian-hebel @SergiiDmytruk @rossphilipson @miczyg1