[4.17?] VMX: correct error handling in vmx_create_vmcs()

Commit Message

Jan Beulich Oct. 10, 2022, 10:25 a.m. UTC

With the addition of vmx_add_msr() calls to construct_vmcs() there are
now cases where simply freeing the VMCS isn't enough: The MSR bitmap
page as well as one of the MSR area ones (if it's the 2nd vmx_add_msr()
which fails) may also need freeing. Switch to using vmx_destroy_vmcs()
instead.

Fixes: 3bd36952dab6 ("x86/spec-ctrl: Introduce an option to control L1D_FLUSH for HVM HAP guests")
Fixes: 53a570b28569 ("x86/spec-ctrl: Support IBPB-on-entry")
Reported-by: Andrew Cooper <[e-mail]>
Signed-off-by: Jan Beulich <[e-mail]>
---
With both referenced commits having been for XSAs, we may want to
consider backporting this also to security-only stable trees. Otoh it's
"just" an error path ...

Comments

Henry Wang Oct. 10, 2022, 10:53 a.m. UTC | #1

Hi Jan,

> -----Original Message-----
> From: Jan Beulich <[e-mail]>
> Subject: [PATCH][4.17?] VMX: correct error handling in vmx_create_vmcs()
> 
> With the addition of vmx_add_msr() calls to construct_vmcs() there are
> now cases where simply freeing the VMCS isn't enough: The MSR bitmap
> page as well as one of the MSR area ones (if it's the 2nd vmx_add_msr()
> which fails) may also need freeing. Switch to using vmx_destroy_vmcs()
> instead.
> 
> Fixes: 3bd36952dab6 ("x86/spec-ctrl: Introduce an option to control
> L1D_FLUSH for HVM HAP guests")
> Fixes: 53a570b28569 ("x86/spec-ctrl: Support IBPB-on-entry")
> Reported-by: Andrew Cooper <[e-mail]>
> Signed-off-by: Jan Beulich <[e-mail]>

I think the change makes sense to me and it is quite simple, so I would
not object to having this change in 4.17. With proper review/ack from
other x86 maintainers:

Release-acked-by: Henry Wang <[e-mail]>

Kind regards,
Henry

Tian, Kevin Oct. 10, 2022, 11:19 p.m. UTC | #2

> From: Jan Beulich <[e-mail]>
> Sent: Monday, October 10, 2022 6:25 PM
> 
> With the addition of vmx_add_msr() calls to construct_vmcs() there are
> now cases where simply freeing the VMCS isn't enough: The MSR bitmap
> page as well as one of the MSR area ones (if it's the 2nd vmx_add_msr()
> which fails) may also need freeing. Switch to using vmx_destroy_vmcs()
> instead.
> 
> Fixes: 3bd36952dab6 ("x86/spec-ctrl: Introduce an option to control
> L1D_FLUSH for HVM HAP guests")
> Fixes: 53a570b28569 ("x86/spec-ctrl: Support IBPB-on-entry")
> Reported-by: Andrew Cooper <[e-mail]>
> Signed-off-by: Jan Beulich <[e-mail]>

Reviewed-by: Kevin Tian <[e-mail]>

Patch

--- a/xen/arch/x86/hvm/vmx/vmcs.c
+++ b/xen/arch/x86/hvm/vmx/vmcs.c
@@ -1821,7 +1821,7 @@  int vmx_create_vmcs(struct vcpu *v)
 
     if ( (rc = construct_vmcs(v)) != 0 )
     {
-        vmx_free_vmcs(vmx->vmcs_pa);
+        vmx_destroy_vmcs(v);
         return rc;
     }