[RFC PATCH 1/2] dma-mapping: Avoid double decrypting with DMA_RESTRICTED_POOL

Mostafa Saleh posted 2 patches 1 month ago
[RFC PATCH 1/2] dma-mapping: Avoid double decrypting with DMA_RESTRICTED_POOL
Posted by Mostafa Saleh 1 month ago
In case a device have a restricted DMA pool, it will be decrypted.
However, in the path of dma_direct_alloc() memory can be allocated
from this pool using, __dma_direct_alloc_pages() =>
dma_direct_alloc_swiotlb()

After that from the same function, it will attempt to decrypt it
using dma_set_decrypted() if force_dma_unencrypted().

Which results in the memory being decrypted twice.

It's not clear how the does realm world/hypervisors deal with that,
for example:
- Clear a bit in the page table and call realm IPA_STATE_SET
- TDX: Seems to issue a hypercall also.
- pKVM: Which doesn't implement force_dma_unencrypted() at the moment,
  uses a share hypercall which is definitely not Idempotent.

This patch will only encrypt/decrypt memory that are not allocated
form the restricted dma pools.

Signed-off-by: Mostafa Saleh <smostafa@google.com>
---
 kernel/dma/direct.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/kernel/dma/direct.c b/kernel/dma/direct.c
index 8f43a930716d..27d804f0473f 100644
--- a/kernel/dma/direct.c
+++ b/kernel/dma/direct.c
@@ -79,7 +79,7 @@ bool dma_coherent_ok(struct device *dev, phys_addr_t phys, size_t size)
 
 static int dma_set_decrypted(struct device *dev, void *vaddr, size_t size)
 {
-	if (!force_dma_unencrypted(dev))
+	if (!force_dma_unencrypted(dev) || is_swiotlb_for_alloc(dev))
 		return 0;
 	return set_memory_decrypted((unsigned long)vaddr, PFN_UP(size));
 }
