Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider: - [High] mm: invalid cc_make_private() call on cc_make_shared() failure --
commit 8a434c0ee7399613dbf060530383d80bbc1ab7af Author: Aneesh Kumar K.V (Arm) <[email protected]> mm: Add an allocator for CoCo shared memory This commit introduces a common allocator and transition helpers for confidential-computing shared memory, including alloc_cc_shared_pages(), alloc_cc_shared_pages_node(), and free_cc_shared_pages(). > diff --git a/mm/cc_shared.c b/mm/cc_shared.c > new file mode 100644 > index 0000000000000..5c4c971cfb7b2 > --- /dev/null > +++ b/mm/cc_shared.c > @@ -0,0 +1,233 @@ [ ... ] > +static int __alloc_cc_shared_pages_node(int nid, gfp_t gfp, > + size_t requested, > + struct cc_shared_pages *mem) > +{ > + struct cc_shared_layout layout; > + struct page *page; > + unsigned int order; > + bool zero = gfp & __GFP_ZERO; > + int ret; > + > + ret = cc_shared_calc_layout(requested, &layout); > + if (ret) > + return ret; > + > + order = get_order(layout.shared_size); > + if (order > MAX_PAGE_ORDER) > + return -EINVAL; > + /* > + * State transitions require a linear-map address and may modify memory. > + * Allocate from low memory and defer requested zeroing until > afterwards. > + * Shared allocations have no migration support, so they must not > + * be allocated with __GFP_MOVABLE. > + */ > + gfp &= ~(__GFP_HIGHMEM | __GFP_ZERO | __GFP_MOVABLE); > + if (nid == NUMA_NO_NODE) > + page = alloc_pages(gfp, order); > + else > + page = alloc_pages_node(nid, gfp, order); > + if (!page) > + return -ENOMEM; > + > + ret = cc_make_shared(page_address(page), layout.shared_size); > + if (ret) { > + if (!cc_make_private(page_address(page), layout.shared_size)) > + __free_pages(page, order); > + else > + pr_warn_ratelimited("leaking %zu bytes with uncertain > shared state\n", > + layout.shared_size); > + return ret; > + } [Severity: High] Is it safe to call cc_make_private() across the entire layout.shared_size range when cc_make_shared() fails in __alloc_cc_shared_pages_node()? When cc_make_shared() fails via set_memory_decrypted(), the pages in the range may not have been transitioned to shared state at all, or only a prefix of the range may have been transitioned. For example, on x86 TDX in arch/x86/mm/pat/set_memory.c and arch/x86/coco/tdx/tdx.c: __set_memory_enc_pgtable() { ret = x86_platform.guest.enc_status_change_prepare(addr, numpages, enc); if (ret) goto vmm_fail; ret = __change_page_attr_set_clr(&cpa, 1); ... if (ret) return ret; ret = x86_platform.guest.enc_status_change_finish(addr, numpages, enc); if (ret) goto vmm_fail; } tdx_enc_status_change_finish() { if (!enc && !tdx_enc_status_changed(vaddr, numpages, enc)) return -EIO; if (enc) atomic_long_sub(numpages, &nr_shared); else atomic_long_add(numpages, &nr_shared); return 0; } If set_memory_decrypted() (enc = false) fails with -EBUSY on mem_enc_lock or with -ENOMEM in __change_page_attr_set_clr() before tdx_enc_status_change_finish() is reached, the pages remain private and accepted in the TDX module, and nr_shared is not incremented. Calling cc_make_private() (set_memory_encrypted(), enc = true) then invokes tdx_enc_status_change_prepare() -> tdx_accept_memory() -> try_accept_one() on already-accepted private memory, which fails and jumps to vmm_fail in __set_memory_enc_pgtable(), triggering a spurious WARN_ONCE and leaking memory that was never shared. Conversely, if tdx_enc_status_change_finish() fails during cc_make_shared() before atomic_long_add(numpages, &nr_shared), and the subsequent cc_make_private() call succeeds, tdx_enc_status_change_finish() will execute atomic_long_sub(numpages, &nr_shared), underflowing nr_shared and corrupting the accounting checked in tdx_kexec_finish(). Similarly, on arm64 pKVM in drivers/virt/coco/pkvm-guest/arm-pkvm-guest.c, pkvm_set_memory_decrypted() shares pages one by one and stops at the first failing page, leaving the remaining pages private. Calling cc_make_private() across the full range then invokes KVM_MEM_UNSHARE on private pages, which fails with -EPERM. Other callers in the kernel (such as dma_direct_alloc_pages(), atomic_pool_expand(), and swiotlb_init_late()) do not call set_memory_encrypted() when set_memory_decrypted() fails, and instead leak the pages directly because the transition state of the range is unknown. Should __alloc_cc_shared_pages_node() do the same here instead of calling cc_make_private()? -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=1
