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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Critical] mm: ciphertext garbage in shared mappings due to zeroing in prepare callback
- [High] hv: uninitialized kernel memory leaked on non-confidential VMs
--

commit 0b9cafc26c64a65c124d5f6d15281e2bb9c41b6e
Author: Aneesh Kumar K.V (Arm) <aneesh.kumar@kernel.org>

mm: Zero memory during shared memory transitions

This commit makes zeroing an implicit property of set_memory_decrypted().
Each architecture clears memory at a safe point in its transition, and
redundant post-transition memset() calls are removed from standard allocators.

> diff --git a/arch/x86/coco/tdx/tdx.c b/arch/x86/coco/tdx/tdx.c
> --- a/arch/x86/coco/tdx/tdx.c
> +++ b/arch/x86/coco/tdx/tdx.c
> @@ -978,6 +978,9 @@ static int tdx_enc_status_change_prepare(unsigned long vaddr, int numpages,
>  					 bool enc)
>  {
> +	if (!enc)
> +		memset((void *)vaddr, 0, (size_t)numpages << PAGE_SHIFT);
> +

[Severity: Critical]
Does this write encrypted zeroes into RAM? Because this memset() in
tdx_enc_status_change_prepare() happens before the PTE encryption bit is
cleared, writing zeroes through the encrypted mapping results in ciphertext
being stored. When the mapping later transitions to shared, reading this
memory will yield that raw ciphertext.

[ ... ]

> diff --git a/arch/x86/mm/mem_encrypt_amd.c b/arch/x86/mm/mem_encrypt_amd.c
> --- a/arch/x86/mm/mem_encrypt_amd.c
> +++ b/arch/x86/mm/mem_encrypt_amd.c
> @@ -285,6 +286,9 @@ static void enc_dec_hypercall(unsigned long vaddr, unsigned long size, bool enc)
>  
>  static int amd_enc_status_change_prepare(unsigned long vaddr, int npages, bool enc)
>  {
> +	if (!enc)
> +		memset((void *)vaddr, 0, (size_t)npages << PAGE_SHIFT);
> +

[Severity: Critical]
Could amd_enc_status_change_prepare() suffer from the same ciphertext issue?
Writing zeroes to the private encrypted mapping before the transition leaves
ciphertext in RAM, which will appear as garbage once the page is mapped as
decrypted and shared.

[ ... ]

> diff --git a/drivers/hv/hv.c b/drivers/hv/hv.c
> --- 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;

[Severity: High]
Does hv_alloc_page() bypass memset() on non-confidential VMs? On standard
VMs, decrypt can evaluate to true. set_memory_decrypted() is a no-op that
returns 0 there, and since __get_free_page() doesn't use __GFP_ZERO, this
path could return completely uninitialized kernel memory to the hypervisor.

[ ... ]

> diff --git a/drivers/hv/hv_common.c b/drivers/hv/hv_common.c
> --- a/drivers/hv/hv_common.c
> +++ b/drivers/hv/hv_common.c
> @@ -505,8 +505,6 @@ int hv_common_cpu_init(unsigned int cpu)
>  				/* It may be unsafe to free 'mem' */
>  				return ret;
>  			}
> -
> -			memset(mem, 0x00, pgcount * HV_HYP_PAGE_SIZE);
>  		}

[Severity: High]
Could hv_common_cpu_init() leak uninitialized memory here? The
kmalloc_array() allocation above doesn't use __GFP_ZERO, so removing this
memset() leaves the hypercall input/output page uninitialized on standard
VMs.

[ ... ]

> diff --git a/kernel/dma/direct.c b/kernel/dma/direct.c
> --- a/kernel/dma/direct.c
> +++ b/kernel/dma/direct.c
> @@ -324,7 +324,9 @@ void *dma_direct_alloc(struct device *dev, size_t size,
>  		cpu_addr = page_address(page);
>  	}
>  
> -	memset(cpu_addr, 0, size);
> +	/* Zero after remapping because the page may be in HighMem. */
> +	if (!mark_mem_decrypt)
> +		memset(cpu_addr, 0, size);
>  
>  	if (set_uncached) {

[Severity: Critical]
Is it safe to bypass this memset() for shared CC memory in
dma_direct_alloc()? Since the memset() inside the x86 prepare callbacks
leaves ciphertext in memory, skipping the post-transition zeroing here means
we will return garbage-filled memory to callers expecting clean DMA buffers.

[ ... ]

> @@ -457,11 +463,13 @@ struct page *dma_direct_alloc_pages(struct device *dev, size_t size,
>  		return NULL;
>  
>  	cpu_addr = page_address(page);
> -	if ((attrs & __DMA_ATTR_ALLOC_CC_SHARED) &&
> -	    dma_set_decrypted(dev, cpu_addr, size))
> -		goto out_leak_pages;
>  setup_page:
> -	memset(cpu_addr, 0, size);
> +	if (mark_mem_decrypt) {
> +		if (dma_set_decrypted(dev, cpu_addr, size))
> +			goto out_leak_pages;
> +	} else {
> +		memset(cpu_addr, 0, size);
> +	}
>  	*dma_handle = phys_to_dma_direct(dev, page_to_phys(page),
>  					 attrs & __DMA_ATTR_ALLOC_CC_SHARED);

[Severity: Critical]
Does skipping the zeroing here in dma_direct_alloc_pages() for
mark_mem_decrypt also expose callers to raw ciphertext in the returned
buffer?

[ ... ]

> diff --git a/mm/cc_shared.c b/mm/cc_shared.c
> --- a/mm/cc_shared.c
> +++ b/mm/cc_shared.c
> @@ -137,9 +137,6 @@ static int __alloc_cc_shared_pages_node(int nid, gfp_t gfp,
>  		return ret;
>  	}
>  
> -	if (zero)
> -		memset(page_address(page), 0, layout.shared_size);
> -
>  	mem->page = page;
>  	mem->shared_size = layout.shared_size;

[Severity: Critical]
Can removing this memset() cause __alloc_cc_shared_pages_node() to return
memory containing ciphertext? Callers expecting zeroed memory will receive
garbage instead.

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