linux-kernel - Re: [PATCH] crypto/ccp: Fix locking on alloc failure handling

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <a0ce9850-cde4-4e17-997b-ad06a76a23d6@amd.com>
Date: Fri, 20 Jun 2025 14:20:26 -0500
From: Tom Lendacky <thomas.lendacky@....com>
To: Alexey Kardashevskiy <aik@....com>, linux-crypto@...r.kernel.org
Cc: linux-kernel@...r.kernel.org, Ashish Kalra <ashish.kalra@....com>,
 John Allen <john.allen@....com>, Herbert Xu <herbert@...dor.apana.org.au>,
 "Borislav Petkov (AMD)" <bp@...en8.de>, Michael Roth <michael.roth@....com>
Subject: Re: [PATCH] crypto/ccp: Fix locking on alloc failure handling

On 6/17/25 04:43, Alexey Kardashevskiy wrote:
> The __snp_alloc_firmware_pages() helper allocates pages in the firmware
> state (alloc + rmpupdate). In case of failed rmpupdate, it tries
> reclaiming pages with already changed state. This requires calling
> the PSP firmware and since there is sev_cmd_mutex to guard such calls,
> the helper takes a "locked" parameter so specify if the lock needs to
> be held.
> 
> Most calls happen from snp_alloc_firmware_page() which executes without
> the lock. However
> 
> commit 24512afa4336 ("crypto: ccp: Handle the legacy TMR allocation when SNP is enabled")
> 
> switched sev_fw_alloc() from alloc_pages() (which does not call the PSP) to
> __snp_alloc_firmware_pages() (which does) but did not account for the fact
> that sev_fw_alloc() is called from __sev_platform_init_locked()
> (via __sev_platform_init_handle_tmr()) and executes with the lock held.
> 
> Add a "locked" parameter to __snp_alloc_firmware_pages().
> Make sev_fw_alloc() use the new parameter to prevent potential deadlock in
> rmp_mark_pages_firmware() if rmpupdate() failed.

Would it make sense to add the locked parameter to sev_fw_alloc(), too?
Right now there is only one caller of sev_fw_alloc(), but in the future,
if some other path should call sev_fw_alloc() and that path doesn't have
the lock, then we'll miss taking it.

Thanks,
Tom

> 
> Fixes: 24512afa4336 ("crypto: ccp: Handle the legacy TMR allocation when SNP is enabled")
> Signed-off-by: Alexey Kardashevskiy <aik@....com>
> ---
>  drivers/crypto/ccp/sev-dev.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/crypto/ccp/sev-dev.c b/drivers/crypto/ccp/sev-dev.c
> index 3451bada884e..16a11d5efe46 100644
> --- a/drivers/crypto/ccp/sev-dev.c
> +++ b/drivers/crypto/ccp/sev-dev.c
> @@ -434,7 +434,7 @@ static int rmp_mark_pages_firmware(unsigned long paddr, unsigned int npages, boo
>  	return rc;
>  }
>  
> -static struct page *__snp_alloc_firmware_pages(gfp_t gfp_mask, int order)
> +static struct page *__snp_alloc_firmware_pages(gfp_t gfp_mask, int order, bool locked)
>  {
>  	unsigned long npages = 1ul << order, paddr;
>  	struct sev_device *sev;
> @@ -453,7 +453,7 @@ static struct page *__snp_alloc_firmware_pages(gfp_t gfp_mask, int order)
>  		return page;
>  
>  	paddr = __pa((unsigned long)page_address(page));
> -	if (rmp_mark_pages_firmware(paddr, npages, false))
> +	if (rmp_mark_pages_firmware(paddr, npages, locked))
>  		return NULL;
>  
>  	return page;
> @@ -463,7 +463,7 @@ void *snp_alloc_firmware_page(gfp_t gfp_mask)
>  {
>  	struct page *page;
>  
> -	page = __snp_alloc_firmware_pages(gfp_mask, 0);
> +	page = __snp_alloc_firmware_pages(gfp_mask, 0, false);
>  
>  	return page ? page_address(page) : NULL;
>  }
> @@ -498,7 +498,7 @@ static void *sev_fw_alloc(unsigned long len)
>  {
>  	struct page *page;
>  
> -	page = __snp_alloc_firmware_pages(GFP_KERNEL, get_order(len));
> +	page = __snp_alloc_firmware_pages(GFP_KERNEL, get_order(len), true);
>  	if (!page)
>  		return NULL;
>