linux-kernel - Re: [PATCH v2 5/7] KVM: nVMX: Reset eVMCS clean fields data from prepare

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] [thread-next>] [day] [month] [year] [list]

Message-ID: <87wnro5lga.fsf@vitty.brq.redhat.com>
Date:   Mon, 24 May 2021 15:07:49 +0200
From:   Vitaly Kuznetsov <vkuznets@...hat.com>
To:     Maxim Levitsky <mlevitsk@...hat.com>, kvm@...r.kernel.org,
        Paolo Bonzini <pbonzini@...hat.com>
Cc:     Sean Christopherson <seanjc@...gle.com>,
        Wanpeng Li <wanpengli@...cent.com>,
        Jim Mattson <jmattson@...gle.com>, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 5/7] KVM: nVMX: Reset eVMCS clean fields data from
 prepare_vmcs02()

Maxim Levitsky <mlevitsk@...hat.com> writes:

> On Mon, 2021-05-17 at 15:50 +0200, Vitaly Kuznetsov wrote:
>> When nested state migration happens during L1's execution, it
>> is incorrect to modify eVMCS as it is L1 who 'owns' it at the moment.
>> At lease genuine Hyper-v seems to not be very happy when 'clean fields'
>> data changes underneath it.
>> 
>> 'Clean fields' data is used in KVM twice: by copy_enlightened_to_vmcs12()
>> and prepare_vmcs02_rare() so we can reset it from prepare_vmcs02() instead.
>> 
>> Signed-off-by: Vitaly Kuznetsov <vkuznets@...hat.com>
>> ---
>>  arch/x86/kvm/vmx/nested.c | 14 ++++++++------
>>  1 file changed, 8 insertions(+), 6 deletions(-)
>> 
>> diff --git a/arch/x86/kvm/vmx/nested.c b/arch/x86/kvm/vmx/nested.c
>> index eb2d25a93356..3bfbf991bf45 100644
>> --- a/arch/x86/kvm/vmx/nested.c
>> +++ b/arch/x86/kvm/vmx/nested.c
>> @@ -2081,14 +2081,10 @@ void nested_sync_vmcs12_to_shadow(struct kvm_vcpu *vcpu)
>>  {
>>  	struct vcpu_vmx *vmx = to_vmx(vcpu);
>>  
>> -	if (vmx->nested.hv_evmcs) {
>> +	if (vmx->nested.hv_evmcs)
>>  		copy_vmcs12_to_enlightened(vmx);
>> -		/* All fields are clean */
>> -		vmx->nested.hv_evmcs->hv_clean_fields |=
>> -			HV_VMX_ENLIGHTENED_CLEAN_FIELD_ALL;
>> -	} else {
>> +	else
>>  		copy_vmcs12_to_shadow(vmx);
>> -	}
>>  
>>  	vmx->nested.need_vmcs12_to_shadow_sync = false;
>>  }
>> @@ -2629,6 +2625,12 @@ static int prepare_vmcs02(struct kvm_vcpu *vcpu, struct vmcs12 *vmcs12,
>>  
>>  	kvm_rsp_write(vcpu, vmcs12->guest_rsp);
>>  	kvm_rip_write(vcpu, vmcs12->guest_rip);
>> +
>> +	/* Mark all fields as clean so L1 hypervisor can set what's dirty */
>> +	if (hv_evmcs)
>> +		vmx->nested.hv_evmcs->hv_clean_fields |=
>> +			HV_VMX_ENLIGHTENED_CLEAN_FIELD_ALL;
>> +
>>  	return 0;
>>  }
>> 
>
> Hi!
>  
> If we avoid calling copy_enlightened_to_vmcs12 from 
> vmx_get_nested_state, then we don't need this patch, right?
>  

Right.

> In addition to that I think that we need to research on why 
> do we need to touch these clean bits, as from the spec, and
> assuming that the clean bits should behave similar to how AMD
> does it, clean bits should only be set by the L1 and never touched by
> us.
>  
> We currently set clean bits in two places:
>  
> 1. nested_vmx_handle_enlightened_vmptrld with vmlaunch, where it seems
> like it is a workaround for a case (as we discussed on IRC) where
> L1 keeps more than one active evmcs on a same vcpu, and 'vmresume's
> them. Since we don't support this and have to do full context switch
> when we switch a vmcs, we reset the clean bits so that evmcs is loaded
> fully.
> Also we reset the clean bits when a evmcs is 'vmlaunched' which
> is also something we need to check if needed, and if needed
> we probably should document that this is because of a bug in Hyper-V,
> as it really should initialize these bits in this case.
>  
> I think that we should just ignore the clean bits in those cases
> instead of resetting them in the evmcs.
>  
>  
> 2. In nested_sync_vmcs12_to_shadow which in practise is done only
> on nested vmexits, when we updated the vmcs12 and need to update evmcs.
> In this case you told me that Hyper-V has a bug that it expects
> the clean bits to be cleaned by us and doesn't clean it on its own.
> This makes sense although it is not documented in the Hyper-V spec,
> and I would appreciate if we were to document this explicitly in the code.

My memory is really foggy but I put 'hv_clean_fields' cleaning to KVM
because I discovered that Hyper-V doesn't do that: with every exit we
get more and more 'dirty' stuff which wasn't touched. It would probably
make sense to repeat the experiment. I'll put this to my backlog, thanks
for the feedback!

-- 
Vitaly