Re: [GRUB PATCH RFC 15/18] i386/txt: Add Intel TXT core implementation

[Ross Philipson]

On 5/22/20 9:24 AM, Krystian Hebel wrote:
> 
> On 05.05.2020 01:21, Daniel Kiper wrote:
>> +static grub_err_t
>> +init_txt_heap (struct grub_slaunch_params *slparams, struct
>> grub_txt_acm_header *sinit)
>> +{
>> +Â grub_uint8_t *txt_heap;
>> +Â grub_uint32_t os_sinit_data_ver, sinit_caps;
>> +Â grub_uint64_t *size;
>> +Â struct grub_txt_os_mle_data *os_mle_data;
>> +Â struct grub_txt_os_sinit_data *os_sinit_data;
>> +Â struct grub_txt_heap_end_element *heap_end_element;
>> +Â struct grub_txt_heap_event_log_pointer2_1_element
>> *heap_event_log_pointer2_1_element;
>> +#ifdef GRUB_MACHINE_EFI
>> +Â struct grub_acpi_rsdp_v20 *rsdp;
>> +#endif
>> +
>> +Â /* BIOS data already verified in grub_txt_verify_platform(). */
>> +
>> +Â txt_heap = grub_txt_get_heap ();
>> +
>> +Â /* OS/loader to MLE data. */
>> +
>> +Â os_mle_data = grub_txt_os_mle_data_start (txt_heap);
>> +Â size = (grub_uint64_t *) ((grub_addr_t) os_mle_data - sizeof
>> (grub_uint64_t));
> There is 'grub_txt_os_mle_data_size()' in previous patch, it would look
> better.
>> +Â *size = sizeof (*os_mle_data) + sizeof (grub_uint64_t);
>> +
>> +Â grub_memset (os_mle_data, 0, sizeof (*os_mle_data));
>> +
>> +Â os_mle_data->version = GRUB_SL_OS_MLE_STRUCT_VERSION;
>> +Â os_mle_data->zero_page_addr = (grub_uint32_t)(grub_addr_t)
>> slparams->params;
>> +Â os_mle_data->saved_misc_enable_msr = grub_rdmsr
>> (GRUB_MSR_X86_MISC_ENABLE);
>> +
>> +Â os_mle_data->ap_wake_block = slparams->ap_wake_block;
>> +
>> +Â save_mtrrs (os_mle_data);
>> +
>> +Â /* OS/loader to SINIT data. */
>> +
>> +Â os_sinit_data_ver = grub_txt_supported_os_sinit_data_ver (sinit);
>> +
>> +Â if (os_sinit_data_ver < OS_SINIT_DATA_MIN_VER)
>> +ÂÂÂ return grub_error (GRUB_ERR_BAD_DEVICE,
>> +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂ N_("unsupported OS to SINIT data version in SINIT ACM:
>> %d"
>> +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂ " expected >= %d"), os_sinit_data_ver,
>> OS_SINIT_DATA_MIN_VER);
>> +
>> +Â os_sinit_data = grub_txt_os_sinit_data_start (txt_heap);
>> +Â size = (grub_uint64_t *) ((grub_addr_t) os_sinit_data - sizeof
>> (grub_uint64_t));
> Ditto
>> +
>> +Â *size = sizeof(grub_uint64_t) + sizeof (struct
>> grub_txt_os_sinit_data) +
>> +ÂÂÂÂÂ sizeof (struct grub_txt_heap_end_element);
>> +
>> +Â if (grub_get_tpm_ver () == GRUB_TPM_12)
>> +ÂÂÂ *size += sizeof (struct grub_txt_heap_tpm_event_log_element);
>> +Â else if (grub_get_tpm_ver () == GRUB_TPM_20)
>> +ÂÂÂ *size += sizeof (struct grub_txt_heap_event_log_pointer2_1_element);
>> +Â else
>> +ÂÂÂ return grub_error (GRUB_ERR_BAD_DEVICE, N_("unsupported TPM
>> version"));
>> +
>> +Â grub_memset (os_sinit_data, 0, *size);
>> +
>> +#ifdef GRUB_MACHINE_EFI
>> +Â rsdp = grub_acpi_get_rsdpv2 ();
>> +
>> +Â if (rsdp == NULL)
>> +ÂÂÂ return grub_printf ("WARNING: ACPI RSDP 2.0 missing\n");
>> +
>> +Â os_sinit_data->efi_rsdt_ptr = (grub_uint64_t)(grub_addr_t) rsdp;
>> +#endif
>> +
>> +Â os_sinit_data->mle_ptab = slparams->mle_ptab_target;
>> +Â os_sinit_data->mle_size = slparams->mle_size;
>> +
>> +Â os_sinit_data->mle_hdr_base = slparams->mle_header_offset;
>> +
>> +Â /* TODO: Check low PMR with RMRR. Look at relevant tboot code too. */
>> +Â /* TODO: Kernel should not allocate any memory outside of PMRs
>> regions!!! */
>> +Â os_sinit_data->vtd_pmr_lo_base = 0;
>> +Â os_sinit_data->vtd_pmr_lo_size = ALIGN_DOWN (grub_mmap_get_highest
>> (0x100000000),
>> +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ GRUB_TXT_PMR_ALIGN);
>> +
>> +Â os_sinit_data->vtd_pmr_hi_base = ALIGN_UP (grub_mmap_get_lowest
>> (0x100000000),
>> +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ GRUB_TXT_PMR_ALIGN);
>> +Â os_sinit_data->vtd_pmr_hi_size = ALIGN_DOWN (grub_mmap_get_highest
>> (0xffffffffffffffff),
>> +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ GRUB_TXT_PMR_ALIGN);
>> +Â os_sinit_data->vtd_pmr_hi_size -= os_sinit_data->vtd_pmr_hi_base;
> Could it be done with just one PMR, from 0 to top of memory, or would

No because there could be these legacy reserved regions in somewhere
below 4G that might cause hangs if you block DMA to them. So the idea is
the high PMR covers everything > 4G and the low PMR needs special logic
to not block these reserved regions. We are working on better logic for
this now.

> TXT complain?
>> +
>> +Â grub_dprintf ("slaunch",
>> +ÂÂÂÂÂÂÂ "vtd_pmr_lo_base: 0x%" PRIxGRUB_UINT64_T " vtd_pmr_lo_size: 0x%"
>> +ÂÂÂÂÂÂÂ PRIxGRUB_UINT64_T " vtd_pmr_hi_base: 0x%" PRIxGRUB_UINT64_T
>> +ÂÂÂÂÂÂÂ " vtd_pmr_hi_size: 0x%" PRIxGRUB_UINT64_T "\n",
>> +ÂÂÂÂÂÂÂ os_sinit_data->vtd_pmr_lo_base, os_sinit_data->vtd_pmr_lo_size,
>> +ÂÂÂÂÂÂÂ os_sinit_data->vtd_pmr_hi_base, os_sinit_data->vtd_pmr_hi_size);
>> +
>>
>> <snip>
>>
>> +/*
>> + * 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.
>> + */
> 
> What do you mean by "special regions"? In TXT spec it is written that
> there may be no holes
> in the virtual address space:
> 
> "There may not be any invalid (not-present) page table entries after the
> first valid
> entry (i.e. there may not be any gaps in the MLEâs linear address space)."
> 
> Also that spec:
> "A breadth-first search of page tables must produce increasing physical
> addresses."
> 
> Maybe I misunderstood, but does it mean that the paging structures
> themselves cannot
> be mapped? Or does it apply to the page tables only, not to the
> addresses of pages in PTEs?

One of the rules for building the page table for the ACM is the following:

Neither the MLE nor the page tables may overlap certain regions of memory:
 - device memory (PCI, PCIe*, etc.)
 - addresses between [640k, 1M] or above Top of Memory (TOM)
 - ISA hole (if enabled)
 - the Intel  TXT heap or SINIT memory regions
 - Intel  VT-d DMAR tables

Perhaps the comment could be better...

> 
>> +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;
> IMHO using 'grub_uint64_t' would result in less type casting and cleaner
> code below.
>> +Â 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);
>> +}
>>
>> <snip>
>>
>> +grub_err_t
>> +grub_txt_boot_prepare (struct grub_slaunch_params *slparams)
>> +{
>> +Â grub_err_t err;
>> +Â struct grub_txt_mle_header *mle_header;
>> +Â struct grub_txt_acm_header *sinit_base;
>> +
>> +Â sinit_base = grub_txt_sinit_select (grub_slaunch_module ());
>> +
>> +Â if (sinit_base == NULL)
>> +ÂÂÂ return grub_errno;
>> +
>> +Â err = init_txt_heap (slparams, sinit_base);
>> +
>> +Â if (err != GRUB_ERR_NONE)
>> +ÂÂÂ return err;
>> +
>> +Â /* Update the MLE header. */
>> +Â mle_header = (struct grub_txt_mle_header *)(grub_addr_t)
>> (slparams->mle_start + slparams->mle_header_offset);
>> +Â mle_header->first_valid_page = 0;
>> +Â mle_header->mle_end = slparams->mle_size;
>> +
>> +Â slparams->sinit_acm_base = (grub_uint32_t)(grub_addr_t) sinit_base;
>> +Â slparams->sinit_acm_size = sinit_base->size * 4;
>> +
>> +Â grub_tpm_relinquish_lcl (0);
>> +
>> +Â err = set_mtrrs_for_acmod (sinit_base);
>> +Â if (err)
>> +ÂÂÂ return grub_error (err, N_("secure launch failed to set MTRRs for
>> ACM"));
>> +
>> +Â err = grub_txt_prepare_cpu ();
>> +Â if ( err )
>> +ÂÂÂ return err;
>> +
>> +Â if (!(grub_rdmsr (GRUB_MSR_X86_APICBASE) & GRUB_MSR_X86_APICBASE_BSP))
>> +ÂÂÂ return grub_error (GRUB_ERR_BAD_DEVICE, N_("secure launch must
>> run on BSP"));
> This test should be the first one, before messing with TPM and MTTRs.
>> +
>> +Â return GRUB_ERR_NONE;
>> +}
> Best regards,
>