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

I'll send it.

Thanks,
Guenter