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

[Tom Lendacky]

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.

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.

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