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

From: Andrew Cooper <andrew.cooper3@citrix.com>
To: Jan Beulich <jbeulich@suse.com>
Cc: "Reima Ishii" <ishiir@g.ecc.u-tokyo.ac.jp>,
	"Roger Pau Monné" <roger.pau@citrix.com>, "Wei Liu" <wl@xen.org>,
	"Jun Nakajima" <jun.nakajima@intel.com>,
	"Kevin Tian" <kevin.tian@intel.com>,
	"Tamas K Lengyel" <tamas@tklengyel.com>,
	"Takahiro Shinagawa" <shina@ecc.u-tokyo.ac.jp>,
	"George Dunlap" <george.dunlap@citrix.com>,
	Xen-devel <xen-devel@lists.xenproject.org>
Subject: Re: [PATCH v2 3/3] x86/vmx: Disallow the use of inactivity states
Date: Fri, 12 Jan 2024 10:43:35 +0000	[thread overview]
Message-ID: <8225cfd3-73d6-4a93-92b4-185399ab0ad3@citrix.com> (raw)
In-Reply-To: <d9d9ee2b-ff95-4d5e-bb81-b1722681a4a5@suse.com>

On 12/01/2024 10:37 am, Jan Beulich wrote:
> 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@g.ecc.u-tokyo.ac.jp>
>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> Reviewed-by: Jan Beulich <jbeulich@suse.com>

Thanks.

> 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?

Already covered.  the innards of __domain_crash() does:

    else if ( d == current->domain )
    {
        printk("Domain %d (vcpu#%d) crashed on cpu#%d:\n",
        ...

~Andrew