2020-01-02 02:20:09

by Miaohe Lin

Subject: [PATCH] KVM: SVM: Fix potential memory leak in svm_cpu_init()

From: Miaohe Lin <[email protected]>

When kmalloc memory for sd->sev_vmcbs failed, we forget to free the page
held by sd->save_area.

Signed-off-by: Miaohe Lin <[email protected]>
---
arch/x86/kvm/svm.c | 8 +++++---
1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c
index 8f1b715dfde8..89eb382e8580 100644
--- a/arch/x86/kvm/svm.c
+++ b/arch/x86/kvm/svm.c
@@ -1012,7 +1012,7 @@ static int svm_cpu_init(int cpu)
r = -ENOMEM;
sd->save_area = alloc_page(GFP_KERNEL);
if (!sd->save_area)
- goto err_1;
+ goto free_cpu_data;

if (svm_sev_enabled()) {
r = -ENOMEM;
@@ -1020,14 +1020,16 @@ static int svm_cpu_init(int cpu)
sizeof(void *),
GFP_KERNEL);
if (!sd->sev_vmcbs)
- goto err_1;
+ goto free_save_area;
}

per_cpu(svm_data, cpu) = sd;

return 0;

-err_1:
+free_save_area:
+ __free_page(sd->save_area);
+free_cpu_data:
kfree(sd);
return r;

--
2.19.1

2020-01-02 13:24:52

by Liran Alon

Subject: Re: [PATCH] KVM: SVM: Fix potential memory leak in svm_cpu_init()

> On 2 Jan 2020, at 4:20, linmiaohe <[email protected]> wrote:
>
> From: Miaohe Lin <[email protected]>
>
> When kmalloc memory for sd->sev_vmcbs failed, we forget to free the page
> held by sd->save_area.
>
> Signed-off-by: Miaohe Lin <[email protected]>

Reviewed-by: Liran Alon <[email protected]>

-Liran

> ---
> arch/x86/kvm/svm.c | 8 +++++---
> 1 file changed, 5 insertions(+), 3 deletions(-)
>
> diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c
> index 8f1b715dfde8..89eb382e8580 100644
> --- a/arch/x86/kvm/svm.c
> +++ b/arch/x86/kvm/svm.c
> @@ -1012,7 +1012,7 @@ static int svm_cpu_init(int cpu)
> r = -ENOMEM;
> sd->save_area = alloc_page(GFP_KERNEL);
> if (!sd->save_area)
> - goto err_1;
> + goto free_cpu_data;
>
> if (svm_sev_enabled()) {
> r = -ENOMEM;
> @@ -1020,14 +1020,16 @@ static int svm_cpu_init(int cpu)
> sizeof(void *),
> GFP_KERNEL);
> if (!sd->sev_vmcbs)
> - goto err_1;
> + goto free_save_area;
> }
>
> per_cpu(svm_data, cpu) = sd;
>
> return 0;
>
> -err_1:
> +free_save_area:
> + __free_page(sd->save_area);
> +free_cpu_data:
> kfree(sd);
> return r;
>
> --
> 2.19.1
>

2020-01-02 13:31:54

by Vitaly Kuznetsov

Subject: Re: [PATCH] KVM: SVM: Fix potential memory leak in svm_cpu_init()

linmiaohe <[email protected]> writes:

> From: Miaohe Lin <[email protected]>
>
> When kmalloc memory for sd->sev_vmcbs failed, we forget to free the page
> held by sd->save_area.
>
> Signed-off-by: Miaohe Lin <[email protected]>
> ---
> arch/x86/kvm/svm.c | 8 +++++---
> 1 file changed, 5 insertions(+), 3 deletions(-)
>
> diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c
> index 8f1b715dfde8..89eb382e8580 100644
> --- a/arch/x86/kvm/svm.c
> +++ b/arch/x86/kvm/svm.c
> @@ -1012,7 +1012,7 @@ static int svm_cpu_init(int cpu)
> r = -ENOMEM;
> sd->save_area = alloc_page(GFP_KERNEL);
> if (!sd->save_area)
> - goto err_1;
> + goto free_cpu_data;
>
> if (svm_sev_enabled()) {
> r = -ENOMEM;

Not your fault but this assignment to 'r' seem to be redundant: it is
already set to '-ENOMEM' above, but this is also not perfect as ...

> @@ -1020,14 +1020,16 @@ static int svm_cpu_init(int cpu)
> sizeof(void *),
> GFP_KERNEL);
> if (!sd->sev_vmcbs)
> - goto err_1;
> + goto free_save_area;
> }
>
> per_cpu(svm_data, cpu) = sd;
>
> return 0;
>
> -err_1:
> +free_save_area:
> + __free_page(sd->save_area);
> +free_cpu_data:
> kfree(sd);
> return r;

... '-ENOMEM' is actually the only possible outcome here. In case you'll
be re-submitting, I'd suggest we drop 'r' entirely and just reture
-ENOMEM here.

Anyways, your patch seems to be correct, so:

Reviewed-by: Vitaly Kuznetsov <[email protected]>

--
Vitaly

2020-01-03 02:20:31

by Miaohe Lin

Subject: Re: [PATCH] KVM: SVM: Fix potential memory leak in svm_cpu_init()

Vitaly writes:
>> From: Miaohe Lin <[email protected]>
>> if (svm_sev_enabled()) {
>> r = -ENOMEM;
>
>Not your fault but this assignment to 'r' seem to be redundant: it is already set to '-ENOMEM' above, but this is also not perfect as ...
>
>> @@ -1020,14 +1020,16 @@ static int svm_cpu_init(int cpu)
>> sizeof(void *),
>> return r;
>
>... '-ENOMEM' is actually the only possible outcome here. In case you'll be re-submitting, I'd suggest we drop 'r' entirely and just reture -ENOMEM here.

The var r is really unnecessary and we should clean it up. Thanks for your good suggest. I would send a patch v2 soon.

>
>Anyways, your patch seems to be correct, so:
>
>Reviewed-by: Vitaly Kuznetsov <[email protected]>

Thanks for your review.