linux-kernel - Re: [PATCH 5/5] KVM: nVMX: Allow VMREAD when Enlightened VMCS is in use

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <87a6g8orr0.fsf@redhat.com>
Date:   Fri, 07 Jan 2022 09:49:07 +0100
From:   Vitaly Kuznetsov <vkuznets@...hat.com>
To:     Sean Christopherson <seanjc@...gle.com>
Cc:     kvm@...r.kernel.org, Paolo Bonzini <pbonzini@...hat.com>,
        Wanpeng Li <wanpengli@...cent.com>,
        Jim Mattson <jmattson@...gle.com>,
        Maxim Levitsky <mlevitsk@...hat.com>,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH 5/5] KVM: nVMX: Allow VMREAD when Enlightened VMCS is in
 use

Sean Christopherson <seanjc@...gle.com> writes:

> On Tue, Dec 14, 2021, Vitaly Kuznetsov wrote:
>> Hyper-V TLFS explicitly forbids VMREAD and VMWRITE instructions when
>> Enlightened VMCS interface is in use:
>> 
>> "Any VMREAD or VMWRITE instructions while an enlightened VMCS is
>> active is unsupported and can result in unexpected behavior.""
>> 
>> Windows 11 + WSL2 seems to ignore this, attempts to VMREAD VMCS field
>> 0x4404 ("VM-exit interruption information") are observed. Failing
>> these attempts with nested_vmx_failInvalid() makes such guests
>> unbootable.
>> 
>> Microsoft confirms this is a Hyper-V bug and claims that it'll get fixed
>> eventually but for the time being we need a workaround. (Temporary) allow
>
> Temporarily.  And for the record, I highly doubt this will be temporary :-)
>

I'm just trying to be optimistic, at least in commit messages, you know :-)

>> VMREAD to get data from the currently loaded Enlightened VMCS.
>
> ...
>
>> @@ -5074,27 +5075,44 @@ static int handle_vmread(struct kvm_vcpu *vcpu)
>>  	if (!nested_vmx_check_permission(vcpu))
>>  		return 1;
>>  
>> +	/* Normal or Enlightened VMPTRLD must be performed first */
>> +	if (vmx->nested.current_vmptr == INVALID_GPA &&
>> +	    !evmptr_is_valid(vmx->nested.hv_evmcs_vmptr))
>> +		return nested_vmx_failInvalid(vcpu);
>
> I believe this is wrong, as it allows this combination
>
> 	current_vmptr == INVALID_GPA && evmptr_is_valid() && is_guest_mode()
>
> which is eVMCS with VMCS shadowing exposed to L2.  SECONDARY_EXEC_SHADOW_VMCS is
> listed in EVMCS1_UNSUPPORTED_2NDEXEC, so it should be impossible for VMCS shadowing
> to be enabled for L2.  And if VMCS shadowing is not enabled, all VMREADs cause
> exits to L1, i.e. shouldn't reach this point.  If we want to allow that behavior,
> then I think that should be a separate change.

I think you're right, there's no need to allow for that. I'll use your
suggestion from below, thanks!

>
> Assuming eVMCS really isn't compatible with shadow VMCS, I believe we can do:
>
> 	/*
> 	 * Decode instruction info and find the field to read.  This can be
> 	 * done speculatively as there are no side effects
> 	 */
> 	field = kvm_register_read(vcpu, (((instr_info) >> 28) & 0xf));
>
> 	if (!evmptr_is_valid(vmx->nested.hv_evmcs_vmptr)) {
> 		/*
> 		 * In VMX non-root operation, when the VMCS-link pointer is
> 		 * INVALID_GPA, any VMREAD sets the ALU flags for VMfailInvalid.
> 		 */
> 		if (vmx->nested.current_vmptr == INVALID_GPA ||
> 		    (is_guest_mode(vcpu) &&
> 		     get_vmcs12(vcpu)->vmcs_link_pointer == INVALID_GPA))
> 			return nested_vmx_failInvalid(vcpu);
>
> 		offset = vmcs12_field_offset(field);
> 		if (offset < 0)
> 			return nested_vmx_fail(vcpu, VMXERR_UNSUPPORTED_VMCS_COMPONENT);
>
> 		if (!is_guest_mode(vcpu) && is_vmcs12_ext_field(field))
> 			copy_vmcs02_to_vmcs12_rare(vcpu, vmcs12);
>
> 		/* Read the field, zero-extended to a u64 value */
> 		value = vmcs12_read_any(vmcs12, field, offset);
> 	} else {
> 		/*
> 		 * <snarky comment about Hyper-V>
> 		 */
> 		if (WARN_ON_ONCE(is_guest_mode(vcpu))
> 			return nested_vmx_failInvalid(vcpu);
>
> 		offset = evmcs_field_offset(field, NULL);
> 		if (offset < 0)
> 			return nested_vmx_fail(vcpu, VMXERR_UNSUPPORTED_VMCS_COMPONENT);
>
> 		/* Read the field, zero-extended to a u64 value */
> 		value = evmcs_read_any(vmx->nested.hv_evmcs, field, offset);
> 	}
>

-- 
Vitaly