[PATCH] KVM: SVM: snp_alloc_firmware_pages: memory leak

[PATCH] KVM: SVM: snp_alloc_firmware_pages: memory leak

[Alexey Kardashevskiy]

Failure to rmpupdate leads to page(s) leak, fix that.

Signed-off-by: Alexey Kardashevskiy <aik@amd.com>
---
 drivers/crypto/ccp/sev-dev.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/crypto/ccp/sev-dev.c b/drivers/crypto/ccp/sev-dev.c
index 2e87ca0e292a..0b5f8ab657c5 100644
--- a/drivers/crypto/ccp/sev-dev.c
+++ b/drivers/crypto/ccp/sev-dev.c
@@ -443,8 +443,10 @@ static struct page *__snp_alloc_firmware_pages(gfp_t gfp_mask, int order)
 		return page;
 
 	paddr = __pa((unsigned long)page_address(page));
-	if (rmp_mark_pages_firmware(paddr, npages, false))
+	if (rmp_mark_pages_firmware(paddr, npages, false)) {
+		__free_pages(page, order);
 		return NULL;
+	}
 
 	return page;
 }
-- 
2.47.1

Re: [PATCH] KVM: SVM: snp_alloc_firmware_pages: memory leak

[Tom Lendacky]

On 2/13/25 21:59, Alexey Kardashevskiy wrote:
> Failure to rmpupdate leads to page(s) leak, fix that.
> 
> Signed-off-by: Alexey Kardashevskiy <aik@amd.com>
> ---
>  drivers/crypto/ccp/sev-dev.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/crypto/ccp/sev-dev.c b/drivers/crypto/ccp/sev-dev.c
> index 2e87ca0e292a..0b5f8ab657c5 100644
> --- a/drivers/crypto/ccp/sev-dev.c
> +++ b/drivers/crypto/ccp/sev-dev.c
> @@ -443,8 +443,10 @@ static struct page *__snp_alloc_firmware_pages(gfp_t gfp_mask, int order)
>  		return page;
>  
>  	paddr = __pa((unsigned long)page_address(page));
> -	if (rmp_mark_pages_firmware(paddr, npages, false))
> +	if (rmp_mark_pages_firmware(paddr, npages, false)) {
> +		__free_pages(page, order);

I'm not sure we can do this. On error, rmp_mark_pages_firmware() attempts
to cleanup and restore any pages that were marked firmware. But
snp_reclaim_pages() will leak pages that it can't restore and we don't
pass back any info to the caller of rmp_mark_pages_firmware() to let it
know what pages are truly available to free.

Thanks,
Tom

>  		return NULL;
> +	}
>  
>  	return page;
>  }

Re: [PATCH] KVM: SVM: snp_alloc_firmware_pages: memory leak

[Alexey Kardashevskiy]

On 15/2/25 01:53, Tom Lendacky wrote:
> On 2/13/25 21:59, Alexey Kardashevskiy wrote:
>> Failure to rmpupdate leads to page(s) leak, fix that.
>>
>> Signed-off-by: Alexey Kardashevskiy <aik@amd.com>
>> ---
>>   drivers/crypto/ccp/sev-dev.c | 4 +++-
>>   1 file changed, 3 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/crypto/ccp/sev-dev.c b/drivers/crypto/ccp/sev-dev.c
>> index 2e87ca0e292a..0b5f8ab657c5 100644
>> --- a/drivers/crypto/ccp/sev-dev.c
>> +++ b/drivers/crypto/ccp/sev-dev.c
>> @@ -443,8 +443,10 @@ static struct page *__snp_alloc_firmware_pages(gfp_t gfp_mask, int order)
>>   		return page;
>>   
>>   	paddr = __pa((unsigned long)page_address(page));
>> -	if (rmp_mark_pages_firmware(paddr, npages, false))
>> +	if (rmp_mark_pages_firmware(paddr, npages, false)) {
>> +		__free_pages(page, order);
> 
> I'm not sure we can do this. On error, rmp_mark_pages_firmware() attempts
> to cleanup and restore any pages that were marked firmware. But
> snp_reclaim_pages() will leak pages that it can't restore and we don't
> pass back any info to the caller of rmp_mark_pages_firmware() to let it
> know what pages are truly available to free.

oh right. But there is snp_leaked_pages_list which 
__snp_alloc_firmware_pages() could look at.

Or just replace __free_pages() above with:

snp_leak_pages(__page_to_pfn(page), 1 << order)

so memory leak leaves traces in dmesg, at least?


> 
> Thanks,
> Tom
> 
>>   		return NULL;
>> +	}
>>   
>>   	return page;
>>   }

