Re: [PATCH] swiotlb: check set_memory_decrypted()'s return value

Robin Murphy
2022-11-21T20:48:58Z

On 2022-11-21 19:48, Dexuan Cui wrote:
> swiotlb_update_mem_attributes() is called from a system where decrypted
> swiotlb bounce buffers are required. Panic the system if
> set_memory_decrypted() fails.
>
> When rmem_swiotlb_device_init() -> set_memory_decrypted(), let's try
> to handle it gracefully.
>
> Not sure how to handle the failure in swiotlb_init_late(). Add a WARN().
>
> Signed-off-by: Dexuan Cui <decui@microsoft.com>
> ---
> kernel/dma/swiotlb.c | 42 +++++++++++++++++++++++++++++-------------
> 1 file changed, 29 insertions(+), 13 deletions(-)
>
> diff --git a/kernel/dma/swiotlb.c b/kernel/dma/swiotlb.c
> index 339a990554e7..8e26c09c625c 100644
> --- a/kernel/dma/swiotlb.c
> +++ b/kernel/dma/swiotlb.c
> @@ -248,12 +248,16 @@ void __init swiotlb_update_mem_attributes(void)
> struct io_tlb_mem *mem = &io_tlb_default_mem;
> void *vaddr;
> unsigned long bytes;
> + int rc;
>
> if (!mem->nslabs || mem->late_alloc)
> return;
> vaddr = phys_to_virt(mem->start);
> bytes = PAGE_ALIGN(mem->nslabs << IO_TLB_SHIFT);
> - set_memory_decrypted((unsigned long)vaddr, bytes >> PAGE_SHIFT);
> +
> + rc = set_memory_decrypted((unsigned long)vaddr, bytes >> PAGE_SHIFT);
> + if (rc)
> + panic("Failed to decrypt swiotlb bounce buffers (%d)\n", rc);

Aww, I just cleaned up all the panics! AFAICS we could warn and set
mem->nslabs to 0 here to semi-gracefully disable SWIOTLB (presumably
decryption failure is sufficiently unexpected that "leaking" the SWIOTLB
memory is the least of the system's problems). Or indeed just warn and
do nothing as in the swiotlb_init_late() case below - what's with the
inconsistency? In either path we have the same expectation that
decryption succeeds (or does nothing, successfully), so failure is no
more or less fatal to later SWIOTLB operation depending on when the
architecture chose to set it up.

> mem->vaddr = swiotlb_mem_remap(mem, bytes);
> if (!mem->vaddr)
> @@ -447,8 +451,10 @@ int swiotlb_init_late(size_t size, gfp_t gfp_mask,
> if (!mem->slots)
> goto error_slots;
>
> - set_memory_decrypted((unsigned long)vstart,
> - (nslabs << IO_TLB_SHIFT) >> PAGE_SHIFT);
> + rc = set_memory_decrypted((unsigned long)vstart,
> + (nslabs << IO_TLB_SHIFT) >> PAGE_SHIFT);
> + WARN(rc, "Failed to decrypt swiotlb bounce buffers (%d)\n", rc);
> +
> swiotlb_init_io_tlb_mem(mem, virt_to_phys(vstart), nslabs, 0, true,
> default_nareas);
>
> @@ -986,6 +992,7 @@ static int rmem_swiotlb_device_init(struct reserved_mem *rmem,
>
> /* Set Per-device io tlb area to one */
> unsigned int nareas = 1;
> + int rc = -ENOMEM;
>
> /*
> * Since multiple devices can share the same pool, the private data,
> @@ -998,21 +1005,22 @@ static int rmem_swiotlb_device_init(struct reserved_mem *rmem,
> return -ENOMEM;
>
> mem->slots = kcalloc(nslabs, sizeof(*mem->slots), GFP_KERNEL);
> - if (!mem->slots) {
> - kfree(mem);
> - return -ENOMEM;
> - }
> + if (!mem->slots)
> + goto free_mem;
>
> mem->areas = kcalloc(nareas, sizeof(*mem->areas),
> GFP_KERNEL);
> - if (!mem->areas) {
> - kfree(mem->slots);
> - kfree(mem);
> - return -ENOMEM;
> + if (!mem->areas)
> + goto free_slots;
> +
> + rc = set_memory_decrypted(
> + (unsigned long)phys_to_virt(rmem->base),
> + rmem->size >> PAGE_SHIFT);
> + if (rc) {
> + pr_err("Failed to decrypt rmem buffer (%d)\n", rc);
> + goto free_areas;

So in 3 init paths we have 3 different outcomes from the same thing :(

1: make the whole system unusable
2: continue with possible data corruption (or at least weird DMA errors)
if devices still see encrypted memory contents
3: cleanly disable SWIOTLB, thus also all subsequent attempts to use it

(OK, for the rmem case this isn't actually 3 since falling back to
io_tlb_default_mem might work out more like 2, but hopefully you get my
point)

Thanks,
Robin.

> }
>
> - set_memory_decrypted((unsigned long)phys_to_virt(rmem->base),
> - rmem->size >> PAGE_SHIFT);
> swiotlb_init_io_tlb_mem(mem, rmem->base, nslabs, SWIOTLB_FORCE,
> false, nareas);
> mem->for_alloc = true;
> @@ -1025,6 +1033,14 @@ static int rmem_swiotlb_device_init(struct reserved_mem *rmem,
> dev->dma_io_tlb_mem = mem;
>
> return 0;
> +
> +free_areas:
> + kfree(mem->areas);
> +free_slots:
> + kfree(mem->slots);
> +free_mem:
> + kfree(mem);
> + return rc;
> }
>
> static void rmem_swiotlb_device_release(struct reserved_mem *rmem,