@@ -88,7 +88,7 @@ static int dma_set_encrypted(struct device *dev, void *vaddr, size_t size)
 {
 	int ret;
 
-	if (!force_dma_unencrypted(dev))
+	if (!force_dma_unencrypted(dev) || is_swiotlb_for_alloc(dev))
 		return 0;
 	ret = set_memory_encrypted((unsigned long)vaddr, PFN_UP(size));
 	if (ret)
-- 
2.53.0.473.g4a7958ca14-goog
Re: [RFC PATCH 1/2] dma-mapping: Avoid double decrypting with DMA_RESTRICTED_POOL
Posted by Catalin Marinas 1 month ago
On Thu, Mar 05, 2026 at 05:03:34PM +0000, Mostafa Saleh wrote:
> In case a device have a restricted DMA pool, it will be decrypted.
> However, in the path of dma_direct_alloc() memory can be allocated
> from this pool using, __dma_direct_alloc_pages() =>
> dma_direct_alloc_swiotlb()
> 
> After that from the same function, it will attempt to decrypt it
> using dma_set_decrypted() if force_dma_unencrypted().
> 
> Which results in the memory being decrypted twice.
> 
> It's not clear how the does realm world/hypervisors deal with that,
> for example:
> - Clear a bit in the page table and call realm IPA_STATE_SET
> - TDX: Seems to issue a hypercall also.
> - pKVM: Which doesn't implement force_dma_unencrypted() at the moment,
>   uses a share hypercall which is definitely not Idempotent.
> 
> This patch will only encrypt/decrypt memory that are not allocated
> form the restricted dma pools.
> 
> Signed-off-by: Mostafa Saleh <smostafa@google.com>
> ---
>  kernel/dma/direct.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/kernel/dma/direct.c b/kernel/dma/direct.c
> index 8f43a930716d..27d804f0473f 100644
> --- a/kernel/dma/direct.c
> +++ b/kernel/dma/direct.c
> @@ -79,7 +79,7 @@ bool dma_coherent_ok(struct device *dev, phys_addr_t phys, size_t size)
>  
>  static int dma_set_decrypted(struct device *dev, void *vaddr, size_t size)
>  {
> -	if (!force_dma_unencrypted(dev))
> +	if (!force_dma_unencrypted(dev) || is_swiotlb_for_alloc(dev))
>  		return 0;
>  	return set_memory_decrypted((unsigned long)vaddr, PFN_UP(size));
>  }
> @@ -88,7 +88,7 @@ static int dma_set_encrypted(struct device *dev, void *vaddr, size_t size)
>  {
>  	int ret;
>  
> -	if (!force_dma_unencrypted(dev))
> +	if (!force_dma_unencrypted(dev) || is_swiotlb_for_alloc(dev))
>  		return 0;
>  	ret = set_memory_encrypted((unsigned long)vaddr, PFN_UP(size));
>  	if (ret)

I think that's functionally correct for rmem buffers. Normally I'd have
moved the is_swiotlb_for_alloc() condition in the caller but even
dma_direct_alloc() doesn't know where the buffer came from, it's hidden
in __dma_direct_alloc_pages().

However, it's unclear to me whether we can get encrypted pages when
is_swiotlb_for_alloc() == false, remap == true and
force_dma_unencrypted() == true in dma_direct_alloc().
dma_set_decrypted() is only called on the !remap path.

-- 
Catalin
Re: [RFC PATCH 1/2] dma-mapping: Avoid double decrypting with DMA_RESTRICTED_POOL
Posted by Catalin Marinas 1 month ago
On Tue, Mar 10, 2026 at 01:36:08PM +0000, Catalin Marinas wrote:
> However, it's unclear to me whether we can get encrypted pages when
> is_swiotlb_for_alloc() == false, remap == true and
> force_dma_unencrypted() == true in dma_direct_alloc().
> dma_set_decrypted() is only called on the !remap path.

Ah, I can see Anneesh trying to address this here:

https://lore.kernel.org/r/yq5abjjl4o0j.fsf@kernel.org

-- 
Catalin
Re: [RFC PATCH 1/2] dma-mapping: Avoid double decrypting with DMA_RESTRICTED_POOL
Posted by Mostafa Saleh 4 weeks, 1 day ago
On Tue, Mar 10, 2026 at 01:55:52PM +0000, Catalin Marinas wrote:
> On Tue, Mar 10, 2026 at 01:36:08PM +0000, Catalin Marinas wrote:
> > However, it's unclear to me whether we can get encrypted pages when
> > is_swiotlb_for_alloc() == false, remap == true and
> > force_dma_unencrypted() == true in dma_direct_alloc().
> > dma_set_decrypted() is only called on the !remap path.
> 
> Ah, I can see Anneesh trying to address this here:
> 
> https://lore.kernel.org/r/yq5abjjl4o0j.fsf@kernel.org

I see, thanks for pointing that out, the case Aneesh is fixing is the
missing decryption in the remap case. However, it’s not clear to me
how we can get there for CCA, I left a comment on his patch.

I can inline the is_swiotlb_for_alloc() checks outside, but I believe
adding this in the lowest level is better as indeed the memory is
decrypted and we don’t have to open code the check in other places are
dma_direct_alloc_pages()

Thanks,
Mostafa


> 
> -- 
> Catalin
Re: [RFC PATCH 1/2] dma-mapping: Avoid double decrypting with DMA_RESTRICTED_POOL
Posted by Aneesh Kumar K.V 3 weeks, 6 days ago
Mostafa Saleh <smostafa@google.com> writes:

> On Tue, Mar 10, 2026 at 01:55:52PM +0000, Catalin Marinas wrote:
>> On Tue, Mar 10, 2026 at 01:36:08PM +0000, Catalin Marinas wrote:
>> > However, it's unclear to me whether we can get encrypted pages when
>> > is_swiotlb_for_alloc() == false, remap == true and
>> > force_dma_unencrypted() == true in dma_direct_alloc().
>> > dma_set_decrypted() is only called on the !remap path.
>> 
>> Ah, I can see Anneesh trying to address this here:
>> 
>> https://lore.kernel.org/r/yq5abjjl4o0j.fsf@kernel.org
>
> I see, thanks for pointing that out, the case Aneesh is fixing is the
> missing decryption in the remap case. However, it’s not clear to me
> how we can get there for CCA, I left a comment on his patch.
>
> I can inline the is_swiotlb_for_alloc() checks outside, but I believe
> adding this in the lowest level is better as indeed the memory is
> decrypted and we don’t have to open code the check in other places are
> dma_direct_alloc_pages()
>

There are a few related changes that I have posted. However, I am
wondering whether it would be simpler to treat the swiotlb pool as
always decrypted. In that case, even when allocating from swiotlb we
would not need to toggle between decrypt/encrypt.

Another reason to treat swiotlb as special is the alignment requirement
when toggling between decrypted and encrypted states.

The patch implementing this approach is here 
https://lore.kernel.org/all/20260309102625.2315725-2-aneesh.kumar@kernel.org

With respect to remapping, there are two conditions that can currently
trigger a remap: when the device is non-coherent, or when we receive a
HighMem allocation. Neither of these conditions applies to CCA. We could
potentially enforce the HighMem case by using the following hunk in the
patch:


+
+	if (force_dma_unencrypted(dev))
+		/*
+		 * Unencrypted/shared DMA requires a linear-mapped buffer
+		 * address to look up the PFN and set architecture-required PFN
+		 * attributes. This is not possible with HighMem. Avoid HighMem
+		 * allocation.
+		 */
+		allow_highmem = false;
+
	/* we always manually zero the memory once we are done */
-	page = __dma_direct_alloc_pages(dev, size, gfp & ~__GFP_ZERO, true);
+	page = __dma_direct_alloc_pages(dev, size, gfp & ~__GFP_ZERO, allow_highmem);
	if (!page)
		return NULL;

https://lore.kernel.org/all/20260102155037.2551524-1-aneesh.kumar@kernel.org

I haven't got much feedback on that patch yet.

-aneesh