-- 
Alexey

Re: [PATCH] KVM: SVM: snp_alloc_firmware_pages: memory leak

[Tom Lendacky]

On 2/17/25 19:24, Alexey Kardashevskiy wrote:
> On 15/2/25 01:53, Tom Lendacky wrote:
>> On 2/13/25 21:59, Alexey Kardashevskiy wrote:
>>> Failure to rmpupdate leads to page(s) leak, fix that.
>>>
>>> Signed-off-by: Alexey Kardashevskiy <aik@amd.com>
>>> ---
>>>   drivers/crypto/ccp/sev-dev.c | 4 +++-
>>>   1 file changed, 3 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/crypto/ccp/sev-dev.c b/drivers/crypto/ccp/sev-dev.c
>>> index 2e87ca0e292a..0b5f8ab657c5 100644
>>> --- a/drivers/crypto/ccp/sev-dev.c
>>> +++ b/drivers/crypto/ccp/sev-dev.c
>>> @@ -443,8 +443,10 @@ static struct page
>>> *__snp_alloc_firmware_pages(gfp_t gfp_mask, int order)
>>>           return page;
>>>         paddr = __pa((unsigned long)page_address(page));
>>> -    if (rmp_mark_pages_firmware(paddr, npages, false))
>>> +    if (rmp_mark_pages_firmware(paddr, npages, false)) {
>>> +        __free_pages(page, order);
>>
>> I'm not sure we can do this. On error, rmp_mark_pages_firmware() attempts
>> to cleanup and restore any pages that were marked firmware. But
>> snp_reclaim_pages() will leak pages that it can't restore and we don't
>> pass back any info to the caller of rmp_mark_pages_firmware() to let it
>> know what pages are truly available to free.
> 
> oh right. But there is snp_leaked_pages_list which
> __snp_alloc_firmware_pages() could look at.
> 
> Or just replace __free_pages() above with:
> 
> snp_leak_pages(__page_to_pfn(page), 1 << order)
> 
> so memory leak leaves traces in dmesg, at least?

I haven't looked too closely at the error path, but it might make sense
to have rmp_mark_pages_firmware() leak all the pages vs trying to do any
cleanup. Also, have snp_reclaim_pages() leak all the pages on any single
reclaim page error, because it looks like, in general, that the pages
are never free'd if any single page fails to be reclaimed, but only the
page that failed and then the remaining pages gets leaked via
snp_leak_pages().

Except in sev_ioctl_do_snp_platform_status(), where __free_pages() is
called if rmp_mark_pages_firmware() fails, which doesn't seem right.

And I'm not sure what to do if rmp_mark_pages_firmware() fails in
snp_prep_cmd_buf(), since __sev_do_cmd_locked() is using pre-allocated
buffers. But it looks like if snp_prep_cmd_buf() fails or
snp_reclaim_cmd_buf() fails, then the buffer usage indicator is never
released and commands will just fail at some point...  But those buffers
are allocated using devm_get_free_pages(), so nothing good would happen
if the ccp module is unloaded and those pages are freed in the wrong state.

Thanks,
Tom

> 
> 
>>
>> Thanks,
>> Tom
>>
>>>           return NULL;
>>> +    }
>>>         return page;
>>>   }
>