[PATCH 1/2] crypto: ccp - Fix a case where SNP_SHUTDOWN is missed

[PATCH 1/2] crypto: ccp - Fix a case where SNP_SHUTDOWN is missed

[Tycho Andersen]

From: Tom Lendacky <thomas.lendacky@amd.com>

If page reclaim fails in sev_ioctl_do_snp_platform_status() and SNP was
moved from UNINIT to INIT for the function, SNP is not moved back to
UNINIT state. Additionally, SNP is not required to be initialized in order
to execute the SNP_PLATFORM_STATUS command, so don't attempt to move to
INIT state and let SNP_PLATFORM_STATUS report the status as is.

Fixes: ceac7fb89e8d ("crypto: ccp - Ensure implicit SEV/SNP init and shutdown in ioctls")
Signed-off-by: Tom Lendacky <thomas.lendacky@amd.com>
Reviewed-by: Tycho Andersen (AMD) <tycho@kernel.org>
Reviewed-by: Alexey Kardashevskiy <aik@amd.com>
Signed-off-by: Tycho Andersen (AMD) <tycho@kernel.org>
---
 drivers/crypto/ccp/sev-dev.c | 46 ++++++++++++++++++------------------
 1 file changed, 23 insertions(+), 23 deletions(-)

diff --git a/drivers/crypto/ccp/sev-dev.c b/drivers/crypto/ccp/sev-dev.c
index 956ea609d0cc..6e6011e363e3 100644
--- a/drivers/crypto/ccp/sev-dev.c
+++ b/drivers/crypto/ccp/sev-dev.c
@@ -2378,11 +2378,10 @@ static int sev_ioctl_do_pdh_export(struct sev_issue_cmd *argp, bool writable)
 static int sev_ioctl_do_snp_platform_status(struct sev_issue_cmd *argp)
 {
 	struct sev_device *sev = psp_master->sev_data;
-	bool shutdown_required = false;
 	struct sev_data_snp_addr buf;
 	struct page *status_page;
-	int ret, error;
 	void *data;
+	int ret;
 
 	if (!argp->data)
 		return -EINVAL;
@@ -2393,31 +2392,35 @@ static int sev_ioctl_do_snp_platform_status(struct sev_issue_cmd *argp)
 
 	data = page_address(status_page);
 
-	if (!sev->snp_initialized) {
-		ret = snp_move_to_init_state(argp, &shutdown_required);
-		if (ret)
-			goto cleanup;
-	}
-
 	/*
-	 * Firmware expects status page to be in firmware-owned state, otherwise
-	 * it will report firmware error code INVALID_PAGE_STATE (0x1A).
+	 * SNP_PLATFORM_STATUS can be executed in any SNP state. But if executed
+	 * when SNP has been initialized, the status page must be firmware-owned.
 	 */
-	if (rmp_mark_pages_firmware(__pa(data), 1, true)) {
-		ret = -EFAULT;
-		goto cleanup;
+	if (sev->snp_initialized) {
+		/*
+		 * Firmware expects the status page to be in Firmware state,
+		 * otherwise it will report an error INVALID_PAGE_STATE.
+		 */
+		if (rmp_mark_pages_firmware(__pa(data), 1, true)) {
+			ret = -EFAULT;
+			goto cleanup;
+		}
 	}
 
 	buf.address = __psp_pa(data);
 	ret = __sev_do_cmd_locked(SEV_CMD_SNP_PLATFORM_STATUS, &buf, &argp->error);
 
-	/*
-	 * Status page will be transitioned to Reclaim state upon success, or
-	 * left in Firmware state in failure. Use snp_reclaim_pages() to
-	 * transition either case back to Hypervisor-owned state.
-	 */
-	if (snp_reclaim_pages(__pa(data), 1, true))
-		return -EFAULT;
+	if (sev->snp_initialized) {
+		/*
+		 * The status page will be in Reclaim state on success, or left
+		 * in Firmware state on failure. Use snp_reclaim_pages() to
+		 * transition either case back to Hypervisor-owned state.
+		 */
+		if (snp_reclaim_pages(__pa(data), 1, true)) {
+			snp_leak_pages(__page_to_pfn(status_page), 1);
+			return -EFAULT;
+		}
+	}
 
 	if (ret)
 		goto cleanup;
@@ -2427,9 +2430,6 @@ static int sev_ioctl_do_snp_platform_status(struct sev_issue_cmd *argp)
 		ret = -EFAULT;
 
 cleanup:
-	if (shutdown_required)
-		__sev_snp_shutdown_locked(&error, false);
-
 	__free_pages(status_page, 0);
 	return ret;
 }

base-commit: 3609fa95fb0f2c1b099e69e56634edb8fc03f87c
-- 
2.52.0

[PATCH 2/2] crypto/ccp: narrow scope of snp_range_list

[Tycho Andersen]

From: "Tycho Andersen (AMD)" <tycho@kernel.org>

snp_range_list is only used in __sev_snp_init_locked() in the SNP_INIT_EX
case, move the declaration there and add a __free() cleanup helper for it
instead of waiting until shutdown.

Fixes: 1ca5614b84ee ("crypto: ccp: Add support to initialize the AMD-SP for SEV-SNP")
Reviewed-by: Alexey Kardashevskiy <aik@amd.com>
Signed-off-by: Tycho Andersen (AMD) <tycho@kernel.org>
---
 drivers/crypto/ccp/sev-dev.c | 13 +------------
 1 file changed, 1 insertion(+), 12 deletions(-)

diff --git a/drivers/crypto/ccp/sev-dev.c b/drivers/crypto/ccp/sev-dev.c
index 6e6011e363e3..1cdadddb744e 100644
--- a/drivers/crypto/ccp/sev-dev.c
+++ b/drivers/crypto/ccp/sev-dev.c
@@ -127,13 +127,6 @@ static size_t sev_es_tmr_size = SEV_TMR_SIZE;
 #define NV_LENGTH (32 * 1024)
 static void *sev_init_ex_buffer;
 
-/*
- * SEV_DATA_RANGE_LIST:
- *   Array containing range of pages that firmware transitions to HV-fixed
- *   page state.
- */
-static struct sev_data_range_list *snp_range_list;
-
 static void __sev_firmware_shutdown(struct sev_device *sev, bool panic);
 
 static int snp_shutdown_on_panic(struct notifier_block *nb,
@@ -1361,6 +1354,7 @@ static int snp_filter_reserved_mem_regions(struct resource *rs, void *arg)
 
 static int __sev_snp_init_locked(int *error, unsigned int max_snp_asid)
 {
+	struct sev_data_range_list *snp_range_list __free(kfree) = NULL;
 	struct psp_device *psp = psp_master;
 	struct sev_data_snp_init_ex data;
 	struct sev_device *sev;
@@ -2780,11 +2774,6 @@ static void __sev_firmware_shutdown(struct sev_device *sev, bool panic)
 		sev_init_ex_buffer = NULL;
 	}
 
-	if (snp_range_list) {
-		kfree(snp_range_list);
-		snp_range_list = NULL;
-	}
-
 	__sev_snp_shutdown_locked(&error, panic);
 }
 
-- 
2.52.0

Re: [PATCH 2/2] crypto/ccp: narrow scope of snp_range_list

[Tom Lendacky]

On 1/5/26 11:22, Tycho Andersen wrote:
> From: "Tycho Andersen (AMD)" <tycho@kernel.org>
> 
> snp_range_list is only used in __sev_snp_init_locked() in the SNP_INIT_EX
> case, move the declaration there and add a __free() cleanup helper for it
> instead of waiting until shutdown.
> 
> Fixes: 1ca5614b84ee ("crypto: ccp: Add support to initialize the AMD-SP for SEV-SNP")
> Reviewed-by: Alexey Kardashevskiy <aik@amd.com>
> Signed-off-by: Tycho Andersen (AMD) <tycho@kernel.org>

Reviewed-by: Tom Lendacky <thomas.lendacky@amd.com>

> ---
>  drivers/crypto/ccp/sev-dev.c | 13 +------------
>  1 file changed, 1 insertion(+), 12 deletions(-)
> 
> diff --git a/drivers/crypto/ccp/sev-dev.c b/drivers/crypto/ccp/sev-dev.c
> index 6e6011e363e3..1cdadddb744e 100644
> --- a/drivers/crypto/ccp/sev-dev.c
> +++ b/drivers/crypto/ccp/sev-dev.c
> @@ -127,13 +127,6 @@ static size_t sev_es_tmr_size = SEV_TMR_SIZE;
>  #define NV_LENGTH (32 * 1024)
>  static void *sev_init_ex_buffer;
>  
> -/*
> - * SEV_DATA_RANGE_LIST:
> - *   Array containing range of pages that firmware transitions to HV-fixed
> - *   page state.
> - */
> -static struct sev_data_range_list *snp_range_list;
> -
>  static void __sev_firmware_shutdown(struct sev_device *sev, bool panic);
>  
>  static int snp_shutdown_on_panic(struct notifier_block *nb,
> @@ -1361,6 +1354,7 @@ static int snp_filter_reserved_mem_regions(struct resource *rs, void *arg)
>  
>  static int __sev_snp_init_locked(int *error, unsigned int max_snp_asid)
>  {
> +	struct sev_data_range_list *snp_range_list __free(kfree) = NULL;
>  	struct psp_device *psp = psp_master;
>  	struct sev_data_snp_init_ex data;
>  	struct sev_device *sev;
> @@ -2780,11 +2774,6 @@ static void __sev_firmware_shutdown(struct sev_device *sev, bool panic)
>  		sev_init_ex_buffer = NULL;
>  	}
>  
> -	if (snp_range_list) {
> -		kfree(snp_range_list);
> -		snp_range_list = NULL;
> -	}
> -
>  	__sev_snp_shutdown_locked(&error, panic);
>  }
>  

Re: [PATCH 1/2] crypto: ccp - Fix a case where SNP_SHUTDOWN is missed

[Herbert Xu]

On Mon, Jan 05, 2026 at 10:22:17AM -0700, Tycho Andersen wrote:
> From: Tom Lendacky <thomas.lendacky@amd.com>
> 
> If page reclaim fails in sev_ioctl_do_snp_platform_status() and SNP was
> moved from UNINIT to INIT for the function, SNP is not moved back to
> UNINIT state. Additionally, SNP is not required to be initialized in order
> to execute the SNP_PLATFORM_STATUS command, so don't attempt to move to
> INIT state and let SNP_PLATFORM_STATUS report the status as is.
> 
> Fixes: ceac7fb89e8d ("crypto: ccp - Ensure implicit SEV/SNP init and shutdown in ioctls")
> Signed-off-by: Tom Lendacky <thomas.lendacky@amd.com>
> Reviewed-by: Tycho Andersen (AMD) <tycho@kernel.org>
> Reviewed-by: Alexey Kardashevskiy <aik@amd.com>
> Signed-off-by: Tycho Andersen (AMD) <tycho@kernel.org>
> ---
>  drivers/crypto/ccp/sev-dev.c | 46 ++++++++++++++++++------------------
>  1 file changed, 23 insertions(+), 23 deletions(-)

All applied.  Thanks.
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [PATCH 1/2] crypto: ccp - Fix a case where SNP_SHUTDOWN is missed

[Guenter Roeck]

Hi,

On Mon, Jan 05, 2026 at 10:22:17AM -0700, Tycho Andersen wrote:
> From: Tom Lendacky <thomas.lendacky@amd.com>
> 
> If page reclaim fails in sev_ioctl_do_snp_platform_status() and SNP was
> moved from UNINIT to INIT for the function, SNP is not moved back to
> UNINIT state. Additionally, SNP is not required to be initialized in order
> to execute the SNP_PLATFORM_STATUS command, so don't attempt to move to
> INIT state and let SNP_PLATFORM_STATUS report the status as is.
> 
> Fixes: ceac7fb89e8d ("crypto: ccp - Ensure implicit SEV/SNP init and shutdown in ioctls")
> Signed-off-by: Tom Lendacky <thomas.lendacky@amd.com>
> Reviewed-by: Tycho Andersen (AMD) <tycho@kernel.org>
> Reviewed-by: Alexey Kardashevskiy <aik@amd.com>
> Signed-off-by: Tycho Andersen (AMD) <tycho@kernel.org>
> ---
>  drivers/crypto/ccp/sev-dev.c | 46 ++++++++++++++++++------------------
>  1 file changed, 23 insertions(+), 23 deletions(-)
> 
> -	if (snp_reclaim_pages(__pa(data), 1, true))
> -		return -EFAULT;
> +	if (sev->snp_initialized) {
> +		/*
> +		 * The status page will be in Reclaim state on success, or left
> +		 * in Firmware state on failure. Use snp_reclaim_pages() to
> +		 * transition either case back to Hypervisor-owned state.
> +		 */
> +		if (snp_reclaim_pages(__pa(data), 1, true)) {
> +			snp_leak_pages(__page_to_pfn(status_page), 1);

This change got flagged by an experimental AI agent:

  If `snp_reclaim_pages()` fails, it already internally calls
  `snp_leak_pages()`. Does calling `snp_leak_pages()` a second time
  on the exact same page corrupt the `snp_leaked_pages_list` because
  `list_add_tail(&page->buddy_list, &snp_leaked_pages_list)` is
  executed again?

I don't claim to understand the code, but it does look like snp_leak_pages()
is indeed called twice on the same page, which does suggest that it is added
twice to the leaked pages list if it is not a compound page.

Does this make sense, or is the AI missing something ?

Thanks,
Guenter

Re: [PATCH 1/2] crypto: ccp - Fix a case where SNP_SHUTDOWN is missed

[Tycho Andersen]

Hi Guenter,

On Tue, Mar 03, 2026 at 02:35:10PM -0800, Guenter Roeck wrote:
> Hi,
> 
> On Mon, Jan 05, 2026 at 10:22:17AM -0700, Tycho Andersen wrote:
> > From: Tom Lendacky <thomas.lendacky@amd.com>
> > 
> > If page reclaim fails in sev_ioctl_do_snp_platform_status() and SNP was
> > moved from UNINIT to INIT for the function, SNP is not moved back to
> > UNINIT state. Additionally, SNP is not required to be initialized in order
> > to execute the SNP_PLATFORM_STATUS command, so don't attempt to move to
> > INIT state and let SNP_PLATFORM_STATUS report the status as is.
> > 
> > Fixes: ceac7fb89e8d ("crypto: ccp - Ensure implicit SEV/SNP init and shutdown in ioctls")
> > Signed-off-by: Tom Lendacky <thomas.lendacky@amd.com>
> > Reviewed-by: Tycho Andersen (AMD) <tycho@kernel.org>
> > Reviewed-by: Alexey Kardashevskiy <aik@amd.com>
> > Signed-off-by: Tycho Andersen (AMD) <tycho@kernel.org>
> > ---
> >  drivers/crypto/ccp/sev-dev.c | 46 ++++++++++++++++++------------------
> >  1 file changed, 23 insertions(+), 23 deletions(-)
> > 
> > -	if (snp_reclaim_pages(__pa(data), 1, true))
> > -		return -EFAULT;
> > +	if (sev->snp_initialized) {
> > +		/*
> > +		 * The status page will be in Reclaim state on success, or left
> > +		 * in Firmware state on failure. Use snp_reclaim_pages() to
> > +		 * transition either case back to Hypervisor-owned state.
> > +		 */
> > +		if (snp_reclaim_pages(__pa(data), 1, true)) {
> > +			snp_leak_pages(__page_to_pfn(status_page), 1);
> 
> This change got flagged by an experimental AI agent:
> 
>   If `snp_reclaim_pages()` fails, it already internally calls
>   `snp_leak_pages()`. Does calling `snp_leak_pages()` a second time
>   on the exact same page corrupt the `snp_leaked_pages_list` because
>   `list_add_tail(&page->buddy_list, &snp_leaked_pages_list)` is
>   executed again?
> 
> I don't claim to understand the code, but it does look like snp_leak_pages()
> is indeed called twice on the same page, which does suggest that it is added
> twice to the leaked pages list if it is not a compound page.
> 
> Does this make sense, or is the AI missing something ?

Thanks for flagging this, I agree. I think we can drop that call:

diff --git a/drivers/crypto/ccp/sev-dev.c b/drivers/crypto/ccp/sev-dev.c
index 096f993974d1..bd31ebfc85d5 100644
--- a/drivers/crypto/ccp/sev-dev.c
+++ b/drivers/crypto/ccp/sev-dev.c
@@ -2410,10 +2410,8 @@ static int sev_ioctl_do_snp_platform_status(struct sev_issue_cmd *argp)
 		 * in Firmware state on failure. Use snp_reclaim_pages() to
 		 * transition either case back to Hypervisor-owned state.
 		 */
-		if (snp_reclaim_pages(__pa(data), 1, true)) {
-			snp_leak_pages(__page_to_pfn(status_page), 1);
+		if (snp_reclaim_pages(__pa(data), 1, true))
 			return -EFAULT;
-		}
 	}
 
 	if (ret)

Double checking other uses of snp_reclaim_pages(), I don't think
anyone else makes this mistake.

Do you want to send a patch? Otherwise, how do I credit via
Reported-by:, to just you?

Thanks,

Tycho

Re: [PATCH 1/2] crypto: ccp - Fix a case where SNP_SHUTDOWN is missed

[Guenter Roeck]

On 3/4/26 06:59, Tycho Andersen wrote:
> Hi Guenter,
> 
> On Tue, Mar 03, 2026 at 02:35:10PM -0800, Guenter Roeck wrote:
>> Hi,
>>
>> On Mon, Jan 05, 2026 at 10:22:17AM -0700, Tycho Andersen wrote:
>>> From: Tom Lendacky <thomas.lendacky@amd.com>
>>>
>>> If page reclaim fails in sev_ioctl_do_snp_platform_status() and SNP was
>>> moved from UNINIT to INIT for the function, SNP is not moved back to
>>> UNINIT state. Additionally, SNP is not required to be initialized in order
>>> to execute the SNP_PLATFORM_STATUS command, so don't attempt to move to
>>> INIT state and let SNP_PLATFORM_STATUS report the status as is.
>>>
>>> Fixes: ceac7fb89e8d ("crypto: ccp - Ensure implicit SEV/SNP init and shutdown in ioctls")
>>> Signed-off-by: Tom Lendacky <thomas.lendacky@amd.com>
>>> Reviewed-by: Tycho Andersen (AMD) <tycho@kernel.org>
>>> Reviewed-by: Alexey Kardashevskiy <aik@amd.com>
>>> Signed-off-by: Tycho Andersen (AMD) <tycho@kernel.org>
>>> ---
>>>   drivers/crypto/ccp/sev-dev.c | 46 ++++++++++++++++++------------------
>>>   1 file changed, 23 insertions(+), 23 deletions(-)
>>>
>>> -	if (snp_reclaim_pages(__pa(data), 1, true))
>>> -		return -EFAULT;
>>> +	if (sev->snp_initialized) {
>>> +		/*
>>> +		 * The status page will be in Reclaim state on success, or left
>>> +		 * in Firmware state on failure. Use snp_reclaim_pages() to
>>> +		 * transition either case back to Hypervisor-owned state.
>>> +		 */
>>> +		if (snp_reclaim_pages(__pa(data), 1, true)) {
>>> +			snp_leak_pages(__page_to_pfn(status_page), 1);
>>
>> This change got flagged by an experimental AI agent:
>>
>>    If `snp_reclaim_pages()` fails, it already internally calls
>>    `snp_leak_pages()`. Does calling `snp_leak_pages()` a second time
>>    on the exact same page corrupt the `snp_leaked_pages_list` because
>>    `list_add_tail(&page->buddy_list, &snp_leaked_pages_list)` is
>>    executed again?
>>
>> I don't claim to understand the code, but it does look like snp_leak_pages()
>> is indeed called twice on the same page, which does suggest that it is added
>> twice to the leaked pages list if it is not a compound page.
>>
>> Does this make sense, or is the AI missing something ?
> 
> Thanks for flagging this, I agree. I think we can drop that call:
> 
> diff --git a/drivers/crypto/ccp/sev-dev.c b/drivers/crypto/ccp/sev-dev.c
> index 096f993974d1..bd31ebfc85d5 100644
> --- a/drivers/crypto/ccp/sev-dev.c
> +++ b/drivers/crypto/ccp/sev-dev.c
> @@ -2410,10 +2410,8 @@ static int sev_ioctl_do_snp_platform_status(struct sev_issue_cmd *argp)
>   		 * in Firmware state on failure. Use snp_reclaim_pages() to
>   		 * transition either case back to Hypervisor-owned state.
>   		 */
> -		if (snp_reclaim_pages(__pa(data), 1, true)) {
> -			snp_leak_pages(__page_to_pfn(status_page), 1);
> +		if (snp_reclaim_pages(__pa(data), 1, true))
>   			return -EFAULT;
> -		}
>   	}
>   
>   	if (ret)
> 
> Double checking other uses of snp_reclaim_pages(), I don't think
> anyone else makes this mistake.
> 
> Do you want to send a patch? Otherwise, how do I credit via
> Reported-by:, to just you?
> 

I'll send it.

Thanks,
Guenter