Re: [PATCH v9 01/14] mm: Add an allocator for CoCo shared memory

[sashiko-bot@kernel.org]

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

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/20261104164555.3555725-1-aneesh.kumar@kernel.org?part=1