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

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

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