Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider: - [Critical] x86/hyperv: premature zeroing in hv_vtom_clear_present() before transition - [High] Drivers: hv: uninitialized SynIC pages in hv_alloc_page() on non-CoCo VMs - [High] dma-pool: dirty cache lines left on linear alias by set_memory_decrypted() - [Medium] x86/mm: set_memory_decrypted() zeroes legacy VGA VRAM/MMIO in VGA_MAP_MEM() - [Medium] powerpc/pseries/svm: set_memory_decrypted() zeroes private page before sharing --
commit 0a47431deefcabfb66f48104fa0c06cece49f6f0 Author: Aneesh Kumar K.V (Arm) <[email protected]> mm: Zero memory during shared memory transitions This commit moves memory zeroing into the architecture-specific implementations of set_memory_decrypted() so each platform clears shared memory at the appropriate point in the private-to-shared transition. It also removes post-transition memset() calls from callers and keeps explicit zeroing on paths where no transition takes place. > diff --git a/arch/powerpc/platforms/pseries/svm.c > b/arch/powerpc/platforms/pseries/svm.c > index 7a403dbd35ee5..a344e094fe1c8 100644 > --- a/arch/powerpc/platforms/pseries/svm.c > +++ b/arch/powerpc/platforms/pseries/svm.c [ ... ] > @@ -59,6 +60,7 @@ int set_memory_decrypted(unsigned long addr, int numpages) > if (!PAGE_ALIGNED(addr)) > return -EINVAL; > > + memset((void *)addr, 0, (size_t)numpages << PAGE_SHIFT); > uv_share_page(PHYS_PFN(__pa(addr)), numpages); > > return 0; [Severity: Medium] Should the memset() in set_memory_decrypted() happen after uv_share_page() rather than before it? On PowerPC pSeries Secure VMs, memory is partitioned between secure memory and normal hypervisor-accessible memory rather than shared in place. When uv_share_page() is called, the Ultravisor unmaps the secure page and the hypervisor sets pvt->skip_page_out = true in kvmppc_share_page() before allocating a new normal page and mapping it into the guest: arch/powerpc/kvm/book3s_hv_uvmem.c:kvmppc_share_page() { if (kvmppc_gfn_is_uvmem_pfn(gfn, kvm, &uvmem_pfn)) { uvmem_page = pfn_to_page(uvmem_pfn); pvt = uvmem_page->zone_device_data; pvt->skip_page_out = true; ... } Zeroing addr before uv_share_page() writes zeros to the private secure page that is immediately unmapped and discarded (and faults in a secure page first if one was not yet populated), instead of zeroing the shared mapping after the transition completes. [ ... ] > diff --git a/arch/x86/hyperv/ivm.c b/arch/x86/hyperv/ivm.c > index 2ce4dfe534727..e9c7799a00943 100644 > --- a/arch/x86/hyperv/ivm.c > +++ b/arch/x86/hyperv/ivm.c > @@ -755,6 +755,10 @@ static int hv_mark_gpa_visibility(u16 count, const u64 > pfn[], > */ > static int hv_vtom_clear_present(unsigned long kbuffer, int pagecount, bool > enc) > { > + if (!enc) > + memset((void *)kbuffer, 0, > + (size_t)pagecount << PAGE_SHIFT); > + > return set_memory_np(kbuffer, pagecount); > } [Severity: Critical] As reported by Sashiko on RFC v8 [1], doesn't zeroing kbuffer in hv_vtom_clear_present() clear the private mapping before the transition to shared completes, leaving the final shared mapping unzeroed? In hv_vtom_init(), hv_vtom_clear_present() is registered as x86_platform.guest.enc_status_change_prepare, while hv_vtom_set_host_visibility() is registered as x86_platform.guest.enc_status_change_finish. When set_memory_decrypted() runs on Hyper-V Isolation VMs using vTOM or a paravisor, __set_memory_enc_pgtable() invokes the callbacks in this order: __set_memory_enc_pgtable() -> hv_vtom_clear_present() (prepare: kbuffer is still private) -> __change_page_attr_set_clr() (updates PTEs with shared_gpa_boundary) -> hv_vtom_set_host_visibility() (finish: marks GPA host-visible and restores _PAGE_PRESENT via set_memory_p()) Because memset() runs in hv_vtom_clear_present() while kbuffer still maps the encrypted/private GPA below ms_hyperv.shared_gpa_boundary, the shared mapping above vTOM is never zeroed after hv_vtom_set_host_visibility() makes the pages host-visible and present again. With the post-transition memset() calls removed from callers such as vmbus_connect(), hv_alloc_page(), dma_direct_alloc(), dma_direct_alloc_pages(), and __alloc_cc_shared_pages_node(), shared buffers on vTOM VMs will contain ciphertext or uninitialized data instead of zeros. Should this memset() be moved to the end of hv_vtom_set_host_visibility() after set_memory_p() succeeds? [ ... ] > diff --git a/arch/x86/mm/mem_encrypt_amd.c b/arch/x86/mm/mem_encrypt_amd.c > index 2f8c32173972d..a1199a74e9a3f 100644 > --- a/arch/x86/mm/mem_encrypt_amd.c > +++ b/arch/x86/mm/mem_encrypt_amd.c [ ... ] > @@ -308,6 +309,9 @@ static int amd_enc_status_change_finish(unsigned long > vaddr, int npages, bool en > if (!cc_platform_has(CC_ATTR_HOST_MEM_ENCRYPT)) > enc_dec_hypercall(vaddr, npages << PAGE_SHIFT, enc); > > + if (!enc) > + memset((void *)vaddr, 0, (size_t)npages << PAGE_SHIFT); > + > return 0; > } [Severity: Medium] What happens during vgacon_startup() when VGA_MAP_MEM() calls set_memory_decrypted() on the legacy VGA video memory range? In arch/x86/include/asm/vga.h, VGA_MAP_MEM() calls set_memory_decrypted() on the direct-map address of legacy VGA memory (0xb8000 or 0xb0000): #define VGA_MAP_MEM(x, s) \ ({ \ unsigned long start = (unsigned long)phys_to_virt(x); \ \ if (IS_ENABLED(CONFIG_AMD_MEM_ENCRYPT)) \ set_memory_decrypted(start, (s) >> PAGE_SHIFT); \ \ start; \ }) During console initialization, con_init() -> vgacon_startup() calls VGA_MAP_MEM(vga_vram_base, vga_vram_size) before probing for VGA hardware and before vgacon_save_screen() saves the pre-boot screen contents. With amd_enc_status_change_finish() and tdx_enc_status_change_finish() now unconditionally calling memset() on !enc: 1. On AMD SME/SEV, set_memory_decrypted() wipes the 32 KiB VGA text buffer before vgacon_startup() checks the buffer or saves the initial screen. 2. On AMD SEV-ES/SEV-SNP and Intel TDX guests, 0xb8000-0xbffff is an MMIO range; executing memset() (which uses rep stosb when X86_FEATURE_FSRS is set) on that shared MMIO range triggers a #VC or #VE exception that insn_decode_mmio() cannot decode, panicking the kernel at boot. [ ... ] > diff --git a/drivers/hv/hv.c b/drivers/hv/hv.c > index fe50090dcc011..6e836a02910a0 100644 > --- a/drivers/hv/hv.c > +++ b/drivers/hv/hv.c > @@ -123,12 +123,13 @@ static int hv_alloc_page(void **page, bool decrypt, > const char *note) > if (!*page) > return -ENOMEM; > > - if (decrypt) > + if (decrypt) { > ret = set_memory_decrypted((unsigned long)*page, 1); > - if (ret) > - goto failed; > - > - memset(*page, 0, PAGE_SIZE); > + if (ret) > + goto failed; > + } else { > + memset(*page, 0, PAGE_SIZE); > + } > return 0; > > failed: [Severity: High] As reported by Sashiko on RFC v8 [2], doesn't skipping memset() when decrypt is true leave SynIC message and event pages uninitialized on standard (non-CoCo) Hyper-V VMs? In hv_synic_alloc(), decrypt is set to !vmbus_is_confidential(): drivers/hv/hv.c:hv_synic_alloc() { const bool decrypt = !vmbus_is_confidential(); ... if (!ms_hyperv.paravisor_present && !hv_root_partition()) { ret = hv_alloc_page(&hv_cpu->hyp_synic_message_page, decrypt, "hypervisor SynIC msg"); if (ret) goto err; ret = hv_alloc_page(&hv_cpu->hyp_synic_event_page, decrypt, "hypervisor SynIC event"); ... } On a standard non-confidential Hyper-V guest, vmbus_is_confidential() is false, so decrypt is true. However, on non-CoCo x86 and arm64 guests where memory encryption is inactive, set_memory_decrypted() is a no-op that returns 0 without zeroing the page. Because hv_alloc_page() allocates the page with __get_free_page(GFP_KERNEL) without __GFP_ZERO and skips the else branch when decrypt is true, hyp_synic_message_page and hyp_synic_event_page are handed to the hypervisor with uninitialized kernel memory, leaving non-zero garbage in message slot headers (msg->header.message_type != HVMSG_NONE) and event flags. [ ... ] > diff --git a/drivers/virt/coco/pkvm-guest/arm-pkvm-guest.c > b/drivers/virt/coco/pkvm-guest/arm-pkvm-guest.c > index 26fe9c3f22e36..7d922e0a28f96 100644 > --- a/drivers/virt/coco/pkvm-guest/arm-pkvm-guest.c > +++ b/drivers/virt/coco/pkvm-guest/arm-pkvm-guest.c [ ... ] > @@ -61,6 +62,8 @@ static int pkvm_set_memory_encrypted(unsigned long addr, > int numpages) > > static int pkvm_set_memory_decrypted(unsigned long addr, int numpages) > { > + memset((void *)addr, 0, (size_t)numpages << PAGE_SHIFT); > + > return __set_memory_range(ARM_SMCCC_VENDOR_HYP_KVM_MEM_SHARE_FUNC_ID, > addr, numpages); > } [Severity: High] When atomic_pool_expand() expands a CoCo atomic DMA pool on arm64 (both here in pkvm_set_memory_decrypted() and in realm_set_memory_decrypted()), doesn't zeroing via the cacheable linear map allocate dirty cache lines after arch_dma_prep_coherent() has already cleaned the cache? In kernel/dma/pool.c:atomic_pool_expand(), arch_dma_prep_coherent() and the non-cacheable vmap remap happen before set_memory_decrypted() is called on page_to_virt(page): kernel/dma/pool.c:atomic_pool_expand() { ... arch_dma_prep_coherent(page, pool_size); #ifdef CONFIG_DMA_DIRECT_REMAP if (dma_pool->cc_shared) prot = pgprot_decrypted(pgprot_dmacoherent(PAGE_KERNEL)); else prot = pgprot_dmacoherent(PAGE_KERNEL); addr = dma_common_contiguous_remap(page, pool_size, prot, __builtin_return_address(0)); if (!addr) goto free_page; #else addr = page_to_virt(page); #endif if (dma_pool->cc_shared) { ret = set_memory_decrypted((unsigned long)page_to_virt(page), 1 << order); ... } Because pkvm_set_memory_decrypted() and realm_set_memory_decrypted() execute memset() through the cacheable linear mapping page_to_virt(page) after arch_dma_prep_coherent() has already run, dirty zeroed cache lines remain in the CPU data cache for the linear alias while consumers of the atomic pool access the buffer through the non-cacheable vmap alias addr or via device DMA. If those dirty cache lines are later evicted, won't they overwrite data written via the non-cacheable mapping or by DMA? [1] https://lore.kernel.org/r/[email protected] [2] https://lore.kernel.org/r/[email protected] -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=2
