Re: [PATCH v2 3/3] x86/vmx: Disallow the use of inactivity states

[Jan Beulich <jbeulich@xxxxxxxx>]

On 12.01.2024 00:13, Andrew Cooper wrote:
> Right now, vvmx will blindly copy L12's ACTIVITY_STATE into the L02 VMCS and
> enter the vCPU.  Luckily for us, nested-virt is explicitly unsupported for
> security bugs.
> 
> The inactivity states are HLT, SHUTDOWN and WAIT-FOR-SIPI, and as noted by the
> SDM in Vol3 27.7 "Special Features of VM Entry":
> 
>   If VM entry ends with the logical processor in an inactive activity state,
>   the VM entry generates any special bus cycle that is normally generated when
>   that activity state is entered from the active state.
> 
> Also,
> 
>   Some activity states unconditionally block certain events.
> 
> I.e. A VMEntry with ACTIVITY=SHUTDOWN will initiate a platform reset, while a
> VMEntry with ACTIVITY=WAIT-FOR-SIPI will really block everything other than
> SIPIs.
> 
> Both of these activity states are for the TXT ACM to use, not for regular
> hypervisors, and Xen doesn't support dropping the HLT intercept either.
> 
> There are two paths in Xen which operate on ACTIVITY_STATE.
> 
> 1) The vmx_{get,set}_nonreg_state() helpers for VM-Fork.
> 
>    As regular VMs can't use any inactivity states, this is just duplicating
>    the 0 from construct_vmcs().  Retain the ability to query activity_state,
>    but crash the domain on any attempt to set an inactivity state.
> 
> 2) Nested virt, because of ACTIVITY_STATE in vmcs_gstate_field[].
> 
>    Explicitly hide the inactivity states in the guest's view of MSR_VMX_MISC,
>    and remove ACTIVITY_STATE from vmcs_gstate_field[].
> 
>    In virtual_vmentry(), we should trigger a VMEntry failure for the use of
>    any inactivity states, but there's no support for that in the code at all
>    so leave a TODO for when we finally start working on nested-virt in
>    earnest.
> 
> Reported-by: Reima Ishii <ishiir@xxxxxxxxxxxxxxxxxxx>
> Signed-off-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>

Reviewed-by: Jan Beulich <jbeulich@xxxxxxxx>
with one remark/suggestion:

> --- a/xen/arch/x86/hvm/vmx/vmx.c
> +++ b/xen/arch/x86/hvm/vmx/vmx.c
> @@ -1551,7 +1551,10 @@ static void cf_check vmx_set_nonreg_state(struct vcpu 
> *v,
>  {
>      vmx_vmcs_enter(v);
>  
> -    __vmwrite(GUEST_ACTIVITY_STATE, nrs->vmx.activity_state);
> +    if ( nrs->vmx.activity_state )
> +        domain_crash(v->domain, "Attempt to set activity_state %#lx\n",
> +                     nrs->vmx.activity_state);

Might be useful to log the offending vCPU here?

Jan