[PATCH v2] dma-pool: decrypt pages before creating the shared remap

[PATCH v2] dma-pool: decrypt pages before creating the shared remap

[Marek Szyprowski]

atomic_pool_expand() creates the pgprot_decrypted() remap of the pool
pages before calling set_memory_decrypted() on them. The error path
when gen_pool_add_virt() fails does the reverse: it re-encrypts the
pages and only then tears down the decrypted remap. In both cases there
is a window where a decrypted (shared) kernel mapping points at a page
that is still private, i.e. encrypted, to the guest.

dma_direct_alloc() gets this right: it converts the pages first, then
creates the remap, and on error removes the remap before re-encrypting.
Do the same in atomic_pool_expand(). The pages are now converted before
arch_dma_prep_coherent() and the remap, and the error path unwinds in
reverse order.

If set_memory_decrypted() fails, the encryption state of the pages is
unknown (the conversion may have partially succeeded), so they must not
be returned to the page allocator or CMA and are intentionally leaked,
as before. No remap exists at that point any more, so nothing else needs
to be undone. Add a comment explaining this.

Closes: https://sashiko.dev/#/bug/linux-9212d9b5-4a56-43e2-bc00-6e69b724078b
Fixes: 76a19940bd62 ("dma-direct: atomic allocations must come from atomic coherent pools")
Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
---
The patch was prepared with LLM assistance.

Best regards
Marek Szyprowski, PhD
Samsung R&D Institute Poland
---
Changelog:

v2:
- addressed more sashiko.dev pointed issues:
  https://sashiko.dev/#/patchset/20261109154403.384434-1-m.szyprowski%40samsung.com

v1: https://lore.kernel.org/all/20261109154403.384434-1-m.szyprowski@samsung.com/
---
 kernel/dma/pool.c | 48 +++++++++++++++++++++++++----------------------
 1 file changed, 26 insertions(+), 22 deletions(-)

