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@xxxxxxx>
> > 
> > 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@xxxxxxx>
> > Reviewed-by: Tycho Andersen (AMD) <tycho@xxxxxxxxxx>
> > Reviewed-by: Alexey Kardashevskiy <aik@xxxxxxx>
> > Signed-off-by: Tycho Andersen (AMD) <tycho@xxxxxxxxxx>
> > ---
> >  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