Re: [PATCH v2 4/4] crypto: ccp: Fix memory leak in SEV INIT_EX path

[Atish Patra]

On 6/2/26 7:54 AM, Tom Lendacky wrote:
> On 6/1/26 18:04, Atish Patra wrote:
> > From: Atish Patra <atishp@xxxxxxxx>
> >
> > allocated pages in _init_ext_path are never freed and sev_init_ex_buffer
> > is left pointing at the leaked memory in case of any failures during the
> > function..
> >
> > Fix by adding an error path that frees the pages and clears
> > sev_init_ex_buffer. Make sure we only free the memory if the failure
> > happens before the conversion. Otherwise, we may end up trying to free
> > up converted pages in case of reclaim failure. rmp_mark_pages_firmware
> > failures should be rare enough to avoid more code complexity to track
> > down which pages were reclaimed/leaked vs which are not.
> >
> > Fixes: 7364a6fbca45 ("crypto: ccp: Handle non-volatile INIT_EX data when SNP is enabled")
> >
> > Reported-by: Sashiko <sashiko-bot@xxxxxxxxxx>
> > Signed-off-by: Atish Patra <atishp@xxxxxxxx>
> Not sure the goto's are the best, but they do the job - just a personal
> preference for me here.
>
> The new comment below is a bit verbose, I would think it is sufficient to
> just say something like "Pages can be in an inconsistent state, don't
> release them back to the system" or such.
Sure. I will update the comment.
> It might be nice in the future if we can identify if the reclaim was
> successful and use that for determining whether the pages are safe to
> freed... but the failure chance should be practically zero, so I'm not
> sure it is worth it.

Yes. I had started that path but was not sure if the code churn is worth it.
I can send it as a separate patch and we can take a call if you are 
think it's worth.

> Reviewed-by: Tom Lendacky <thomas.lendacky@xxxxxxx>
>
> > ---
> >   drivers/crypto/ccp/sev-dev.c | 16 ++++++++++++++--
> >   1 file changed, 14 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/crypto/ccp/sev-dev.c b/drivers/crypto/ccp/sev-dev.c
> > index 3d4793e8e34b..8566f164430b 100644
> > --- a/drivers/crypto/ccp/sev-dev.c
> > +++ b/drivers/crypto/ccp/sev-dev.c
> > @@ -1550,7 +1550,7 @@ static int __sev_platform_init_handle_init_ex_path(struct sev_device *sev)
> >   
> >   	rc = sev_read_init_ex_file();
> >   	if (rc)
> > -		return rc;
> > +		goto err_free;
> >   
> >   	/* If SEV-SNP is initialized, transition to firmware page. */
> >   	if (sev->snp_initialized) {
> > @@ -1559,11 +1559,23 @@ static int __sev_platform_init_handle_init_ex_path(struct sev_device *sev)
> >   		npages = 1UL << get_order(NV_LENGTH);
> >   		if (rmp_mark_pages_firmware(__pa(sev_init_ex_buffer), npages, true)) {
> >   			dev_err(sev->dev, "SEV: INIT_EX NV memory page state change failed.\n");
> > -			return -ENOMEM;
> > +			rc = -ENOMEM;
> > +			/*
> > +			 * Don't free on conversion failure: the rollback may
> > +			 * have left pages firmware-owned, and a high-order
> > +			 * block can't be partially freed.
> > +			 */
> > +			goto err_reset;
> >   		}
> >   	}
> >   
> >   	return 0;
> > +
> > +err_free:
> > +	__free_pages(page, get_order(NV_LENGTH));
> > +err_reset:
> > +	sev_init_ex_buffer = NULL;
> > +	return rc;
> >   }
> >   
> >   static int __sev_platform_init_locked(int *error)