Inteltxt support #13
Inteltxt support #13
Conversation
|
@marmarek could you please review these changes? |
|
Early comments: 1109-efi-Return-grub_efi_status_t-from-grub_efi_get_varia.patch
1121-grub-core-loader-fix-compilation-issues.patch:
1122-grub-core-loader-multiboot_mbi2.c-remove-wrong-incre.patch:
1123-grub-core-loader-multiboot_mbi2.c-fix-warnings.patch:
1123-grub-core-loader-multiboot_mbi2.c-fix-warnings.patch:
1128-grub-core-loader-multiboot_mbi2.c-fix-MLE-header-off.patch:
1129-grub-core-loader-multiboot_mbi2.c-do-not-make-mle_hd.patch:
1130-grub-core-loader-multiboot-fix-virtual-vs-physical-e.patch:
1132-grub-core-loader-multiboot-fix-searching-for-MLE-hea.patch:
1134-grub-core-loader-i386-code-cleanup.patch:
1136-Update-grub-core-loader-i386-txt-txt.c.patch:
1137-grub-core-loader-i386-txt-txt.c-simplify-IF.patch:
|
Set of patches which adds TPM1.2 support for Intel TXT in GRUB2 for TrenchBoot. This is necessary to create Proof of Concept for TrenchBoot Anti Evil Maid for QubesOS. The TrenchBoot support hasn't been implemented and verified with TPM 1.2 on Intel TXT path. This changes ensures the TPM 1.2 is also supported for older Intel hardware with Intel TXT. Signed-off-by: Tomasz Żyjewski <[email protected]>
TomaszAIR
force-pushed
the
inteltxt-support
branch
from
a9b0878 to
3e5c333
Compare
|
@marmarek thanks to @krystian-hebel we reworked the patches and pushed the update. CI builds them correctly. Could you please review? |
|
1106-mmap-Add-grub_mmap_get_lowest-and-grub_mmap_get_high.patch
1108-i386-tpm-Add-TPM-TIS-and-CRB-driver.patch
1109-i386-slaunch-Add-basic-platform-support-for-secure-l.patch
1111-i386-txt-Add-Intel-TXT-core-implementation.patch
The subsequent patches are split a bit weird – I don't think doing it file-by-file has any merit, but it's not a dealbreaker. If anything, it's upstream that'll complain. I'll take a close look at how this code relates to the TXT specification next. |
For MMIO functions, at least Linux and Xen uses |
|
The |
|
Looks like I messed up my grep for that. |
|
The Intel TXT Software Development Guide, Section 2.2.2 "Detection of Previous Errors" stipulates that the software checks |
|
You don't seem to be setting the new SLP type parameter for the relocator in the |
Indeed. There was at least one generation of early VT-x hardware (therefore == SMX hardware too) which failed to lock FEATURE_CONTROL. |
Actually, as long as we keep this as a patchset, this approach can silently break for new uses of |
| + 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_("Secure launcher module command")); |
There was a problem hiding this comment.
This description just rephrases the name of the command. How about "Load secure launcher module from file". Depending on how long of a description we want, we could even mention "(chipset ACM for Intel TXT)".
There was a problem hiding this comment.
I agree with first part, but since we plan to add support for AMD (and perhaps ARM in the future) using the same command for different types of modules, I'd omit the second part.
| + { | ||
| + grub_printf ("Secure launcher: Intel TXT\n"); | ||
| + grub_txt_state_show (); | ||
| + } |
There was a problem hiding this comment.
else { BUG(); }? That might be too severe, but we definitely don't want to just output nothing if someone forgets to update this function when adding a new slp type.
| + * If the first parameter "x" is greater than zero and | ||
| + * if that is true, that the largest possible value 0xFFFFFFFF / "x" | ||
| + * is less than the second parameter "y". If "y" is zero then | ||
| + * it will also fail because no unsigned number is less than zero. |
There was a problem hiding this comment.
What do you mean? If y = 0, then this will correctly report no overflow.
| +} | ||
| + | ||
| +/* | ||
| + * These three "plus overflow" functions take a "x" value |
|
I find it somewhat peculiar that the functions whose purpose is obvious get elaborate comments (e.g. |
| +{ | ||
| + static struct grub_txt_acm_info_table *info_table; | ||
| + | ||
| + /* Assumes that it passed is_sinit_acmod() */ |
There was a problem hiding this comment.
This function is now called grub_txt_is_sinit_acmod.
| + return 0; | ||
| + | ||
| + /* Then check type and vendor */ | ||
| + if ( (acm_hdr->module_type != GRUB_TXT_ACM_MODULE_TYPE) || |
There was a problem hiding this comment.
Should we check module_sub_type here? The pseudocode in the spec doesn't, but the documentation of the header (in appendix A) mentions that only a TXT_ACM subtype will work.
| + if ( user_area_off + sizeof(struct grub_txt_acm_info_table) > hdr->size * 4 ) | ||
| + { | ||
| + /* TODO: Is (grub_uint32_t) correct??? */ | ||
| + grub_error (GRUB_ERR_OUT_OF_RANGE, N_("ACM info table size too large: %x > %x"), |
There was a problem hiding this comment.
This error message doesn't make sense, the info table size is a constant, it can't be too large. "ACM info table out of bounds"?
| + { | ||
| + /* TODO: Is (grub_uint32_t) correct??? */ | ||
| + grub_error (GRUB_ERR_OUT_OF_RANGE, N_("ACM info table size too large: %x > %x"), | ||
| + user_area_off + (grub_uint32_t)sizeof(struct grub_txt_acm_info_table), hdr->size * 4); |
There was a problem hiding this comment.
Why do you only need the cast for the error message, and not for the actual check?
| + } | ||
| + | ||
| + /* Overflow? */ | ||
| + if ( plus_overflow_u32 ((grub_uint32_t)(unsigned long)hdr, user_area_off) ) |
There was a problem hiding this comment.
For the ACM module loaded from file case, we've allocated it ourselves so anything within the actual size of the module doesn't have any chance of overflowing the pointer. In the case where the module is provided by BIOS, we might indeed need such checks, but much earlier than this (I haven't checked yet whether they are actually present there).
| + (acm_hdr->module_vendor != GRUB_TXT_ACM_MODULE_VENDOR_INTEL) ) | ||
| + return 0; | ||
| + | ||
| + info_table = get_acmod_info_table (acm_hdr); |
There was a problem hiding this comment.
We should check header_len before this point to make sure it's not too small, as that would cause some weird overlaps of the data structures.
There was a problem hiding this comment.
The info table also has a length field, checking that is probably a good idea.
There was a problem hiding this comment.
Should I check for equality or just >= sizeof(struct...) and hope for the best? The specification doesn't seem to give any guaranties as to compatibility of headers fields.
There was a problem hiding this comment.
It seems that the way the spec evolves is that they add new fields at the end and increase the header_len. I wouldn't expect new versions to change the semantics of existing fields, but logging a warning when we see a version field with a higher value than we know about might be a good idea.
| + | ||
| + /* Then check type and vendor */ | ||
| + if ( (acm_hdr->module_type != GRUB_TXT_ACM_MODULE_TYPE) || | ||
| + (acm_hdr->module_vendor != GRUB_TXT_ACM_MODULE_VENDOR_INTEL) ) |
There was a problem hiding this comment.
I don't think we're likely to see a non-Intel ACM, but I don't think there would be anything wrong with one, and the spec doesn't mandate any such check, so I'm somewhat torn on this one.
It's a shame we don't report what exactly failed about the ACM module validation, because then an approach of "just release it, if we get this slightly wrong our users will tell us" would be more defensible.
| +/* Format of VER.FSBIF and VER.QPIIF registers. */ | ||
| +typedef union { | ||
| + grub_uint64_t _raw; | ||
| + struct { | ||
| + grub_uint64_t reserved : 31; | ||
| + grub_uint64_t prod_fused : 1; | ||
| + }; | ||
| +} grub_txt_ver_fsbif_qpiif_t; |
There was a problem hiding this comment.
Can you point me to the docs explaining in what circumstances bitfield layout can be relied upon?
| + ver._raw = grub_txt_reg_pub_readq (GRUB_TXT_VER_FSBIF); | ||
| + if ( (ver._raw & 0xffffffff) == 0xffffffff || | ||
| + (ver._raw & 0xffffffff) == 0x00 ) /* Need to use VER.QPIIF */ | ||
| + ver._raw = grub_txt_reg_pub_readq (GRUB_TXT_VER_QPIIF); |
There was a problem hiding this comment.
I had to seek out an earlier version of the TXT docs to find a description of VER_FSBIF. A revision from December 2009 only specifies a value of 0xffffffff as necessitating a read from QPIIF instead. Though if the current guidance is to always read from QPIIF only, this doesn't matter much. Still, I'm curious, where did the == 0x00 part come from?
Also, since this is a 4-byte register, I'm not sure why you're using readq to read it.
There was a problem hiding this comment.
This probably comes from description of FSBIF in Appendix B of older TXT docs revision, like this one: https://kib.kiev.ua/x86docs/Intel/TXT/315168-011.pdf. Pseudocode in that revision uses just 0xffffffff, though.
| + if ( (ver._raw & GRUB_TXT_VERSION_DEBUG_FUSED) && | ||
| + (hdr->flags & GRUB_TXT_ACM_FLAG_DEBUG_SIGNED) ) |
There was a problem hiding this comment.
This errors out on a production chipset with a debug ACM, but not on a debug chipset with a production ACM. I don't think that's intended.
| + /* Overflow? */ | ||
| + if ( plus_overflow_u32 (id_list_off, sizeof(struct grub_txt_acm_chipset_id)) ) | ||
| + { | ||
| + grub_error (GRUB_ERR_OUT_OF_RANGE, N_("id_list_off plus acm_chipset_id_t size overflows")); | ||
| + return NULL; | ||
| + } | ||
| + | ||
| + /* Check that chipset id table is w/in ACM */ | ||
| + if ( id_list_off + sizeof(struct grub_txt_acm_chipset_id) > size ) | ||
| + { | ||
| + grub_error (GRUB_ERR_OUT_OF_RANGE, N_("chipset id list is too big: %x"), id_list_off); | ||
| + return NULL; | ||
| + } |
There was a problem hiding this comment.
I believe this check should be for sizeof(struct grub_txt_acm_chipset_id_list) instead.
There was a problem hiding this comment.
Considering how many repetitive and error prone bound checks the ACM parsing code does, how about a helper function:
/* Returns hdr + offset if [offset, offset + size) is within the bounds of the ACM, NULL otherwise. */
static void* fits_in_acm(struct grub_txt_acm_header *hdr, grub_uint32_t offset, grub_uint32_t size);
| + /* Overflow? */ | ||
| + if ( plus_overflow_u32 ((grub_uint32_t)(unsigned long)hdr, id_list_off) ) | ||
| + { | ||
| + grub_error (GRUB_ERR_OUT_OF_RANGE, N_("hdr plus id_list_off overflows")); | ||
| + return NULL; | ||
| + } |
There was a problem hiding this comment.
As before, checking for overflow of pointer + offset, when you've already checked that the offset is inbounds for the object the pointer points to, seems like a code smell.
| + if ( plus_overflow_u32 (id_list_off + sizeof(struct grub_txt_acm_chipset_id), | ||
| + chipset_id_list->count * sizeof(struct grub_txt_acm_chipset_id)) ) | ||
| + { | ||
| + grub_error (GRUB_ERR_OUT_OF_RANGE, N_("size of all entries overflows")); | ||
| + return NULL; | ||
| + } | ||
| + | ||
| + /* Check that all entries are w/in ACM */ | ||
| + if ( id_list_off + sizeof(struct grub_txt_acm_chipset_id) + | ||
| + chipset_id_list->count * sizeof(struct grub_txt_acm_chipset_id) > size ) | ||
| + { | ||
| + grub_error (GRUB_ERR_OUT_OF_RANGE, N_("ACM chipset id entries are too big: %x"), | ||
| + chipset_id_list->count); | ||
| + return NULL; | ||
| + } |
There was a problem hiding this comment.
Likewise, id_list_off + sizeof(struct grub_txt_acm_chipset_id) should use sizeof(...id_list) instead.
| + | ||
| + grub_dprintf ("slaunch", "%d SINIT ACM chipset id entries:\n", chipset_id_list->count); | ||
| + | ||
| + chipset_id = (struct grub_txt_acm_chipset_id *) ((grub_addr_t)chipset_id_list + sizeof (chipset_id_list->count)); |
There was a problem hiding this comment.
This could be avoided by putting a flexible array member at the end of the chipset_id_list struct.
| + (didvid.did == chipset_id->device_id ) && | ||
| + ( ( ( (chipset_id->flags & GRUB_TXT_ACM_REVISION_ID_MASK) == 0) && | ||
| + (didvid.rid == chipset_id->revision_id) ) || | ||
| + ( ( (chipset_id->flags & GRUB_TXT_ACM_REVISION_ID_MASK) == 1) && |
There was a problem hiding this comment.
== 1 only works because REVISION_ID_MASK happens to be the lowest bit. How about this?
| + ( ( (chipset_id->flags & GRUB_TXT_ACM_REVISION_ID_MASK) == 1) && | |
| + ( ( (chipset_id->flags & GRUB_TXT_ACM_REVISION_ID_MASK) != 0) && |
| + if ( (didvid.vid == chipset_id->vendor_id ) && | ||
| + (didvid.did == chipset_id->device_id ) && | ||
| + ( ( ( (chipset_id->flags & GRUB_TXT_ACM_REVISION_ID_MASK) == 0) && | ||
| + (didvid.rid == chipset_id->revision_id) ) || | ||
| + ( ( (chipset_id->flags & GRUB_TXT_ACM_REVISION_ID_MASK) == 1) && | ||
| + ( (didvid.rid & chipset_id->revision_id) != 0 ) ) ) ) |
There was a problem hiding this comment.
Extracting this into a separate function taking a chipset_id and didvid would let you write this logic better than with a monstrous if statement.
| + /* | ||
| + * Logic inverted from oringal to avoid the if block. Not sure what drives | ||
| + * the logic of not checking processor infrmation for version 4 or less. | ||
| + */ | ||
| + if ( info_table->version < 4 ) | ||
| + return 1; |
There was a problem hiding this comment.
Unfortunately the spec isn't too clear on what the changes to the info table were, across the different versions, but an old version of the entire spec document shows that the processor table field didn't exist when the latest version of the info table was 3.
I guess this is a good reason to verify the length field in the info table.
| +} | ||
| + | ||
| +static struct grub_txt_acm_processor_id_list* | ||
| +get_acmod_processor_list (struct grub_txt_acm_header* hdr) |
There was a problem hiding this comment.
Same comments as the chipset list.
| + | ||
| + if ( i >= proc_id_list->count ) | ||
| + { | ||
| + grub_error (GRUB_ERR_BAD_DEVICE, N_("chipset id mismatch")); |
| + proc_id_list = get_acmod_processor_list(hdr); | ||
| + if ( !proc_id_list ) | ||
| + return 1; |
There was a problem hiding this comment.
If the version field indicates that there is a processor list, shouldn't we error out if we consider that list malformed?
| + data, data & 0x000000000000ffff, | ||
| + (data & 0x00000000ffff0000) >> 16, | ||
| + (data & 0x0000ffff00000000) >> 32, data >> 48); |
There was a problem hiding this comment.
Why not use the union grub_txt_didvid here?
There was a problem hiding this comment.
Doing it manually is safer when it comes to endianness, then again Intel is always LE.
There was a problem hiding this comment.
In that case union grub_txt_didvid shouldn't exist at all, don't you think?
| +/* | ||
| + * This checks to see if two numbers multiplied together are larger | ||
| + * than the type that they are. Returns TRUE if OVERFLOWING. | ||
| + * If the first parameter "x" is greater than zero and | ||
| + * if that is true, that the largest possible value 0xFFFFFFFF / "x" | ||
| + * is less than the second parameter "y". If "y" is zero then | ||
| + * it will also fail because no unsigned number is less than zero. | ||
| + */ | ||
| +static inline int | ||
| +multiply_overflow_u32 (grub_uint32_t x, grub_uint32_t y) | ||
| +{ | ||
| + /* Use x instead of (x > 0)? */ | ||
| + return (x > 0) ? ((((grub_uint32_t)(~0))/x) < y) : 0; | ||
| +} | ||
| + | ||
| +/* | ||
| + * These three "plus overflow" functions take a "x" value | ||
| + * and add the "y" value to it and if the two values are | ||
| + * greater than the size of the variable type, they will | ||
| + * overflow the type and end up with a smaller value and | ||
| + * return TRUE - that they did overflow. i.e. | ||
| + */ | ||
| +static inline int plus_overflow_u32 (grub_uint32_t x, grub_uint32_t y) | ||
| +{ | ||
| + return ((((grub_uint32_t)(~0)) - x) < y); | ||
| +} |
There was a problem hiding this comment.
Better functions already exist in <grub/safemath.h>.
There was a problem hiding this comment.
They do now, weren't there when the original code was created. Will update it.
| +#define KERNEL_INFO_HEADER "LToP" | ||
| +#define KERNEL_INFO_MIN_SIZE_TOTAL 12 |
There was a problem hiding this comment.
Is this a new header? Do we document this somewhere?
|
@rossphilipson @dpsmith do you want to address comments related to your code? I'm fine with doing that myself, I just want to avoid a) repeating something that may already be done somewhere else, b) adding changes here that may not propagate to other uses of those patches, and c) breaking some corner case for other users of that code. |
|
@meithecatte thanks for the extensive review. Ack on most of the comments, I've began to work on new set of patches. Few replies from me:
Well, there is
This wouldn't be enough in general, but may be here. It may not work e.g. if some code prepared data in normal RAM that would be consumed after access to MMIO by either:
I think I'll just change the order of those patches. |
That is a good point. In the case where this would then be consumed through DMA, though, wouldn't the function in question still need to handle flushing the cache? I think the barrier is best put there. |
| +static void init_tpm12_event_log(struct grub_slaunch_params *slparams) | ||
| +{ | ||
| + struct event_log_container *elog; | ||
| + elog = (struct event_log_container *)(grub_addr_t)slparams->tpm_evt_log_base; |
There was a problem hiding this comment.
slparams->tpm_evt_log_base was set as get_physical_target_address() so it isn't safe to use as a pointer.
|
I've uploaded new version of patches, I think I've addressed most of the comments. I haven't changed anything related to |
Signed-off-by: Krystian Hebel <[email protected]>
| +#define GRUB_TXT_ACM_HEADER_LEN_3_0 224 | ||
| + | ||
| +#define GRUB_TXT_ACM_HEADER_VERSION_0_0 0x0000 | ||
| +#define GRUB_TXT_ACM_HEADER_VERSION_3_0 0x0300 |
There was a problem hiding this comment.
Also, these are all unused right now.
| +#define GRUB_CR4_X86_PGE 0x00000080 /* Enable Page global */ | ||
| +#define GRUB_CR4_X86_PCE 0x00000100 /* Enable Performance monitoring counter */ | ||
| +#define GRUB_CR4_X86_FXSR 0x00000200 /* Fast FPU save/restore */ | ||
| +#define GRUB_CR4_X86_XMM 0x00000400 /* Enable SIMD/MMX2 to use except 16 */ |
There was a problem hiding this comment.
This bit means that unmasked exceptions generate #XM, rather than #UD.
| +#define GRUB_MSR_X86_APICBASE 0x0000001b | ||
| +#define GRUB_MSR_X86_APICBASE_BSP (1<<8) | ||
| +#define GRUB_MSR_X86_APICBASE_ENABLE (1<<11) | ||
| +#define GRUB_MSR_X86_APICBASE_BASE (0xfffff<<12) |
There was a problem hiding this comment.
What does this refer to? Mask for APIC base address?
There was a problem hiding this comment.
Comments after each of these #defines would be appreciated, as was done for GRUB_EFLAGS_* above.
| +#define GRUB_MSR_AMD64_PATCH_LEVEL 0x0000008b | ||
| +#define GRUB_MSR_AMD64_PATCH_CLEAR 0xc0010021 /* AMD-specific microcode patch clear */ | ||
| +#define GRUB_MSR_AMD64_VM_CR 0xc0010114 | ||
| +#define GRUB_MSR_SVM_VM_CR_SVM_DISABLE 4 |
There was a problem hiding this comment.
Where are these MSRs specified in AMD’s official documentation?
There was a problem hiding this comment.
VM_CR can be found in AMD64 Architecture Programmer’s Manual Volume 2. The ones for microcode patching aren't officially, publicly documented to the best of my knowledge. I don't think that microcode MSRs will be used in the foreseeable future so I guess they can be removed.
There was a problem hiding this comment.
Can each of these have a comment after them?
If possible, the code should be understandable without having to have the vendor’s manuals open at the same time.
| + struct | ||
| + { | ||
| + grub_uint64_t reserved1 : 11; | ||
| + grub_uint64_t v : 1; /* valid */ | ||
| + grub_uint64_t mask : 52; /* define as max width and mask w/ */ | ||
| + /* MAXPHYADDR when using */ | ||
| + }; |
There was a problem hiding this comment.
What guarantees that this bitfield will have the correct layout?
There was a problem hiding this comment.
The same thing that makes __builtin_*() works - small set of supported compilers, together with fact that this code won't ever be executed on big endian platform, or even on anything else than x86. I agree that bitfields are bad for most cases, especially if data has to be sent between different processors (including all auxiliary cores in typical computer), but in this case the bitfield behaves sufficiently decent.
That said, if you insist I can change it to use shifting and masking. I'm not strongly for or against any of those approaches, I just think that some of them are blamed without consideration for a particular use case.
There was a problem hiding this comment.
Bitfields often result in poorly optimized code from the compiler (per Linus Torvalds), but that doesn’t matter here. The bitfield is more readable, so just add a comment explaining why this works.
| + grub_writeb (TIS_RELINQUISH_LCL, addr + TPM_ACCESS); | ||
| + else if (tpm_intf == TPM_INTF_CRB) | ||
| + grub_writel (CRB_RELINQUISH_LCL, addr + TPM_LOC_CTRL); | ||
| +} |
There was a problem hiding this comment.
What if neither of those are correct, as would be the case on e.g. a system with no TPM at all?
There was a problem hiding this comment.
ACM will fail, but I think there are enough tests in this code that should catch it before this function is called. In any case, the function does the sane thing of not writing to non-existing register if there is no TPM detected.
| + cmd_tpm_type = grub_register_command ("tpm_type", grub_cmd_tpm_type, | ||
| + NULL, N_("Show TPM version and interface type.")); | ||
| + | ||
| + intf_cap = grub_readl (TPM_MMIO_BASE + TPM_INTF_CAPABILITY); |
There was a problem hiding this comment.
This register is invalid if the TPM uses CRB instead of FIFO. Therefore, this check needs to come after the check for CRB vs FIFO.
| + if (!(feat_ctrl & GRUB_MSR_X86_FEATURE_CTRL_LOCK)) | ||
| + { | ||
| + grub_dprintf ("slaunch", "Firmware didn't lock FEATURE_CONTROL MSR," | ||
| + "locking it now\n"); | ||
| + /* Not setting SENTER_FUNCTIONS and SENTER_ENABLE because they were tested | ||
| + * in grub_txt_verify_platform() */ | ||
| + feat_ctrl |= GRUB_MSR_X86_FEATURE_CTRL_LOCK | GRUB_MSR_X86_ENABLE_VMX_IN_SMX; | ||
| + grub_wrmsr (GRUB_MSR_X86_FEATURE_CONTROL, feat_ctrl); | ||
| + } |
There was a problem hiding this comment.
This should also enable VMX outside of SMX. If the firmware locked the register but did not enable VMX in SMX, the code should likely fail, as attempting to boot Xen would at a minimum prevent any VM from starting.
There was a problem hiding this comment.
This is how this bit is supposed to work, if firmware disabled VMX and locked configuration then is shouldn't be possible to change it.
As for VMX outside of SMX, how would it help? VMX is still available after SENTER, and any failure to boot through SENTER should halt the boot process. Non-SMX entry doesn't call this function so it wouldn't be impacted by locking with VMX disabled.
There was a problem hiding this comment.
GRUB is interactive, so the user might decide to boot without SMX if this command fails.
| + break; | ||
| + | ||
| + default: | ||
| + grub_error (GRUB_ERR_BAD_ARGUMENT, N_("Unknown SMX parameter")); |
There was a problem hiding this comment.
This error also needs to be propogated.
| + if ( enable ) | ||
| + mtrr_def_type |= GRUB_MSR_X86_MTRR_ENABLE; | ||
| + else | ||
| + mtrr_def_type &= ~GRUB_MSR_X86_MTRR_ENABLE; |
There was a problem hiding this comment.
This will truncate mtrr_def_type unless GRUB_MSR_X86_MTRR_ENABLE is defined as 64 bits. Better to define a macro for this that ensures that no truncation happens.
There was a problem hiding this comment.
No, it won't 🙂 but will do.
There was a problem hiding this comment.
Generally, I recommend something like:
#define MASK_NEGATE(a, b) ((a) & ~(__typeof__(a))(b))
and enabling -Werror=conversion in the build.
There was a problem hiding this comment.
This may not be popular opinion, but I'm against using -Werror=conversion, it often (at least in my experience) leads to bad decisions. For example, in this case the error message would suggest to make GRUB_MSR_X86_MTRR_ENABLE unsigned (e.g. by using (1u<<11)). This would make the error go away, but then the value would actually be truncated (unless MASK_NEGATE is used).
In any case, I still believe that the current code is well defined:
GRUB_MSR_X86_MTRR_ENABLEhas a constant, relatively small value that isn't in any way modified, neither by shifting that would impact sign bit nor by user input.~operator causes that constant to undergo integer promotion. As the value is small enough to be represented byint, that type is used (instead ofunsigned int), so~operates on and returnsint. As the sign bit inGRUB_MSR_X86_MTRR_ENABLEwas not set, it is set in the result, which makes it a negative signedint.&=follows the usual arithmetic conversions rules. In this case, both values are integers, they differ in type and in signedness, and unsigned type has greater conversion rank than a signed one. In that case, the operand with the signed type (int) is implicitly converted to the unsigned type (grub_uint64_t). This follows integer conversion rules. First point from that link doesn't apply, sincegrub_uint64_tcan't represent negative result of~. Second point is used, i.e. 2^64 is added to that result, which gives a number that can be represented bygrub_uint64_t, so that value is used.
| + } | ||
| + | ||
| + if (sinit != NULL) | ||
| + grub_dprintf ("slaunch", "SINIT ACM date: %" PRIxGRUB_UINT32_T "\n", sinit->date); |
There was a problem hiding this comment.
| + grub_dprintf ("slaunch", "SINIT ACM date: %" PRIxGRUB_UINT32_T "\n", sinit->date); | |
| + grub_dprintf ("slaunch", "SINIT ACM date: 0x%8" PRIxGRUB_UINT32_T "\n", sinit->date); |
There was a problem hiding this comment.
This is BCD date in YYYYMMDD format, what is the use of 0x prefix?
| + /* Does BIOS provide SINIT ACM? */ | ||
| + if (bios_sinit != NULL) | ||
| + { | ||
| + grub_dprintf ("slaunch", "BIOS SINIT ACM date: %" PRIxGRUB_UINT32_T "\n", |
There was a problem hiding this comment.
| + grub_dprintf ("slaunch", "BIOS SINIT ACM date: %" PRIxGRUB_UINT32_T "\n", | |
| + grub_dprintf ("slaunch", "BIOS SINIT ACM date: 0x%8" PRIxGRUB_UINT32_T "\n", |
| + ver = grub_txt_reg_pub_readl (GRUB_TXT_VER_FSBIF); | ||
| + if ( ver == 0xffffffff || ver == 0x00 ) /* Need to use VER.QPIIF */ | ||
| + ver = grub_txt_reg_pub_readl (GRUB_TXT_VER_QPIIF); |
There was a problem hiding this comment.
Should be the other way around, as FSBIF is deprecated by Intel.
| +/* | ||
| + * TODO: Why 1 GiB limit? It does not seem that it is required by TXT spec. | ||
| + * If there is a limit then it should be checked before allocation and image load. | ||
| + * | ||
| + * If enough room is available in front of the MLE, the maximum size of an | ||
| + * MLE that can be covered is 1G. This is due to having 512 PDEs pointing | ||
| + * to 512 page tables with 512 PTEs each. | ||
| + */ | ||
| +grub_uint32_t | ||
| +grub_txt_get_mle_ptab_size (grub_uint32_t mle_size) | ||
| +{ | ||
| + /* | ||
| + * #PT + 1 PT + #PD + 1 PD + 1 PDT | ||
| + * | ||
| + * Why do we need 2 extra PTEs and PDEs? Yes, because MLE image may not | ||
| + * start and end at PTE (page) and PDE (2 MiB) boundary... | ||
| + */ | ||
| + return ((((mle_size / GRUB_PAGE_SIZE) + 2) / 512) + 1 + | ||
| + (((mle_size / (512 * GRUB_PAGE_SIZE)) + 2) / 512) + 1 + 1) * GRUB_PAGE_SIZE; | ||
| +} | ||
| + | ||
| +/* Page directory and table entries only need Present set */ | ||
| +#define MAKE_PT_MLE_ENTRY(addr) (((grub_uint64_t)(grub_addr_t)(addr) & GRUB_PAGE_MASK) | 0x01) | ||
| + | ||
| +/* | ||
| + * The MLE page tables have to be below the MLE and have no special regions in | ||
| + * between them and the MLE (this is a bit of an unwritten rule). | ||
| + * 20 pages are carved out of memory below the MLE. That leave 18 page table | ||
| + * pages that can cover up to 36M . | ||
| + * can only contain 4k pages | ||
| + * | ||
| + * TODO: TXT Spec p.32; List section name and number with PT MLE requirments here. | ||
| + * | ||
| + * TODO: This function is not able to cover MLEs larger than 1 GiB. Fix it!!! | ||
| + * After fixing inrease GRUB_TXT_MLE_MAX_SIZE too. | ||
| + */ | ||
| +void | ||
| +grub_txt_setup_mle_ptab (struct grub_slaunch_params *slparams) | ||
| +{ | ||
| + grub_uint8_t *pg_dir, *pg_dir_ptr_tab = slparams->mle_ptab_mem, *pg_tab; | ||
| + grub_uint32_t mle_off = 0, pd_off = 0; | ||
| + grub_uint64_t *pde, *pte; | ||
| + | ||
| + grub_memset (pg_dir_ptr_tab, 0, slparams->mle_ptab_size); | ||
| + | ||
| + pg_dir = pg_dir_ptr_tab + GRUB_PAGE_SIZE; | ||
| + pg_tab = pg_dir + GRUB_PAGE_SIZE; | ||
| + | ||
| + /* Only use first entry in page dir ptr table */ | ||
| + *(grub_uint64_t *)pg_dir_ptr_tab = MAKE_PT_MLE_ENTRY(pg_dir); | ||
| + | ||
| + /* Start with first entry in page dir */ | ||
| + *(grub_uint64_t *)pg_dir = MAKE_PT_MLE_ENTRY(pg_tab); | ||
| + | ||
| + pte = (grub_uint64_t *)pg_tab; | ||
| + pde = (grub_uint64_t *)pg_dir; | ||
| + | ||
| + do | ||
| + { | ||
| + *pte = MAKE_PT_MLE_ENTRY(slparams->mle_start + mle_off); | ||
| + | ||
| + pte++; | ||
| + mle_off += GRUB_PAGE_SIZE; | ||
| + | ||
| + if (!(++pd_off % 512)) | ||
| + { | ||
| + /* Break if we don't need any additional page entries */ | ||
| + if (mle_off >= slparams->mle_size) | ||
| + break; | ||
| + pde++; | ||
| + *pde = MAKE_PT_MLE_ENTRY(pte); | ||
| + } | ||
| + } while (mle_off < slparams->mle_size); | ||
| +} |
| + /* TODO: Check low PMR with RMRR. Look at relevant tboot code too. */ | ||
| + /* TODO: Kernel should not allocate any memory outside of PMRs regions!!! */ |
| + mtrr_def_type &= ~(GRUB_MSR_X86_MTRR_ENABLE_FIXED | GRUB_MSR_X86_DEF_TYPE_MASK); | ||
| + mtrr_def_type |= GRUB_MTRR_MEMORY_TYPE_UC; | ||
| + grub_wrmsr (GRUB_MSR_X86_MTRR_DEF_TYPE, mtrr_def_type); | ||
| + | ||
| + /* Initially disable all variable MTRRs (we'll enable the ones we use) */ | ||
| + mtrr_cap = grub_rdmsr (GRUB_MSR_X86_MTRRCAP); | ||
| + vcnt = (mtrr_cap & GRUB_MSR_X86_VCNT_MASK); | ||
| + | ||
| + for ( ndx = 0; ndx < vcnt; ndx++ ) | ||
| + { | ||
| + mtrr_physmask.raw = grub_rdmsr (GRUB_MSR_X86_MTRR_PHYSMASK0 + ndx*2); | ||
| + mtrr_physmask.v = 0; | ||
| + grub_wrmsr (GRUB_MSR_X86_MTRR_PHYSMASK0 + ndx*2, mtrr_physmask.raw); | ||
| + } | ||
| + | ||
| + /* Map all AC module pages as mem_type */ | ||
| + num_pages = GRUB_PAGE_UP(size) >> GRUB_PAGE_SHIFT; | ||
| + | ||
| + grub_dprintf ("slaunch", "setting MTRRs for acmod: base=%p, size=%x, num_pages=%d\n", | ||
| + base, size, num_pages); | ||
| + | ||
| + /* Each VAR MTRR base must be a multiple if that MTRR's Size */ | ||
| + base_v = (unsigned long)base; | ||
| + /* MTRR size in pages */ | ||
| + mtrr_s = 1; | ||
| + | ||
| + while ( (base_v & 0x01) == 0 ) | ||
| + { | ||
| + i++; | ||
| + base_v = base_v >> 1; | ||
| + } | ||
| + | ||
| + for (j = i - 12; j > 0; j--) | ||
| + mtrr_s = mtrr_s*2; /* mtrr_s = mtrr_s << 1 */ | ||
| + | ||
| + grub_dprintf ("slaunch", "The maximum allowed MTRR range size=%d Pages \n", mtrr_s); | ||
| + | ||
| + ndx = 0; | ||
| + | ||
| + while ( num_pages >= mtrr_s ) | ||
| + { | ||
| + mtrr_physbase.raw = grub_rdmsr (GRUB_MSR_X86_MTRR_PHYSBASE0 + ndx*2); | ||
| + mtrr_physbase.base = ((unsigned long)base >> GRUB_PAGE_SHIFT) & | ||
| + SINIT_MTRR_MASK; | ||
| + mtrr_physbase.type = mem_type; | ||
| + grub_wrmsr (GRUB_MSR_X86_MTRR_PHYSBASE0 + ndx*2, mtrr_physbase.raw); | ||
| + | ||
| + mtrr_physmask.raw = grub_rdmsr (GRUB_MSR_X86_MTRR_PHYSMASK0 + ndx*2); | ||
| + mtrr_physmask.mask = ~(mtrr_s - 1) & SINIT_MTRR_MASK; | ||
| + mtrr_physmask.v = 1; | ||
| + grub_wrmsr (GRUB_MSR_X86_MTRR_PHYSMASK0 + ndx*2, mtrr_physmask.raw); | ||
| + | ||
| + base += (mtrr_s * GRUB_PAGE_SIZE); | ||
| + num_pages -= mtrr_s; | ||
| + ndx++; | ||
| + if ( ndx == vcnt ) | ||
| + return grub_error (GRUB_ERR_BAD_DEVICE, | ||
| + N_("exceeded number of var MTRRs when mapping range")); | ||
| + } | ||
| + | ||
| + ndx = 0; | ||
| + | ||
| + while ( num_pages > 0 ) | ||
| + { | ||
| + /* Set the base of the current MTRR */ | ||
| + mtrr_physbase.raw = grub_rdmsr (GRUB_MSR_X86_MTRR_PHYSBASE0 + ndx*2); | ||
| + mtrr_physbase.base = ((unsigned long)base >> GRUB_PAGE_SHIFT) & | ||
| + SINIT_MTRR_MASK; | ||
| + mtrr_physbase.type = mem_type; | ||
| + grub_wrmsr (GRUB_MSR_X86_MTRR_PHYSBASE0 + ndx*2, mtrr_physbase.raw); | ||
| + | ||
| + /* | ||
| + * Calculate MTRR mask | ||
| + * MTRRs can map pages in power of 2 | ||
| + * may need to use multiple MTRRS to map all of region | ||
| + */ | ||
| + pages_in_range = 1 << (fls (num_pages) - 1); | ||
| + | ||
| + mtrr_physmask.raw = grub_rdmsr (GRUB_MSR_X86_MTRR_PHYSMASK0 + ndx*2); | ||
| + mtrr_physmask.mask = ~(pages_in_range - 1) & SINIT_MTRR_MASK; | ||
| + mtrr_physmask.v = 1; | ||
| + grub_wrmsr (GRUB_MSR_X86_MTRR_PHYSMASK0 + ndx*2, mtrr_physmask.raw); | ||
| + | ||
| + /* | ||
| + * Prepare for the next loop depending on number of pages | ||
| + * We figure out from the above how many pages could be used in this | ||
| + * mtrr. Then we decrement the count, increment the base, | ||
| + * increment the mtrr we are dealing with, and if num_pages is | ||
| + * still not zero, we do it again. | ||
| + */ | ||
| + base += (pages_in_range * GRUB_PAGE_SIZE); | ||
| + num_pages -= pages_in_range; | ||
| + ndx++; | ||
| + if ( ndx == vcnt ) | ||
| + return grub_error (GRUB_ERR_BAD_DEVICE, | ||
| + N_("exceeded number of var MTRRs when mapping range")); | ||
| + } | ||
| + | ||
| + return GRUB_ERR_NONE; | ||
| +} |
There was a problem hiding this comment.
I will look at this again later.
| + grub_uint8_t data[]; | ||
| +} GRUB_PACKED; | ||
| + | ||
| +#define EVTLOG_SIGNATURE "TXT Event Container\0" |
There was a problem hiding this comment.
| +#define EVTLOG_SIGNATURE "TXT Event Container\0" | |
| +#define EVTLOG_SIGNATURE "TXT Event Container" |
Trailing NUL is implied.
| + if (grub_get_tpm_ver () == GRUB_TPM_20) | ||
| + { | ||
| + /* CBnT bits 5:4 must be 11b, since D/A mapping is the only one supported. */ | ||
| + if ((sinit_caps & os_sinit_data->capabilities) != os_sinit_data->capabilities) | ||
| + return grub_error (GRUB_ERR_BAD_ARGUMENT, | ||
| + N_("Details/authorities PCR usage is not supported")); | ||
| + } |
There was a problem hiding this comment.
This change is unrelated to TPM 1.2 and should be in an earlier patch.
There was a problem hiding this comment.
This is also no longer valid because Intel deprecated those bits without ensuring backward compatibility. Fix will be included in next revision of patches.
| + grub_uint8_t *ptr = (grub_uint8_t *)elt; | ||
| + struct grub_txt_heap_bios_spec_ver_element *bios_spec_ver_elt = | ||
| + (struct grub_txt_heap_bios_spec_ver_element *)ptr; |
There was a problem hiding this comment.
This cast does not make sense to me. Why not just use sizeof(struct grub_txt_heap_bios_spec_ver_element)?
| +static grub_err_t | ||
| +verify_acm_elt (struct grub_txt_heap_ext_data_element *elt) | ||
| +{ | ||
| + grub_uint8_t *ptr = ((grub_uint8_t *)elt + sizeof(*elt)); |
There was a problem hiding this comment.
| + grub_uint8_t *ptr = ((grub_uint8_t *)elt + sizeof(*elt)); | |
| + grub_uint8_t *ptr = elt->data; |
unless this runs into strict aliasing or other problems.
There was a problem hiding this comment.
I think I'll rework all of those elements to include struct grub_txt_heap_ext_data_element at the beginning, then all those calculations won't be necessary. Maybe even having all elements as one union type? Will need to check if GRUB code style allows it.
| + struct grub_txt_heap_acm_element *acm_elt = | ||
| + (struct grub_txt_heap_acm_element *)ptr; |
There was a problem hiding this comment.
Should first check that elt->size is at least sizeof(*elt) + sizeof(*acm_elt).
| + mtrr_physbase.raw = grub_rdmsr (GRUB_MSR_X86_MTRR_PHYSBASE0 + ndx*2); | ||
| + mtrr_physbase.base = ((unsigned long)base >> GRUB_PAGE_SHIFT) & | ||
| + SINIT_MTRR_MASK; | ||
| + mtrr_physbase.type = mem_type; | ||
| + grub_wrmsr (GRUB_MSR_X86_MTRR_PHYSBASE0 + ndx*2, mtrr_physbase.raw); |
There was a problem hiding this comment.
This should be a utility function. The repetition makes the math very difficult to follow.
| + mtrr_physmask.raw = grub_rdmsr (GRUB_MSR_X86_MTRR_PHYSMASK0 + ndx*2); | ||
| + mtrr_physmask.mask = ~(pages_in_range - 1) & SINIT_MTRR_MASK; | ||
| + mtrr_physmask.v = 1; | ||
| + grub_wrmsr (GRUB_MSR_X86_MTRR_PHYSMASK0 + ndx*2, mtrr_physmask.raw); |
| + while ( num_pages >= mtrr_s ) | ||
| + { | ||
| + mtrr_physbase.raw = grub_rdmsr (GRUB_MSR_X86_MTRR_PHYSBASE0 + ndx*2); | ||
| + mtrr_physbase.base = ((unsigned long)base >> GRUB_PAGE_SHIFT) & | ||
| + SINIT_MTRR_MASK; | ||
| + mtrr_physbase.type = mem_type; | ||
| + grub_wrmsr (GRUB_MSR_X86_MTRR_PHYSBASE0 + ndx*2, mtrr_physbase.raw); | ||
| + | ||
| + mtrr_physmask.raw = grub_rdmsr (GRUB_MSR_X86_MTRR_PHYSMASK0 + ndx*2); | ||
| + mtrr_physmask.mask = ~(mtrr_s - 1) & SINIT_MTRR_MASK; | ||
| + mtrr_physmask.v = 1; | ||
| + grub_wrmsr (GRUB_MSR_X86_MTRR_PHYSMASK0 + ndx*2, mtrr_physmask.raw); | ||
| + | ||
| + base += (mtrr_s * GRUB_PAGE_SIZE); | ||
| + num_pages -= mtrr_s; | ||
| + ndx++; | ||
| + if ( ndx == vcnt ) | ||
| + return grub_error (GRUB_ERR_BAD_DEVICE, | ||
| + N_("exceeded number of var MTRRs when mapping range")); | ||
| + } |
There was a problem hiding this comment.
I don’t think this is optimal, but it is hard to follow the math here. What algorithm is being used?
There was a problem hiding this comment.
The idea is to find biggest possible MTRR size lower or equal to ACM base address alignment. Then as much as possible MTRRs with that size are used, without exceeding ACM size. The rest of ACM is covered with smaller and smaller regions, until all of the ACM (rounded up to page size, this is requirement described in SDG 2.2.5.1 "MTRR Setup Prior to GETSEC[SENTER] Execution") is covered.
This loop may be executed more than once if ACM is poorly aligned, say, has size of 512 KiB but is loaded to address aligned to 64 KiB. It will use only 64 KiB regions, even if it could use bigger regions after few iterations, so it isn't optimal, but trying to make it so would require in much deeper loops. This is one-time operation, and MTRRs will soon be restored by Xen anyway, so there is little to be gained here. The only possible problem may be running out of MTRRs, but in practice all SINIT base addresses we've seen are aligned to at least 1 MiB.
| + /* | ||
| + * TODO: TXT spec: Note: BiosDataSize + OsMleDataSize + OsSinitDataSize + SinitMleDataSize | ||
| + * must be less than or equal to TXT.HEAP.SIZE, TXT spec, p. 102. | ||
| + */ |
| + return ((((mle_size / GRUB_PAGE_SIZE) + 2) / 512) + 1 + | ||
| + (((mle_size / (512 * GRUB_PAGE_SIZE)) + 2) / 512) + 1 + 1) * GRUB_PAGE_SIZE; |
There was a problem hiding this comment.
Please break this up into multiple statements with intermediate variables so that this is easier to read.
There was a problem hiding this comment.
This code doesn't agree with what happens in next function, and neither of these fits the description above that function. I'll have to take a better look at this and guess what author had in mind, AFAICT the logic was copied from tboot with minimal modifications, other than GRUB-ification.
There was a problem hiding this comment.
@DemiMarie thanks for the review, ack on most comments, I replied to some of the rest.
Note that there will be a bigger change caused by a rebase to newer Secure Launch protocol. There were also some bugs we found while testing on different platforms, both with TPM 2.0 and 1.2, partially caused by Intel changing the documentation without updating ACMs.
| +static grub_err_t | ||
| +verify_acm_elt (struct grub_txt_heap_ext_data_element *elt) | ||
| +{ | ||
| + grub_uint8_t *ptr = ((grub_uint8_t *)elt + sizeof(*elt)); |
There was a problem hiding this comment.
I think I'll rework all of those elements to include struct grub_txt_heap_ext_data_element at the beginning, then all those calculations won't be necessary. Maybe even having all elements as one union type? Will need to check if GRUB code style allows it.
| + case GRUB_TXT_HEAP_EXTDATA_TYPE_MADT: | ||
| + /* Copy of ACPI MADT, not validating */ | ||
| + break; |
There was a problem hiding this comment.
Hmm, out of these, only GRUB_TXT_HEAP_EXTDATA_TYPE_BIOS_SPEC_VER, _ACM, _STM, _END (obviously) and _CUSTOM are to be found in BIOS data section according to spec, I think I'll change the code to return error instead of warning for each type not mentioned above.
_MADT in particular is part of SINIT to MLE data, which is produced by SINIT ACM. It isn't available at this point.
| }; | ||
| #endif | ||
|
|
||
| +#define OFFSET_OF(x, y) ((grub_size_t)((grub_uint8_t *)(&(y)->x) - (grub_uint8_t *)(y))) |
There was a problem hiding this comment.
How portable is it?
| - if (grub_add (ctx.real_size, efi_mmap_size, &sz)) | ||
| + if (grub_add (ctx.real_size, efi_mmap_size + ap_wake_block_size, &sz)) |
There was a problem hiding this comment.
The latest version shouldn't need AP wake block at all, this is a leftover from Tboot way of booting other cores.
| + } | ||
| + else | ||
| + { | ||
| + /* FIXME. */ |
There was a problem hiding this comment.
This is part of the original code, just at different indentation level. I have no idea what the author had in mind so this is left as it was before.
| + base_v = base_v >> 1; | ||
| + } | ||
| + | ||
| + for (j = i - 12; j > 0; j--) |
There was a problem hiding this comment.
Will it? Both j and i are signed, so subtraction would give negative result and the loop won't be entered at all.
| + while ( num_pages >= mtrr_s ) | ||
| + { | ||
| + mtrr_physbase.raw = grub_rdmsr (GRUB_MSR_X86_MTRR_PHYSBASE0 + ndx*2); | ||
| + mtrr_physbase.base = ((unsigned long)base >> GRUB_PAGE_SHIFT) & | ||
| + SINIT_MTRR_MASK; | ||
| + mtrr_physbase.type = mem_type; | ||
| + grub_wrmsr (GRUB_MSR_X86_MTRR_PHYSBASE0 + ndx*2, mtrr_physbase.raw); | ||
| + | ||
| + mtrr_physmask.raw = grub_rdmsr (GRUB_MSR_X86_MTRR_PHYSMASK0 + ndx*2); | ||
| + mtrr_physmask.mask = ~(mtrr_s - 1) & SINIT_MTRR_MASK; | ||
| + mtrr_physmask.v = 1; | ||
| + grub_wrmsr (GRUB_MSR_X86_MTRR_PHYSMASK0 + ndx*2, mtrr_physmask.raw); | ||
| + | ||
| + base += (mtrr_s * GRUB_PAGE_SIZE); | ||
| + num_pages -= mtrr_s; | ||
| + ndx++; | ||
| + if ( ndx == vcnt ) | ||
| + return grub_error (GRUB_ERR_BAD_DEVICE, | ||
| + N_("exceeded number of var MTRRs when mapping range")); | ||
| + } |
There was a problem hiding this comment.
The idea is to find biggest possible MTRR size lower or equal to ACM base address alignment. Then as much as possible MTRRs with that size are used, without exceeding ACM size. The rest of ACM is covered with smaller and smaller regions, until all of the ACM (rounded up to page size, this is requirement described in SDG 2.2.5.1 "MTRR Setup Prior to GETSEC[SENTER] Execution") is covered.
This loop may be executed more than once if ACM is poorly aligned, say, has size of 512 KiB but is loaded to address aligned to 64 KiB. It will use only 64 KiB regions, even if it could use bigger regions after few iterations, so it isn't optimal, but trying to make it so would require in much deeper loops. This is one-time operation, and MTRRs will soon be restored by Xen anyway, so there is little to be gained here. The only possible problem may be running out of MTRRs, but in practice all SINIT base addresses we've seen are aligned to at least 1 MiB.
| + N_("exceeded number of var MTRRs when mapping range")); | ||
| + } | ||
| + | ||
| + ndx = 0; |
| + if ( err ) | ||
| + return err; |
There was a problem hiding this comment.
Safe? Yes. Fast? No.
I guess it would be better to just continue executing this function and return err instead of GRUB_ERR_NONE.
| + if (rsdp == NULL) | ||
| + return grub_error (GRUB_ERR_BAD_DEVICE, N_("ACPI RSDP 2.0 missing\n")); | ||
| + | ||
| + os_sinit_data->efi_rsdt_ptr = (grub_uint64_t)(grub_addr_t) rsdp; |
There was a problem hiding this comment.
We can't change the definition of fields consumed by SINIT. It expects just a pointer, so we give it a pointer. It does something with ACPI tables, but your guess is as good as mine. MLE should validate any "external" data, including all of ACPI.
| + if ((addr < al->limit) && ((addr + size) > al->limit)) | ||
| + al->addr = al->limit; |
There was a problem hiding this comment.
A common bounds-checking ideom in C is:
if (a > b || c > b - a)
goto fail;but grub_add is obviously much clearer.
| + if (type != GRUB_MEMORY_AVAILABLE) | ||
| + return 0; | ||
| + | ||
| + if ((addr + size) < al->limit) |
There was a problem hiding this comment.
Is not changing al safe from a security perspective? If not then just panic.
| +#define GRUB_MSR_X86_APICBASE 0x0000001b | ||
| +#define GRUB_MSR_X86_APICBASE_BSP (1<<8) | ||
| +#define GRUB_MSR_X86_APICBASE_ENABLE (1<<11) | ||
| +#define GRUB_MSR_X86_APICBASE_BASE (0xfffff<<12) |
There was a problem hiding this comment.
Comments after each of these #defines would be appreciated, as was done for GRUB_EFLAGS_* above.
| +#define GRUB_MSR_AMD64_PATCH_LEVEL 0x0000008b | ||
| +#define GRUB_MSR_AMD64_PATCH_CLEAR 0xc0010021 /* AMD-specific microcode patch clear */ | ||
| +#define GRUB_MSR_AMD64_VM_CR 0xc0010114 | ||
| +#define GRUB_MSR_SVM_VM_CR_SVM_DISABLE 4 |
There was a problem hiding this comment.
Can each of these have a comment after them?
If possible, the code should be understandable without having to have the vendor’s manuals open at the same time.
| +#define GRUB_TXT_ERRORCODE 0x0030 | ||
| +#define GRUB_TXT_CMD_RESET 0x0038 | ||
| +#define GRUB_TXT_CMD_CLOSE_PRIVATE 0x0048 | ||
| +#define GRUB_TXT_VER_FSBIF 0x0100 |
There was a problem hiding this comment.
That’s fine, but please add comments next to the register definitions. I should be able to understand this without having to look at the SDM, especially since the SDM can change and I am not sure where to find an old version that does document this.
| + struct | ||
| + { | ||
| + grub_uint64_t reserved1 : 11; | ||
| + grub_uint64_t v : 1; /* valid */ | ||
| + grub_uint64_t mask : 52; /* define as max width and mask w/ */ | ||
| + /* MAXPHYADDR when using */ | ||
| + }; |
There was a problem hiding this comment.
Bitfields often result in poorly optimized code from the compiler (per Linus Torvalds), but that doesn’t matter here. The bitfield is more readable, so just add a comment explaining why this works.
| + base_v = base_v >> 1; | ||
| + } | ||
| + | ||
| + for (j = i - 12; j > 0; j--) |
There was a problem hiding this comment.
Ah, I forgot that i and j are signed integers. Peril of reviewing code on GitHub, rather than in a local text editor.
| }; | ||
| #endif | ||
|
|
||
| +#define OFFSET_OF(x, y) ((grub_size_t)((grub_uint8_t *)(&(y)->x) - (grub_uint8_t *)(y))) |
There was a problem hiding this comment.
Both have been used since beginning of Linux git history.
| - if (grub_add (ctx.real_size, efi_mmap_size, &sz)) | ||
| + if (grub_add (ctx.real_size, efi_mmap_size + ap_wake_block_size, &sz)) |
| + { | ||
| + slparams->mle_header_offset = 0xffffffff; | ||
| + | ||
| + for (mle_hdr_offset = 0; mle_hdr_offset < 0x1000; mle_hdr_offset += 16) |
There was a problem hiding this comment.
A scan seems like a really bad idea. I would add a new tag.
Signed-off-by: Krystian Hebel <[email protected]>
There was a problem hiding this comment.
Some of us are having difficulties opening this PR in the browser, probably due to its size. Opening in private mode makes it possible to view it in normal mode, logged in, but sometimes only once. Would it be OK to split it into two smaller ones if that problem makes it impossible to reply? I was thinking about separating it right after TPM changes, everything up to that point is more or less independent of TXT and Slaunch stuff.
| + return *(grub_uint64_t *)(heap + grub_txt_bios_data_size (heap) + | ||
| + grub_txt_os_mle_data_size (heap) + | ||
| + grub_txt_os_sinit_data_size (heap)); |
There was a problem hiding this comment.
To be honest, I didn't believe you that this can be UB, I had to check. I can more or less understand reasoning behind other undefined behaviors, but this one doesn't make any sense to me. I would expect it to be implementation-defined, as are alignments of the fields in structures, but not UB.
In any case, I'm considering incorporating those size fields into respective grub_txt_*_data structures. This would be slightly different than what SDG suggests, but much nicer to work with in my opinion.
| + | ||
| +#define GRUB_SMX_PROCESSOR_BASE_SCRTM 0x00000020 | ||
| +#define GRUB_SMX_MACHINE_CHECK_HANLDING 0x00000040 | ||
| +#define GRUB_SMX_GET_TXT_EXT_FEATURES(v) (v & (GRUB_SMX_PROCESSOR_BASE_SCRTM|GRUB_SMX_MACHINE_CHECK_HANLDING)) |
| + if ( enable ) | ||
| + mtrr_def_type |= GRUB_MSR_X86_MTRR_ENABLE; | ||
| + else | ||
| + mtrr_def_type &= ~GRUB_MSR_X86_MTRR_ENABLE; |
There was a problem hiding this comment.
This may not be popular opinion, but I'm against using -Werror=conversion, it often (at least in my experience) leads to bad decisions. For example, in this case the error message would suggest to make GRUB_MSR_X86_MTRR_ENABLE unsigned (e.g. by using (1u<<11)). This would make the error go away, but then the value would actually be truncated (unless MASK_NEGATE is used).
In any case, I still believe that the current code is well defined:
GRUB_MSR_X86_MTRR_ENABLEhas a constant, relatively small value that isn't in any way modified, neither by shifting that would impact sign bit nor by user input.~operator causes that constant to undergo integer promotion. As the value is small enough to be represented byint, that type is used (instead ofunsigned int), so~operates on and returnsint. As the sign bit inGRUB_MSR_X86_MTRR_ENABLEwas not set, it is set in the result, which makes it a negative signedint.&=follows the usual arithmetic conversions rules. In this case, both values are integers, they differ in type and in signedness, and unsigned type has greater conversion rank than a signed one. In that case, the operand with the signed type (int) is implicitly converted to the unsigned type (grub_uint64_t). This follows integer conversion rules. First point from that link doesn't apply, sincegrub_uint64_tcan't represent negative result of~. Second point is used, i.e. 2^64 is added to that result, which gives a number that can be represented bygrub_uint64_t, so that value is used.
| + while ( (base_v & 0x01) == 0 ) | ||
| + { | ||
| + i++; | ||
| + base_v = base_v >> 1; | ||
| + } |
There was a problem hiding this comment.
I've added a test for proper alignment to code that reads it from SINIT.BASE register instead. CPU requires that alignment, may as well fail before messing with MTRRs.
| + return ((((mle_size / GRUB_PAGE_SIZE) + 2) / 512) + 1 + | ||
| + (((mle_size / (512 * GRUB_PAGE_SIZE)) + 2) / 512) + 1 + 1) * GRUB_PAGE_SIZE; |
There was a problem hiding this comment.
This code doesn't agree with what happens in next function, and neither of these fits the description above that function. I'll have to take a better look at this and guess what author had in mind, AFAICT the logic was copied from tboot with minimal modifications, other than GRUB-ification.
| + if ( ptr != NULL ) | ||
| + { | ||
| + if ( info_table_get (ptr, length) < info_table_size ) | ||
| + return NULL; | ||
| + | ||
| + info_table_size = info_table_get (ptr, length); | ||
| + ptr = fits_in_acm(hdr, user_area_off, info_table_size); | ||
| + } |
There was a problem hiding this comment.
Why? First invocation only checks if grub_txt_acm_info_table->length fits in ACM, and this part checks if whole info table (with size specified by grub_txt_acm_info_table->length) fits. We can't depend on length being valid unless we know that it is part of ACM, because reading anything past it may have side effects.
| + } | ||
| + | ||
| + if (sinit != NULL) | ||
| + grub_dprintf ("slaunch", "SINIT ACM date: %" PRIxGRUB_UINT32_T "\n", sinit->date); |
There was a problem hiding this comment.
This is BCD date in YYYYMMDD format, what is the use of 0x prefix?
| + { | ||
| + slparams->mle_header_offset = 0xffffffff; | ||
| + | ||
| + for (mle_hdr_offset = 0; mle_hdr_offset < 0x1000; mle_hdr_offset += 16) |
There was a problem hiding this comment.
But tags are also found by scanning the file, so I don't see how it would be better.
Anyway, we will be soon looking at implementing AEM for AMD, which may have to use different header (but we hope to keep it identical for simplicity). I'd rather hold with defining new tag until we know what data it should hold for both implementations.
| + | ||
| +/* MTRR Specific */ | ||
| +#define GRUB_MTRR_MEMORY_TYPE_UC 0 | ||
| +#define GRUB_MTRR_MEMORY_TYPE_WC 1 | ||
| +#define GRUB_MTRR_MEMORY_TYPE_WT 4 | ||
| +#define GRUB_MTRR_MEMORY_TYPE_WP 5 | ||
| +#define GRUB_MTRR_MEMORY_TYPE_WB 6 |
There was a problem hiding this comment.
| + | |
| +/* MTRR Specific */ | |
| +#define GRUB_MTRR_MEMORY_TYPE_UC 0 | |
| +#define GRUB_MTRR_MEMORY_TYPE_WC 1 | |
| +#define GRUB_MTRR_MEMORY_TYPE_WT 4 | |
| +#define GRUB_MTRR_MEMORY_TYPE_WP 5 | |
| +#define GRUB_MTRR_MEMORY_TYPE_WB 6 |
These seem to be unused.
| +#define GRUB_SLR_ENTRY_INVALID 0x0000 | ||
| +#define GRUB_SLR_ENTRY_DL_INFO 0x0001 | ||
| +#define GRUB_SLR_ENTRY_LOG_INFO 0x0002 | ||
| +#define GRUB_SLR_ENTRY_DRTM_POLICY 0x0003 | ||
| +#define GRUB_SLR_ENTRY_INTEL_INFO 0x0004 | ||
| +#define GRUB_SLR_ENTRY_AMD_INFO 0x0005 | ||
| +#define GRUB_SLR_ENTRY_ARM_INFO 0x0006 | ||
| +#define GRUB_SLR_ENTRY_UEFI_INFO 0x0007 | ||
| +#define GRUB_SLR_ENTRY_UEFI_CONFIG 0x0008 | ||
| +#define GRUB_SLR_ENTRY_END 0xffff |
There was a problem hiding this comment.
Would you mind including the corresponding structure types?
| +#define GRUB_SLR_ENTRY_INVALID 0x0000 | ||
| +#define GRUB_SLR_ENTRY_DL_INFO 0x0001 | ||
| +#define GRUB_SLR_ENTRY_LOG_INFO 0x0002 | ||
| +#define GRUB_SLR_ENTRY_DRTM_POLICY 0x0003 |
There was a problem hiding this comment.
What struct types does this use?
| +struct grub_slr_entry_amd_info | ||
| +{ | ||
| + struct grub_slr_entry_hdr hdr; | ||
| +} GRUB_PACKED; | ||
| + | ||
| +/* | ||
| + * ARM DRTM Info table | ||
| + */ | ||
| +struct grub_slr_entry_arm_info | ||
| +{ | ||
| + struct grub_slr_entry_hdr hdr; | ||
| +} GRUB_PACKED; |
There was a problem hiding this comment.
Are these usable? Right now, it seems like these structs will only ever have a header, which is obviously wrong. Should these be marked as reserved instead?
| + struct grub_slr_entry_hdr *next = (struct grub_slr_entry_hdr *) | ||
| + ((grub_uint8_t *) curr + curr->size); | ||
| + | ||
| + if ((void *)next >= grub_slr_end_of_entries (table)) | ||
| + return NULL; | ||
| + if (next->tag == GRUB_SLR_ENTRY_END) | ||
| + return NULL; | ||
| + | ||
| + return next; |
There was a problem hiding this comment.
This creates an out of bounds pointer if the size is too large. This is undefined behavior in standard C, and it could theoretically wrap if this is too close to the top of the address space. Perhaps something like this?
| + struct grub_slr_entry_hdr *next = (struct grub_slr_entry_hdr *) | |
| + ((grub_uint8_t *) curr + curr->size); | |
| + | |
| + if ((void *)next >= grub_slr_end_of_entries (table)) | |
| + return NULL; | |
| + if (next->tag == GRUB_SLR_ENTRY_END) | |
| + return NULL; | |
| + | |
| + return next; | |
| + grub_uint8_t *end = grub_slr_end_of_entries (table); | |
| + grub_uint8_t *next; | |
| + size_t remaining; | |
| + | |
| + if (end < (grub_uint8_t *)curr) | |
| + return NULL; | |
| + remaining = end - (grub_uint8_t *)curr; | |
| + if (remaining < sizeof(*curr) || | |
| + curr->size < sizeof(*curr) || | |
| + remaining < curr->size) | |
| + return NULL; | |
| + | |
| + next = (struct grub_slr_entry_hdr *)((grub_uint8_t *)curr + cur->size); | |
| + if (next->tag == GRUB_SLR_ENTRY_END) | |
| + return NULL; | |
| + remaining = (size_t)(end - (grub_uint8_t *)next); | |
| + if (remaining < next->size) | |
| + return NULL; | |
| + | |
| + return next; |
However, this API is not great for iterating through an untrusted table. I would have something like,
/** Returns the first entry in the table, or NULL if the table is invalid. A table is invalid if:
*
* - it is empty
* - it does not fit in `size` bytes.
* - the first entry does not fit in the table.
*/
const struct grub_slr_entry_hdr *grub_slr_entry_first(const struct grub_slr_table *table, size_t size);
/** Returns the next entry in the table, or NULL if any of the following hold:
*
* - There are no more entries in the table.
* - The entry has tag GRUB_SLR_TAG_END.
* - The entry is too large to fit in the table.
* - The entry has a wrong size for its tag.
*/
const struct grub_slr_entry_hdr *grub_slr_entry_next(const struct grub_slr_table *table, const grub_slr_entry_hdr *hdr);| +#define GRUB_EFLAGS_X86_NT 0x00004000 /* Nested Task */ | ||
| +#define GRUB_EFLAGS_X86_RF 0x00010000 /* Resume Flag */ | ||
| +#define GRUB_EFLAGS_X86_VM 0x00020000 /* Virtual Mode */ | ||
| +#define GRUB_EFLAGS_X86_AC 0x00040000 /* Alignment Check */ |
There was a problem hiding this comment.
| +#define GRUB_EFLAGS_X86_AC 0x00040000 /* Alignment Check */ | |
| +#define GRUB_EFLAGS_X86_AC 0x00040000 /* Alignment Check/Allow Kernel Access to Userspace Pages */ |
| +static inline unsigned long | ||
| +grub_read_flags_register(void) | ||
| +{ | ||
| + unsigned long flags; | ||
| + | ||
| +#ifdef __x86_64__ | ||
| + asm volatile ("pushfq; popq %0" : "=r" (flags)); | ||
| +#else | ||
| + asm volatile ("pushfl; popl %0" : "=r" (flags)); | ||
| +#endif | ||
| + | ||
| + return flags; | ||
| +} |
There was a problem hiding this comment.
Should this also use grub_uint32_t?
| +#define GRUB_MSR_X86_APICBASE 0x0000001b | ||
| +#define GRUB_MSR_X86_APICBASE_BSP (1<<8) | ||
| +#define GRUB_MSR_X86_APICBASE_ENABLE (1<<11) | ||
| +#define GRUB_MSR_X86_APICBASE_BASE (0xfffff<<12) /* Mask for APIC base address */ |
There was a problem hiding this comment.
Does this need a cast to int to make sure it is signed?
| +#define GRUB_MSR_X86_MASK_VALID (1<<11) | ||
| + | ||
| +#define GRUB_MSR_X86_MTRR_DEF_TYPE 0x000002ff | ||
| +#define GRUB_MSR_X86_DEF_TYPE_MASK 0xff |
There was a problem hiding this comment.
| +#define GRUB_MSR_X86_DEF_TYPE_MASK 0xff | |
| +#define GRUB_MSR_X86_DEF_TYPE_MASK 0x7 |
| +#define GRUB_MSR_EFER_LME (1<<8) /* Enable Long Mode/IA-32e */ | ||
| +#define GRUB_MSR_EFER_LMA (1<<10) /* Long Mode/IA-32e Active */ | ||
| +#define GRUB_MSR_EFER_SVME (1<<12) /* Enable SVM (AMD-V) */ |
There was a problem hiding this comment.
Intel only documents bit 11 (No Execute).
Referring to earlier comments will be problematic, but since I doubt the situation will improve with even more comments, I guess we have not much of a choice. @DemiMarie are you okay with that? when would be a good time to do that? |
I’m definitely fine with that. Another option would be:
This would be much easier for me to review, as it is what GitHub is designed to work with. Right now, I have to review patches of patches, which is quite annoying. |
|
I think this is more or less TrenchBoot/grub#16, @krystian-hebel can you confirm? |
It is now. I had to change the base, diff shown there used to be between previous release and what was sent here, but now it should be the same. New base is 2.06 with 3 commits for CI on top, so it should even build RPM packages in case someone is brave enough to test it. Patches sent here were prepared from |
Reviewed on TrenchBoot/grub#16 Signed-off-by: Krystian Hebel <[email protected]>
|
PipelineRetry |
1 similar comment
|
PipelineRetry |
|
openQArun |
1 similar comment
|
openQArun |
OpenQA test summaryComplete test suite and dependencies: https://openqa.qubes-os.org/tests/overview?distri=qubesos&version=4.2&build=202404081323-4.2&flavor=pull-requests New failures, excluding unstableCompared to: https://openqa.qubes-os.org/tests/overview?distri=qubesos&version=4.2&build=2024031904-4.2&flavor=update
Failed tests25 failures
Fixed failuresCompared to: https://openqa.qubes-os.org/tests/94176#dependencies 52 fixed
Unstable tests
|
|
FYI our CI has created a repo with this package, to use it one needs with the key file downloaded from https://gitlab.com/QubesOS/qubes-grub2/-/jobs/6564370619/artifacts/raw/repo/key.pub |
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]>
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]>
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]>
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]>
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]>
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]>
Set of patches which adds TPM1.2 support for Intel TXT in GRUB2 for
TrenchBoot.
This is necessary to create Proof of Concept for TrenchBoot Anti Evil
Maid for QubesOS.
The TrenchBoot support hasn't been implemented and verified with TPM 1.2
on Intel TXT path. This changes ensures the TPM 1.2 is also supported
for older Intel hardware with Intel TXT.
Signed-off-by: Tomasz Żyjewski [email protected]