In case memory needs to be remapped on systems with
force_dma_unencrypted(), where this memory is not allocated
from a restricted-dma pool, this was currently ignored, while only
setting the decrypted pgprot in the remapped alias.
The memory still needs to be decrypted in that case.
With memory decryption, don't allow highmem allocations, but that
shouldn't be a problem on such modern systems.
Reported-by: Catalin Marinas <catalin.marinas@arm.com>
Fixes: f3c962226dbe ("dma-direct: clean up the remapping checks in dma_direct_alloc")
Signed-off-by: Mostafa Saleh <smostafa@google.com>
---
kernel/dma/direct.c | 16 +++++++++++-----
1 file changed, 11 insertions(+), 5 deletions(-)
diff --git a/kernel/dma/direct.c b/kernel/dma/direct.c
index 1a402bb956d9..a4260689bcc8 100644
--- a/kernel/dma/direct.c
+++ b/kernel/dma/direct.c
@@ -203,6 +203,7 @@ static void *dma_direct_alloc_no_mapping(struct device *dev, size_t size,
void *dma_direct_alloc(struct device *dev, size_t size,
dma_addr_t *dma_handle, gfp_t gfp, unsigned long attrs)
{
+ bool allow_highmem = !force_dma_unencrypted(dev);
bool remap = false, set_uncached = false;
struct page *page;
void *ret;
@@ -251,7 +252,7 @@ void *dma_direct_alloc(struct device *dev, size_t size,
return dma_direct_alloc_from_pool(dev, size, dma_handle, gfp);
/* 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;
@@ -265,6 +266,9 @@ void *dma_direct_alloc(struct device *dev, size_t size,
set_uncached = false;
}
+ if (dma_set_decrypted(dev, page_address(page), size))
+ goto out_leak_pages;
+
if (remap) {
pgprot_t prot = dma_pgprot(dev, PAGE_KERNEL, attrs);
@@ -278,11 +282,9 @@ void *dma_direct_alloc(struct device *dev, size_t size,
ret = dma_common_contiguous_remap(page, size, prot,
__builtin_return_address(0));
if (!ret)
- goto out_free_pages;
+ goto out_encrypt_pages;
} else {
ret = page_address(page);
- if (dma_set_decrypted(dev, ret, size))
- goto out_leak_pages;
}
memset(ret, 0, size);
@@ -300,7 +302,6 @@ void *dma_direct_alloc(struct device *dev, size_t size,
out_encrypt_pages:
if (dma_set_encrypted(dev, page_address(page), size))
return NULL;
-out_free_pages:
__dma_direct_free_pages(dev, page, size);
return NULL;
out_leak_pages:
@@ -339,7 +340,12 @@ void dma_direct_free(struct device *dev, size_t size,
return;
if (is_vmalloc_addr(cpu_addr)) {
+ void *vaddr = page_address(dma_direct_to_page(dev, dma_addr));
+
vunmap(cpu_addr);
+
+ if (dma_set_encrypted(dev, vaddr, size))
+ return;
} else {
if (IS_ENABLED(CONFIG_ARCH_HAS_DMA_CLEAR_UNCACHED))
arch_dma_clear_uncached(cpu_addr, size);
--
2.53.0.1185.g05d4b7b318-googOn Mon, Mar 30, 2026 at 02:50:41PM +0000, Mostafa Saleh wrote:
> @@ -265,6 +266,9 @@ void *dma_direct_alloc(struct device *dev, size_t size,
> set_uncached = false;
> }
>
> + if (dma_set_decrypted(dev, page_address(page), size))
> + goto out_leak_pages;
> +
> if (remap) {
> pgprot_t prot = dma_pgprot(dev, PAGE_KERNEL, attrs);
>
> if (force_dma_unencrypted(dev))
> prot = pgprot_decrypted(prot);
It seems confusing, why do we unconditionally call something called
dma_set_decrypted() and then conditionally call pgprot_decrypted()?
So, I think the same remark, lets not sprinkle these tests all over
the place and risk them becoming inconsistent. It should be much more
direct, like:
page = __dma_direct_alloc_pages(dev, size, gfp & ~__GFP_ZERO,
allow_highmem, &flags);
if (!dev_can_dma_from_encrypted(dev) && !(flags & FLAG_DECRYPTED)) {
dma_set_decrypted(dev, page_address(page));
flags = FLAG_DECRYPTED;
}
if (flags & FLAG_DECRYPTED)
prot = pgprot_decrypted(prot);
And so on.
The one place we should see a force_dma_unencrypted() is directly
before setting the flag.
Jason
On Mon, Mar 30, 2026 at 12:19:00PM -0300, Jason Gunthorpe wrote:
> On Mon, Mar 30, 2026 at 02:50:41PM +0000, Mostafa Saleh wrote:
>
> > @@ -265,6 +266,9 @@ void *dma_direct_alloc(struct device *dev, size_t size,
> > set_uncached = false;
> > }
> >
> > + if (dma_set_decrypted(dev, page_address(page), size))
> > + goto out_leak_pages;
> > +
> > if (remap) {
> > pgprot_t prot = dma_pgprot(dev, PAGE_KERNEL, attrs);
> >
> > if (force_dma_unencrypted(dev))
> > prot = pgprot_decrypted(prot);
>
> It seems confusing, why do we unconditionally call something called
> dma_set_decrypted() and then conditionally call pgprot_decrypted()?
dma_set_decrypted() will call force_dma_unencrypted() so that check
is consistent.
>
> So, I think the same remark, lets not sprinkle these tests all over
> the place and risk them becoming inconsistent. It should be much more
> direct, like:
I agree we shouldn’t be sprinkling all these random calls all over the code,
that’s why I was trying to consolidate the logic in the next patch.
>
> page = __dma_direct_alloc_pages(dev, size, gfp & ~__GFP_ZERO,
> allow_highmem, &flags);
>
> if (!dev_can_dma_from_encrypted(dev) && !(flags & FLAG_DECRYPTED)) {
> dma_set_decrypted(dev, page_address(page));
> flags = FLAG_DECRYPTED;
> }
>
> if (flags & FLAG_DECRYPTED)
> prot = pgprot_decrypted(prot);
>
> And so on.
>
> The one place we should see a force_dma_unencrypted() is directly
> before setting the flag.
I will look more into this, but my main worry would be
phys_to_dma_direct() and it's callers as I am not sure if is possible to
preserve the alloction origin in all contexts.
Thanks,
Mostafa
>
> Jason
On Mon, Mar 30, 2026 at 08:49:31PM +0000, Mostafa Saleh wrote: > > The one place we should see a force_dma_unencrypted() is directly > > before setting the flag. > > I will look more into this, but my main worry would be > phys_to_dma_direct() and it's callers as I am not sure if is possible to > preserve the alloction origin in all contexts. I'd rather have a very small number of places that recover the phys & flag through some convoluted logic than have it sprinkled all over the place in ever layer.. The helpers were not helpful, they were much more confusing. Jason
© 2016 - 2026 Red Hat, Inc.