On 2023-10-23 17:46, Edgecombe, Rick P wrote:
On Wed, 2023-10-18 at 18:42 +0100, Robin Murphy wrote:
On 2023-10-17 21:25, Rick Edgecombe wrote:
On TDX it is possible for the untrusted host to cause
set_memory_encrypted() or set_memory_decrypted() to fail such that
an
error is returned and the resulting memory is shared. Callers need
to take
care to handle these errors to avoid returning decrypted (shared)
memory to
the page allocator, which could lead to functional or security
issues.
DMA could free decrypted/shared pages if set_memory_decrypted()
fails.
Use the recently added free_decrypted_pages() to avoid this.
Several paths also result in proper encrypted pages being freed
through
the same freeing function. Rely on free_decrypted_pages() to not
leak the
memory in these cases.
If something's needed in the fallback path here, what about the
cma_release() paths?
You mean inside cma_release(). If so, unfortunately I think it won't
fit great because there are callers that are never dealing with shared
memory (huge tlb). The reset-to-private operation does extra work that
would be nice to avoid when possible.
The cases I thought exhibited the issue were the two calls sites of
dma_set_decrypted(). Playing around with it, I was thinking it might be
easier to just fix those to open code leaking the pages on
dma_set_decrypted() error. In which case it won't have the re-encrypt
problem.
It make's it less fool proof, but more efficient. And
free_decrypted_pages() doesn't fit great anyway, as pointed out by
Christoph.
My point is that in dma_direct_alloc(), we get some memory either
straight from the page allocator *or* from a CMA area, then call
set_memory_decrypted() on it. If the problem is that
set_memory_decrypted() can fail and require cleanup, then logically if
that cleanup is necessary for the dma_free_contiguous()->__free_pages()
call, then surely it must also be necessary for the
dma_free_contiguous()->cma_release()->free_contig_range()->__free_page()
calls.
Thanks,
Robin.
