crypto/ccp: Fix locking on alloc failure handling

PATCH In review Alexey Kardashevskiy 4 participants 6 messages Jun 17, 2025 – Jul 7, 2025

Subsystems (3)

Crypto API AMD cryptographic coprocessor (CCP) driver AMD cryptographic coprocessor (CCP) driver - SEV support

lore

Share & tools

lore

Alexey Kardashevskiy

author

2025-06-17 09:44 UTC #0 lore raw

PATCH 1/1 crypto/ccp: Fix locking on alloc failure handling

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.

Fixes 24512afa4336 ("crypto: ccp: Handle the legacy TMR allocation when SNP is enabled")

Signed-off-by Alexey Kardashevskiy <[email protected]>

---
 drivers/crypto/ccp/sev-dev.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

drivers/crypto/ccp/sev-dev.c +4 -4

index 3451bada884e..16a11d5efe46 100644
@@ -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;

Signature · 1 line

2.49.0

Tom Lendacky

maintainer

2025-06-20 19:20 UTC #1 lore raw

Quoted 12 lines · Alexey Kardashevskiy

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")

Quoted 8 lines

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

Quoted 47 lines

Fixes 24512afa4336 ("crypto: ccp: Handle the legacy TMR allocation when SNP is enabled")

Signed-off-by Alexey Kardashevskiy <[email protected]>

---
 drivers/crypto/ccp/sev-dev.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

drivers/crypto/ccp/sev-dev.c +4 -4

index 3451bada884e..16a11d5efe46 100644
@@ -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;

Alexey Kardashevskiy

author

2025-06-24 01:41 UTC #2 lore raw

Quoted 18 lines · Tom Lendacky

Quoted 17 lines · Alexey Kardashevskiy
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.

Quoted 5 lines

Quoted 3 lines
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?

That would be another patch then, this one is a fix ;)

and I'd probably just ditch both snp_alloc_firmware_page() and sev_fw_alloc(), rename __snp_alloc_firmware_pages() to snp_alloc_firmware_page() and just use this one everywhere. Nobody needs page struct anyway, and the locking will be clear everywhere. Also do the same for snp_free_firmware_page().

It is just that snp_alloc_firmware_page() and snp_free_firmware_page() are EXPORT_SYMBOL_GPL,

Quoted 3 lines

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.

I'd rather just ditch sev_fw_alloc(), does not look very useful. Thanks,

Quoted 50 lines

Thanks,
Tom

Quoted 47 lines

Fixes 24512afa4336 ("crypto: ccp: Handle the legacy TMR allocation when SNP is enabled")

Signed-off-by Alexey Kardashevskiy <[email protected]>

---
  drivers/crypto/ccp/sev-dev.c | 8 ++++----
  1 file changed, 4 insertions(+), 4 deletions(-)

drivers/crypto/ccp/sev-dev.c +4 -4

index 3451bada884e..16a11d5efe46 100644
@@ -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;

Signature · 1 line

Alexey

Tom Lendacky

maintainer Reviewed-by

2025-06-24 15:21 UTC #3 lore raw

Quoted 35 lines · Alexey Kardashevskiy

Quoted 20 lines · Tom Lendacky
Quoted 19 lines · Alexey Kardashevskiy
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.
Quoted 5 lines
Quoted 3 lines
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?
That would be another patch then, this one is a fix ;)
and I'd probably just ditch both snp_alloc_firmware_page() and sev_fw_alloc(), rename __snp_alloc_firmware_pages() to snp_alloc_firmware_page() and just use this one everywhere. Nobody needs page struct anyway, and the locking will be clear everywhere. Also do the same for snp_free_firmware_page().

Quoted 8 lines

It is just that snp_alloc_firmware_page() and snp_free_firmware_page() are EXPORT_SYMBOL_GPL,
Quoted 3 lines
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.
I'd rather just ditch sev_fw_alloc(), does not look very useful. Thanks,

Ok, I'm good with this patch for the fix. Can you also provide a follow up patch(es) to address what you've discussed?

For the fix:

Reviewed-by Tom Lendacky <[email protected]>

Thanks,
Tom

Quoted 53 lines

Quoted 52 lines

Thanks,
Tom

Quoted 46 lines

Fixes 24512afa4336 ("crypto: ccp: Handle the legacy TMR allocation

when SNP is enabled")

Signed-off-by Alexey Kardashevskiy <[email protected]>

---
drivers/crypto/ccp/sev-dev.c | 8 ++++----

  1 file changed, 4 insertions(+), 4 deletions(-)

drivers/crypto/ccp/sev-dev.c +0 -0

index 3451bada884e..16a11d5efe46 100644
@@ -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;

drivers/crypto/ccp/sev-dev.c +3 -1

@@ -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;

Pratik R. Sampat

Reviewed-by on v1 1/1

2025-06-26 14:50 UTC #4 lore raw

In reply to Alexey Kardashevskiy

Hi Alexey,

Quoted 17 lines · Alexey Kardashevskiy

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.

Quoted 6 lines

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.
Fixes 24512afa4336 ("crypto: ccp: Handle the legacy TMR allocation when SNP is enabled")
Signed-off-by Alexey Kardashevskiy <[email protected]>

Thank you for fixing this.
I was facing a locking issue and was writing a similar patch when I discovered this!

Reviewed-by Pratik R. Sampat <[email protected]>

Quoted 44 lines

---
 drivers/crypto/ccp/sev-dev.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

drivers/crypto/ccp/sev-dev.c +4 -4

index 3451bada884e..16a11d5efe46 100644
@@ -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;

Herbert Xu

maintainer

2025-07-07 03:29 UTC #5 lore raw

In reply to Alexey Kardashevskiy

Quoted 21 lines · Alexey Kardashevskiy

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.

Quoted 5 lines

Fixes 24512afa4336 ("crypto: ccp: Handle the legacy TMR allocation when SNP is enabled")
Signed-off-by Alexey Kardashevskiy <[email protected]>
---
 drivers/crypto/ccp/sev-dev.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

Patch applied. Thanks.

Signature · 3 lines

Email: Herbert Xu <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt