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

[Aneesh Kumar K.V <aneesh.kumar@kernel.org>]

sashiko-bot@kernel.org writes:

> 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()?
>

Yes, I agree that we should do this. Something like:

modified   kernel/dma/swiotlb.c
@@ -748,14 +748,9 @@ static struct page *alloc_dma_pages(gfp_t gfp, size_t bytes,
 
 	vaddr = phys_to_virt(paddr);
 	if (cc_shared && cc_make_shared(vaddr, bytes))
-		goto error;
+		/* leak pages on error */
+		return NULL;
 	return page;
-
-error:
-	/* Intentional leak if pages cannot be encrypted again. */
-	if (cc_shared && !cc_make_private(vaddr, bytes))
-		__free_pages(page, order);
-	return NULL;
 }
 
 /**
modified   mm/cc_shared.c
@@ -133,11 +133,16 @@ static int __alloc_cc_shared_pages_node(int nid, gfp_t gfp,
 
 	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);
+		/*
+		 * A failed transition may leave the range partially
+		 * shared, with no indication of which pages changed state.
+		 * Converting the whole range back to private is not safe
+		 * on architectures that cannot handle pages which are
+		 * already private. Leak the allocation rather than return
+		 * pages with uncertain state to the allocator.
+		 */
+		pr_warn_ratelimited("leaking %zu bytes with uncertain shared state\n",
+				    layout.shared_size);
 		return ret;
 	}