linux-kernel - Re: [PATCH] KVM: x86: fix missed memory synchronization when patch hypercall

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: <20130609101934.GN4725@redhat.com>
Date:	Sun, 9 Jun 2013 13:19:34 +0300
From:	Gleb Natapov <gleb@...hat.com>
To:	Xiao Guangrong <xiaoguangrong.eric@...il.com>
Cc:	Paolo Bonzini <pbonzini@...hat.com>,
	Marcelo Tosatti <mtosatti@...hat.com>,
	LKML <linux-kernel@...r.kernel.org>, KVM <kvm@...r.kernel.org>
Subject: Re: [PATCH] KVM: x86: fix missed memory synchronization when patch
 hypercall

On Sun, Jun 09, 2013 at 06:01:45PM +0800, Xiao Guangrong wrote:
> On 06/09/2013 05:39 PM, Gleb Natapov wrote:
> > On Sun, Jun 09, 2013 at 05:29:37PM +0800, Xiao Guangrong wrote:
> >> On 06/09/2013 04:45 PM, Gleb Natapov wrote:
> >>
> >>> +static int emulator_fix_hypercall(struct x86_emulate_ctxt *ctxt)
> >>> +{
> >>> +	struct kvm_vcpu *vcpu = emul_to_vcpu(ctxt);
> >>> +	return kvm_exec_with_stopped_vcpu(vcpu->kvm,
> >>> +			emulator_fix_hypercall_cb, ctxt);
> >>> +}
> >>> +
> >>> +
> >>>  /*
> >>>   * Check if userspace requested an interrupt window, and that the
> >>>   * interrupt window is open.
> >>> @@ -5761,6 +5769,10 @@ static int vcpu_enter_guest(struct kvm_vcpu *vcpu)
> >>>  			kvm_deliver_pmi(vcpu);
> >>>  		if (kvm_check_request(KVM_REQ_SCAN_IOAPIC, vcpu))
> >>>  			vcpu_scan_ioapic(vcpu);
> >>> +		if (kvm_check_request(KVM_REQ_STOP_VCPU, vcpu)){
> >>> +			mutex_lock(&vcpu->kvm->lock);
> >>> +			mutex_unlock(&vcpu->kvm->lock);
> >>
> >> We should execute a serializing instruction here?
> >>
> >>> --- a/virt/kvm/kvm_main.c
> >>> +++ b/virt/kvm/kvm_main.c
> >>> @@ -222,6 +222,18 @@ void kvm_make_scan_ioapic_request(struct kvm *kvm)
> >>>  	make_all_cpus_request(kvm, KVM_REQ_SCAN_IOAPIC);
> >>>  }
> >>>
> >>> +int kvm_exec_with_stopped_vcpu(struct kvm *kvm, int (*cb)(void *), void *data)
> >>> +{
> >>> +	int r;
> >>> +
> >>> +	mutex_lock(&kvm->lock);
> >>> +	make_all_cpus_request(kvm, KVM_REQ_STOP_VCPU);
> >>> +	r = cb(data);
> >>
> >> And here?
> > Since the serialisation instruction the SDM suggest to use is CPUID I
> > think the point here is to flush CPU pipeline. Since all vcpus are out
> > of a guest mode I think out of order execution of modified instruction
> > is no an issue here.
> 
> I checked the SDM that it did not said VMLAUNCH/VMRESUME are the
> serializing instructions both in VM-Entry description and Instruction
> reference, instead it said the VMX related serializing instructions are:
> INVEPT, INVVPID.
> 
> So, i guess the explicit serializing instruction is needed here.
> 
Again the question is what for? SDM says:

  The Intel 64 and IA-32 architectures define several serializing
  instructions. These instructions force the processor to complete all
  modifications to flags, registers, and memory by previous instructions
  and to drain all buffered writes to memory before the next instruction
  is fetched and executed.

So flags and registers modifications on a host are obviously irrelevant for a guest.
And for memory ordering we have smp_mb() on a guest entry.


--
			Gleb.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/