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

[PATCH] 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 page state is unknown, so the
pages are still leaked rather than freed. No remap exists at that point
any more, so nothing else needs to be undone.

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
---
 kernel/dma/pool.c | 39 ++++++++++++++++++---------------------
 1 file changed, 18 insertions(+), 21 deletions(-)

diff --git a/kernel/dma/pool.c b/kernel/dma/pool.c
index 381b3cb28368..3c67728a2471 100644
--- a/kernel/dma/pool.c
+++ b/kernel/dma/pool.c
@@ -106,6 +106,17 @@ 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);
+		if (ret)
+			goto out;
+	}
+
 	arch_dma_prep_coherent(page, pool_size);
 
 #ifdef CONFIG_DMA_DIRECT_REMAP
@@ -117,41 +128,27 @@ 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;
+		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] 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 page state is unknown, so the
> pages are still leaked rather than freed. No remap exists at that point
> any more, so nothing else needs to be undone.
>

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
> ---
>  kernel/dma/pool.c | 39 ++++++++++++++++++---------------------
>  1 file changed, 18 insertions(+), 21 deletions(-)
>
> diff --git a/kernel/dma/pool.c b/kernel/dma/pool.c
> index 381b3cb28368..3c67728a2471 100644
> --- a/kernel/dma/pool.c
> +++ b/kernel/dma/pool.c
> @@ -106,6 +106,17 @@ 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);
> +		if (ret)
> +			goto out;
> +	}
> +
>  	arch_dma_prep_coherent(page, pool_size);
>  
>  #ifdef CONFIG_DMA_DIRECT_REMAP
> @@ -117,41 +128,27 @@ 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;
> +		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