Re: [RFC PATCH 08/12] arch: x86: hyperv: Build initial vCPU context for VTL1 secure kernel

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] Page table initialization out-of-bounds write to the PDE page if the mapping crosses a 1GB boundary.
- [High] Hardware-consumed GDT, TSS, and Kernel Stack pages are mapped but never zero-initialized, exposing host memory fragments.
- [Low] The TSS GDT descriptor limit is set to `tss_size` instead of `tss_size - 1`.
--

commit 6f8a1a825749fc111adb451004cf1aa1f0e24ee1
Author: Thara Gopinath <tgopinath@linux.microsoft.com>

arch: x86: hyperv: Build initial vCPU context for VTL1 secure kernel

This commit adds the architecture-specific initialization function to construct
the initial page tables, GDT/TSS, and CPU register state for transitioning the
boot processor to the VTL1 secure kernel. It allocates and populates the data
structures required by Hyper-V's hv_init_vp_context.

> diff --git a/arch/x86/hyperv/hv_vtl_vsm.c b/arch/x86/hyperv/hv_vtl_vsm.c
> new file mode 100644
> index 0000000000000..edc55264c4d87
> --- /dev/null
> +++ b/arch/x86/hyperv/hv_vtl_vsm.c
[ ... ]
> +static void __init hv_vsm_fill_pte_tables(phys_addr_t sk_pa, u64 *pde,
> +					  int pd_index, int num_pte_tables)
> +{
> +	u16 i, j;
> +	phys_addr_t pte_pa;
> +	u64 *pte;
> +
> +	/* Fill page tables with entries */
> +	for (i = 0; i < num_pte_tables; i++) {
> +		pte_pa = PAGE_AT(sk_pa, VSM_PTE_PAGES + i);
> +		pte = phys_to_virt(pte_pa);
> +		*(pde + pd_index + i) = pte_pa | VSM_PTE_FLAGS;

[Severity: High]
Does this loop overflow the 4KB PDE page if sk_pa crosses a 1GB boundary? 

The allocator only guarantees a 2MB alignment for sk_pa, so pd_index can
be near the end of the page directory (e.g., 511). Since num_pte_tables
can be up to 8 (for a 16MB initial mapping), pd_index + i will exceed the
511 maximum index of the page directory, causing an out-of-bounds write into
the adjacent VSM_PTE_PAGES memory.

[ ... ]
> +static void __init hv_vsm_init_page_tables(struct hv_init_vp_context *vp_ctx, phys_addr_t sk_pa)
> +{
[ ... ]
> +	pml4e = phys_to_virt(pml4e_pa);
> +	pdpe = phys_to_virt(pdpe_pa);
> +	pde = phys_to_virt(pde_pa);
> +
> +	/*
> +	 * Zero the PML4, PDP, PD and PTE pages before populating them so that
> +	 * any entry not explicitly written below has its present bit clear.
> +	 */
> +	memset(pml4e, 0,
> +	       (VSM_KERNEL_STACK_PAGE - VSM_PML4E_PAGE) * VSM_PAGE_SIZE);

[Severity: High]
By starting the memset at VSM_PML4E_PAGE and stopping before
VSM_KERNEL_STACK_PAGE, does this leave VSM_GDT_PAGE, VSM_TSS_PAGE, and
the kernel stack page completely uninitialized?

The underlying physical memory is not guaranteed to be zeroed by the allocator,
which would expose host memory fragments directly to the secure kernel.

[ ... ]
> +static void __init hv_vsm_init_gdt(struct hv_init_vp_context *vp_ctx, phys_addr_t sk_pa)
> +{
> +	phys_addr_t gdt_pa, tss_pa, kstack_pa;
> +	void *gdt_va;
> +	u64 tss_sk_va, gdt;
> +	struct x86_hw_tss *tss;
> +	size_t gdt_size = sizeof(gdt), tss_size = sizeof(*tss), gdt_offset = 0;
> +
> +	/* Get a page for the GDT */
> +	gdt_pa = PAGE_AT(sk_pa, VSM_GDT_PAGE);
> +	gdt_va = phys_to_virt(gdt_pa);
> +	/* Get a page for the TSS */
> +	tss_pa = PAGE_AT(sk_pa, VSM_TSS_PAGE);
> +	tss = phys_to_virt(tss_pa);

[Severity: High]
Since the memory backing tss was skipped by the memset in
hv_vsm_init_page_tables(), will the hardware task state segment be populated
with random garbage?

This would leave hardware fields like the interrupt stack tables (ist) and
the IO bitmap base uninitialized, potentially causing a fatal exception when an
interrupt is taken in VTL1.

[ ... ]
> +	/*
> +	 * Make and add a system segment descriptor for the TSS in the GDT.
> +	 *
> +	 * In 64-bit mode a system-segment descriptor (TSS/LDT) is 16 bytes
> +	 * wide: the lower 8 bytes have the same layout as the legacy 32-bit
> +	 * descriptor (produced by GDT_ENTRY), and the upper 8 bytes hold
> +	 * base[63:32] in the low 32 bits with the high 32 bits reserved 0.
> +	 * GDT_ENTRY masks base to 32 bits, so the upper half must be written
> +	 * explicitly.
> +	 */
> +	gdt = GDT_ENTRY(DESC_TSS32, tss_sk_va, tss_size);

[Severity: Low]
Is this descriptor's limit off by one?

x86 segment limits are inclusive, so shouldn't this pass tss_size - 1 rather
than tss_size to avoid allowing access one byte past the end of the TSS?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260901165647.3160413-1-tgopinath@linux.microsoft.com?part=8