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) <[email protected]> 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/[email protected]?part=2
