Re: [PATCH v9 02/14] mm: Zero memory during shared memory transitions

[sashiko-bot@kernel.org]

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) <aneesh.kumar@kernel.org>

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/20260924100529.1398790-3-aneesh.kumar@kernel.org
[2] https://lore.kernel.org/r/20260924100529.1398790-3-aneesh.kumar@kernel.org

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261104164555.3555725-1-aneesh.kumar@kernel.org?part=2