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

[Atish Patra <atish.patra@linux.dev>]

On 6/2/26 7:54 AM, Tom Lendacky wrote:
> On 6/1/26 18:04, Atish Patra wrote:
>> From: Atish Patra <atishp@meta.com>
>>
>> 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@kernel.org>
>> Signed-off-by: Atish Patra <atishp@meta.com>
> 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@amd.com>
>
>> ---
>>   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)
>>