* Re: [PATCH tip] KVM: nSVM: avoid freeing uninitialized pointers in svm_set_nested_state()
2020-09-14 13:37 [PATCH tip] KVM: nSVM: avoid freeing uninitialized pointers in svm_set_nested_state() Vitaly Kuznetsov
@ 2020-09-14 15:10 ` Sean Christopherson
2020-09-14 15:44 ` Joerg Roedel
` (2 subsequent siblings)
3 siblings, 0 replies; 5+ messages in thread
From: Sean Christopherson @ 2020-09-14 15:10 UTC (permalink / raw)
To: Vitaly Kuznetsov
Cc: x86, kvm, linux-kernel, Borislav Petkov, Paolo Bonzini,
Wanpeng Li, Jim Mattson, Joerg Roedel, Dan Carpenter, Colin King,
Thomas Gleixner, Ingo Molnar
On Mon, Sep 14, 2020 at 03:37:25PM +0200, Vitaly Kuznetsov wrote:
> The save and ctl pointers are passed uninitialized to kfree() when
> svm_set_nested_state() follows the 'goto out_set_gif' path. While
> the issue could've been fixed by initializing these on-stack varialbles
> to NULL, it seems preferable to eliminate 'out_set_gif' label completely
> as it is not actually a failure path and duplicating a single svm_set_gif()
> call doesn't look too bad.
>
> Fixes: 6ccbd29ade0d ("KVM: SVM: nested: Don't allocate VMCB structures on stack")
> Addresses-Coverity: ("Uninitialized pointer read")
> Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
> Reported-by: Joerg Roedel <jroedel@suse.de>
> Reported-by: Colin King <colin.king@canonical.com>
> Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com>
> ---
Reviewed-by: Sean Christopherson <sean.j.christopherson@intel.com>
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH tip] KVM: nSVM: avoid freeing uninitialized pointers in svm_set_nested_state()
2020-09-14 13:37 [PATCH tip] KVM: nSVM: avoid freeing uninitialized pointers in svm_set_nested_state() Vitaly Kuznetsov
2020-09-14 15:10 ` Sean Christopherson
@ 2020-09-14 15:44 ` Joerg Roedel
2020-09-14 16:03 ` Tom Lendacky
2020-09-14 17:16 ` [tip: x86/seves] KVM: nSVM: Avoid " tip-bot2 for Vitaly Kuznetsov
3 siblings, 0 replies; 5+ messages in thread
From: Joerg Roedel @ 2020-09-14 15:44 UTC (permalink / raw)
To: Vitaly Kuznetsov
Cc: x86, kvm, linux-kernel, Borislav Petkov, Paolo Bonzini,
Sean Christopherson, Wanpeng Li, Jim Mattson, Dan Carpenter,
Colin King, Thomas Gleixner, Ingo Molnar
On Mon, Sep 14, 2020 at 03:37:25PM +0200, Vitaly Kuznetsov wrote:
> The save and ctl pointers are passed uninitialized to kfree() when
> svm_set_nested_state() follows the 'goto out_set_gif' path. While
> the issue could've been fixed by initializing these on-stack varialbles
> to NULL, it seems preferable to eliminate 'out_set_gif' label completely
> as it is not actually a failure path and duplicating a single svm_set_gif()
> call doesn't look too bad.
>
> Fixes: 6ccbd29ade0d ("KVM: SVM: nested: Don't allocate VMCB structures on stack")
> Addresses-Coverity: ("Uninitialized pointer read")
> Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
> Reported-by: Joerg Roedel <jroedel@suse.de>
> Reported-by: Colin King <colin.king@canonical.com>
> Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com>
Acked-by: Joerg Roedel <jroedel@suse.de>
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH tip] KVM: nSVM: avoid freeing uninitialized pointers in svm_set_nested_state()
2020-09-14 13:37 [PATCH tip] KVM: nSVM: avoid freeing uninitialized pointers in svm_set_nested_state() Vitaly Kuznetsov
2020-09-14 15:10 ` Sean Christopherson
2020-09-14 15:44 ` Joerg Roedel
@ 2020-09-14 16:03 ` Tom Lendacky
2020-09-14 17:16 ` [tip: x86/seves] KVM: nSVM: Avoid " tip-bot2 for Vitaly Kuznetsov
3 siblings, 0 replies; 5+ messages in thread
From: Tom Lendacky @ 2020-09-14 16:03 UTC (permalink / raw)
To: Vitaly Kuznetsov, x86
Cc: kvm, linux-kernel, Borislav Petkov, Paolo Bonzini,
Sean Christopherson, Wanpeng Li, Jim Mattson, Joerg Roedel,
Dan Carpenter, Colin King, Thomas Gleixner, Ingo Molnar
On 9/14/20 8:37 AM, Vitaly Kuznetsov wrote:
> The save and ctl pointers are passed uninitialized to kfree() when
> svm_set_nested_state() follows the 'goto out_set_gif' path. While
> the issue could've been fixed by initializing these on-stack varialbles
> to NULL, it seems preferable to eliminate 'out_set_gif' label completely
> as it is not actually a failure path and duplicating a single svm_set_gif()
> call doesn't look too bad.
>
> Fixes: 6ccbd29ade0d ("KVM: SVM: nested: Don't allocate VMCB structures on stack")
> Addresses-Coverity: ("Uninitialized pointer read")
> Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
> Reported-by: Joerg Roedel <jroedel@suse.de>
> Reported-by: Colin King <colin.king@canonical.com>
> Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com>
Tested-by: Tom Lendacky <thomas.lendacky@amd.com>
^ permalink raw reply [flat|nested] 5+ messages in thread* [tip: x86/seves] KVM: nSVM: Avoid freeing uninitialized pointers in svm_set_nested_state()
2020-09-14 13:37 [PATCH tip] KVM: nSVM: avoid freeing uninitialized pointers in svm_set_nested_state() Vitaly Kuznetsov
` (2 preceding siblings ...)
2020-09-14 16:03 ` Tom Lendacky
@ 2020-09-14 17:16 ` tip-bot2 for Vitaly Kuznetsov
3 siblings, 0 replies; 5+ messages in thread
From: tip-bot2 for Vitaly Kuznetsov @ 2020-09-14 17:16 UTC (permalink / raw)
To: linux-tip-commits
Cc: Dan Carpenter, Joerg Roedel, Colin King, Vitaly Kuznetsov,
Borislav Petkov, Sean Christopherson, Tom Lendacky, x86, LKML
The following commit has been merged into the x86/seves branch of tip:
Commit-ID: e6eb15c9ba3165698488ae5c34920eea20eaa38e
Gitweb: https://git.kernel.org/tip/e6eb15c9ba3165698488ae5c34920eea20eaa38e
Author: Vitaly Kuznetsov <vkuznets@redhat.com>
AuthorDate: Mon, 14 Sep 2020 15:37:25 +02:00
Committer: Borislav Petkov <bp@suse.de>
CommitterDate: Mon, 14 Sep 2020 18:08:54 +02:00
KVM: nSVM: Avoid freeing uninitialized pointers in svm_set_nested_state()
The save and ctl pointers are passed uninitialized to kfree() when
svm_set_nested_state() follows the 'goto out_set_gif' path. While the
issue could've been fixed by initializing these on-stack varialbles to
NULL, it seems preferable to eliminate 'out_set_gif' label completely as
it is not actually a failure path and duplicating a single svm_set_gif()
call doesn't look too bad.
[ bp: Drop obscure Addresses-Coverity: tag. ]
Fixes: 6ccbd29ade0d ("KVM: SVM: nested: Don't allocate VMCB structures on stack")
Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
Reported-by: Joerg Roedel <jroedel@suse.de>
Reported-by: Colin King <colin.king@canonical.com>
Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com>
Signed-off-by: Borislav Petkov <bp@suse.de>
Reviewed-by: Sean Christopherson <sean.j.christopherson@intel.com>
Acked-by: Joerg Roedel <jroedel@suse.de>
Tested-by: Tom Lendacky <thomas.lendacky@amd.com>
Link: https://lkml.kernel.org/r/20200914133725.650221-1-vkuznets@redhat.com
---
arch/x86/kvm/svm/nested.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/arch/x86/kvm/svm/nested.c b/arch/x86/kvm/svm/nested.c
index 2803662..d1ae94f 100644
--- a/arch/x86/kvm/svm/nested.c
+++ b/arch/x86/kvm/svm/nested.c
@@ -1092,7 +1092,8 @@ static int svm_set_nested_state(struct kvm_vcpu *vcpu,
if (!(kvm_state->flags & KVM_STATE_NESTED_GUEST_MODE)) {
svm_leave_nested(svm);
- goto out_set_gif;
+ svm_set_gif(svm, !!(kvm_state->flags & KVM_STATE_NESTED_GIF_SET));
+ return 0;
}
if (!page_address_valid(vcpu, kvm_state->hdr.svm.vmcb_pa))
@@ -1145,7 +1146,6 @@ static int svm_set_nested_state(struct kvm_vcpu *vcpu,
load_nested_vmcb_control(svm, ctl);
nested_prepare_vmcb_control(svm);
-out_set_gif:
svm_set_gif(svm, !!(kvm_state->flags & KVM_STATE_NESTED_GIF_SET));
ret = 0;
^ permalink raw reply related [flat|nested] 5+ messages in thread