diff --git a/kernel/dma/pool.c b/kernel/dma/pool.c
index 381b3cb28368..5e8cf12e8222 100644
--- a/kernel/dma/pool.c
+++ b/kernel/dma/pool.c
@@ -106,6 +106,22 @@ static int atomic_pool_expand(struct dma_gen_pool *dma_pool, size_t pool_size,
 	if (!page)
 		goto out;
 
+	/*
+	 * Memory in the atomic DMA pools must be unencrypted, the pools do not
+	 * shrink so no re-encryption occurs in dma_direct_free().
+	 */
+	if (dma_pool->cc_shared) {
+		ret = set_memory_decrypted((unsigned long)page_to_virt(page),
+					   1 << order);
+		/*
+		 * On failure the encryption state of the pages is unknown,
+		 * so they must not be returned to the page allocator or CMA.
+		 * Leak them intentionally.
+		 */
+		if (ret)
+			goto out;
+	}
+
 	arch_dma_prep_coherent(page, pool_size);
 
 #ifdef CONFIG_DMA_DIRECT_REMAP
@@ -116,42 +132,30 @@ static int atomic_pool_expand(struct dma_gen_pool *dma_pool, size_t pool_size,
 
 	addr = dma_common_contiguous_remap(page, pool_size, prot,
 			__builtin_return_address(0));
-	if (!addr)
-		goto free_page;
+	if (!addr) {
+		ret = -ENOMEM;
+		goto encrypt_pages;
+	}
 #else
 	addr = page_to_virt(page);
 #endif
-	/*
-	 * Memory in the atomic DMA pools must be unencrypted, the pools do not
-	 * shrink so no re-encryption occurs in dma_direct_free().
-	 */
-	if (dma_pool->cc_shared) {
-		ret = set_memory_decrypted((unsigned long)page_to_virt(page),
-					   1 << order);
-		if (ret) {
-			leak_pages = true;
-			goto remove_mapping;
-		}
-	}
-
 	ret = gen_pool_add_virt(dma_pool->pool, (unsigned long)addr,
 				page_to_phys(page), pool_size, NUMA_NO_NODE);
 	if (ret)
-		goto encrypt_mapping;
+		goto remove_mapping;
 
 	dma_atomic_pool_size_add(gfp, pool_size);
 	return 0;
 
-encrypt_mapping:
-	if (dma_pool->cc_shared &&
-	    set_memory_encrypted((unsigned long)page_to_virt(page), 1 << order))
-		leak_pages = true;
-
 remove_mapping:
 #ifdef CONFIG_DMA_DIRECT_REMAP
 	dma_common_free_remap(addr, pool_size);
-free_page:
+encrypt_pages:
 #endif
+	if (dma_pool->cc_shared &&
+	    set_memory_encrypted((unsigned long)page_to_virt(page), 1 << order))
+		leak_pages = true;
+
 	if (!leak_pages &&
 	    !dma_release_from_contiguous(NULL, page, 1 << order))
 		__free_pages(page, order);
-- 
2.43.0

Re: [PATCH v2] dma-pool: decrypt pages before creating the shared remap

[Aneesh Kumar K.V]

Marek Szyprowski <m.szyprowski@samsung.com> writes:

> atomic_pool_expand() creates the pgprot_decrypted() remap of the pool
> pages before calling set_memory_decrypted() on them. The error path
> when gen_pool_add_virt() fails does the reverse: it re-encrypts the
> pages and only then tears down the decrypted remap. In both cases there
> is a window where a decrypted (shared) kernel mapping points at a page
> that is still private, i.e. encrypted, to the guest.
>
> dma_direct_alloc() gets this right: it converts the pages first, then
> creates the remap, and on error removes the remap before re-encrypting.
> Do the same in atomic_pool_expand(). The pages are now converted before
> arch_dma_prep_coherent() and the remap, and the error path unwinds in
> reverse order.
>
> If set_memory_decrypted() fails, the encryption state of the pages is
> unknown (the conversion may have partially succeeded), so they must not
> be returned to the page allocator or CMA and are intentionally leaked,
> as before. No remap exists at that point any more, so nothing else needs
> to be undone. Add a comment explaining this.
>

Reviewed-by: Aneesh Kumar K.V (Arm) <aneesh.kumar@kernel.org>

> Closes: https://sashiko.dev/#/bug/linux-9212d9b5-4a56-43e2-bc00-6e69b724078b
> Fixes: 76a19940bd62 ("dma-direct: atomic allocations must come from atomic coherent pools")
> Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
> ---
> The patch was prepared with LLM assistance.
>
> Best regards
> Marek Szyprowski, PhD
> Samsung R&D Institute Poland
> ---
> Changelog:
>
> v2:
> - addressed more sashiko.dev pointed issues:
>   https://sashiko.dev/#/patchset/20261109154403.384434-1-m.szyprowski%40samsung.com
>
> v1: https://lore.kernel.org/all/20261109154403.384434-1-m.szyprowski@samsung.com/
> ---
>  kernel/dma/pool.c | 48 +++++++++++++++++++++++++----------------------
>  1 file changed, 26 insertions(+), 22 deletions(-)
>
> diff --git a/kernel/dma/pool.c b/kernel/dma/pool.c
> index 381b3cb28368..5e8cf12e8222 100644
> --- a/kernel/dma/pool.c
> +++ b/kernel/dma/pool.c
> @@ -106,6 +106,22 @@ static int atomic_pool_expand(struct dma_gen_pool *dma_pool, size_t pool_size,
>  	if (!page)
>  		goto out;
>  
> +	/*
> +	 * Memory in the atomic DMA pools must be unencrypted, the pools do not
> +	 * shrink so no re-encryption occurs in dma_direct_free().
> +	 */
> +	if (dma_pool->cc_shared) {
> +		ret = set_memory_decrypted((unsigned long)page_to_virt(page),
> +					   1 << order);
> +		/*
> +		 * On failure the encryption state of the pages is unknown,
> +		 * so they must not be returned to the page allocator or CMA.
> +		 * Leak them intentionally.
> +		 */
> +		if (ret)
> +			goto out;
> +	}
> +
>  	arch_dma_prep_coherent(page, pool_size);
>  
>  #ifdef CONFIG_DMA_DIRECT_REMAP
> @@ -116,42 +132,30 @@ static int atomic_pool_expand(struct dma_gen_pool *dma_pool, size_t pool_size,
>  
>  	addr = dma_common_contiguous_remap(page, pool_size, prot,
>  			__builtin_return_address(0));
> -	if (!addr)
> -		goto free_page;
> +	if (!addr) {
> +		ret = -ENOMEM;
> +		goto encrypt_pages;
> +	}
>  #else
>  	addr = page_to_virt(page);
>  #endif
> -	/*
> -	 * Memory in the atomic DMA pools must be unencrypted, the pools do not
> -	 * shrink so no re-encryption occurs in dma_direct_free().
> -	 */
> -	if (dma_pool->cc_shared) {
> -		ret = set_memory_decrypted((unsigned long)page_to_virt(page),
> -					   1 << order);
> -		if (ret) {
> -			leak_pages = true;
> -			goto remove_mapping;
> -		}
> -	}
> -
>  	ret = gen_pool_add_virt(dma_pool->pool, (unsigned long)addr,
>  				page_to_phys(page), pool_size, NUMA_NO_NODE);
>  	if (ret)
> -		goto encrypt_mapping;
> +		goto remove_mapping;
>  
>  	dma_atomic_pool_size_add(gfp, pool_size);
>  	return 0;
>  
> -encrypt_mapping:
> -	if (dma_pool->cc_shared &&
> -	    set_memory_encrypted((unsigned long)page_to_virt(page), 1 << order))
> -		leak_pages = true;
> -
>  remove_mapping:
>  #ifdef CONFIG_DMA_DIRECT_REMAP
>  	dma_common_free_remap(addr, pool_size);
> -free_page:
> +encrypt_pages:
>  #endif
> +	if (dma_pool->cc_shared &&
> +	    set_memory_encrypted((unsigned long)page_to_virt(page), 1 << order))
> +		leak_pages = true;
> +
>  	if (!leak_pages &&
>  	    !dma_release_from_contiguous(NULL, page, 1 << order))
>  		__free_pages(page, order);
> -- 
> 2.43.0