Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.32 as permitted sender) client-ip=23.128.96.32;
Date:   Sat, 28 Oct 2023 19:24:14 +0200
From:   Petr =?UTF-8?B?VGVzYcWZw61r?= <petr@tesarici.cz>
To:     Christoph Hellwig <hch@lst.de>
Cc:     Petr Tesarik <petrtesarik@huaweicloud.com>,
        Marek Szyprowski <m.szyprowski@samsung.com>,
        Robin Murphy <robin.murphy@arm.com>,
        Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        Petr Tesarik <petr.tesarik1@huawei-partners.com>,
        iommu@lists.linux.dev (open list:DMA MAPPING HELPERS),
        linux-kernel@vger.kernel.org (open list),
        Wangkefeng <wangkefeng.wang@huawei.com>,
        Roberto Sassu <roberto.sassu@huaweicloud.com>
Subject: Re: [PATCH] swiotlb: check dynamically allocated TLB address before
 decrypting
Message-ID: <20231028192414.637bf4fd@meshulam.tesarici.cz>
In-Reply-To: <20231026095123.222-1-petrtesarik@huaweicloud.com>
References: <20231026095123.222-1-petrtesarik@huaweicloud.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Content-Transfer-Encoding: 7bit
Precedence: bulk

Hi Christoph,

On Thu, 26 Oct 2023 11:51:23 +0200
Petr Tesarik <petrtesarik@huaweicloud.com> wrote:

> From: Petr Tesarik <petr.tesarik1@huawei-partners.com>
> 
> Do not decrypt a dynamically allocated TLB area until its physical address
> is known to be below the requested limit. Currently, pages are allocated
> and decrypted, but then they may be freed while still decrypted if
> swiotlb_alloc_tlb() determines that the physical address is too high.
> 
> Let the caller differentiate between unsuitable physical address (=> retry
> from a lower zone) and allocation failures (=> no point in retrying).
> 
> Fixes: 79636caad361 ("swiotlb: if swiotlb is full, fall back to a transient memory pool")
> Signed-off-by: Petr Tesarik <petr.tesarik1@huawei-partners.com>

This patch fixes a potential information leak from a CoCo VM, so I
would like to see it reviewed. Is this on your radar, or did it fall
through the cracks?

Petr T

> ---
>  kernel/dma/swiotlb.c | 20 +++++++++++---------
>  1 file changed, 11 insertions(+), 9 deletions(-)
> 
> diff --git a/kernel/dma/swiotlb.c b/kernel/dma/swiotlb.c
> index dff067bd56b1..d1118f6f61b8 100644
> --- a/kernel/dma/swiotlb.c
> +++ b/kernel/dma/swiotlb.c
> @@ -558,30 +558,36 @@ void __init swiotlb_exit(void)
>   * alloc_dma_pages() - allocate pages to be used for DMA
>   * @gfp:	GFP flags for the allocation.
>   * @bytes:	Size of the buffer.
> + * @phys_limit:	Maximum allowed physical address of the
> buffer. *
>   * Allocate pages from the buddy allocator. If successful, make the
> allocated
>   * pages decrypted that they can be used for DMA.
>   *
> - * Return: Decrypted pages, or %NULL on failure.
> + * Return: Decrypted pages, %NULL on allocation failure, or
> ERR_PTR(-EAGAIN)
> + * if the allocated physical address was above @phys_limit.
>   */
> -static struct page *alloc_dma_pages(gfp_t gfp, size_t bytes)
> +static struct page *alloc_dma_pages(gfp_t gfp, size_t bytes, u64
> phys_limit) {
>  	unsigned int order = get_order(bytes);
>  	struct page *page;
> +	phys_addr_t paddr;
>  	void *vaddr;
>  
>  	page = alloc_pages(gfp, order);
>  	if (!page)
>  		return NULL;
>  
> -	vaddr = page_address(page);
> +	paddr = page_to_phys(page);
> +	if (paddr + bytes - 1 > phys_limit)
> +		goto error;
> +	vaddr = phys_to_virt(paddr);
>  	if (set_memory_decrypted((unsigned long)vaddr,
> PFN_UP(bytes))) goto error;
>  	return page;
>  
>  error:
>  	__free_pages(page, order);
> -	return NULL;
> +	return ERR_PTR(-EAGAIN);
>  }
>  
>  /**
> @@ -618,11 +624,7 @@ static struct page *swiotlb_alloc_tlb(struct
> device *dev, size_t bytes, else if (phys_limit <= DMA_BIT_MASK(32))
>  		gfp |= __GFP_DMA32;
>  
> -	while ((page = alloc_dma_pages(gfp, bytes)) &&
> -	       page_to_phys(page) + bytes - 1 > phys_limit) {
> -		/* allocated, but too high */
> -		__free_pages(page, get_order(bytes));
> -
> +	while (IS_ERR(page = alloc_dma_pages(gfp, bytes,
> phys_limit))) { if (IS_ENABLED(CONFIG_ZONE_DMA32) &&
>  		    phys_limit < DMA_BIT_MASK(64) &&
>  		    !(gfp & (__GFP_DMA32 | __GFP_DMA)))