From mboxrd@z Thu Jan  1 00:00:00 1970
From: Gleb Natapov <gleb@redhat.com>
Subject: Re: KVM: x86: drop alignment checks from KVM_MSR_SYSTEM_TIME address
Date: Sat, 23 Mar 2013 16:12:11 +0200
Message-ID: <20130323141211.GA26995@redhat.com>
References: <20130322191407.GA23681@amt.cnet>
 <20130322192100.GA9382@redhat.com>
 <20130322194735.GA21288@amt.cnet>
 <20130322211947.GA2685@amt.cnet>
 <20130322215708.GB9382@redhat.com>
 <CAKB9nXsBWku77VsERxX4u8HFv4s+aZ2zgvxTYKS30PRnqXcWFg@mail.gmail.com>
Mime-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Cc: Marcelo Tosatti <mtosatti@redhat.com>, kvm <kvm@vger.kernel.org>,
	Eric Northup <digitaleric@google.com>
To: Andrew Honig <ahonig@google.com>
Return-path: <kvm-owner@vger.kernel.org>
Received: from mx1.redhat.com ([209.132.183.28]:15670 "EHLO mx1.redhat.com"
	rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP
	id S1750806Ab3CWOMO (ORCPT <rfc822;kvm@vger.kernel.org>);
	Sat, 23 Mar 2013 10:12:14 -0400
Content-Disposition: inline
In-Reply-To: <CAKB9nXsBWku77VsERxX4u8HFv4s+aZ2zgvxTYKS30PRnqXcWFg@mail.gmail.com>
Sender: kvm-owner@vger.kernel.org
List-ID: <kvm.vger.kernel.org>

On Fri, Mar 22, 2013 at 05:17:38PM -0700, Andrew Honig wrote:
> kvm_write_guest would work, but it will hurt performance a bit because
> it'll be doing the address translation each time the time is updated,
> which happens on most guest enters.
> 
Time updates are rare, so this should no be an issue. Marcelo?

> Another possibility would be to change kvm_gfn_to_hva_cache_init to
> accept a size parameter.  If the requested range is all on one page
> then it operates the same as it currently does.  If the address range
> is on more than one page then it falls back to kvm_write_guest.  This
> preserves the good performance for all cases that currently work,
> while still supporting the unlikely case of page straddling requests.
> It also makes it harder to write a security bugs for other callers of
> kvm_gfn_to_hva_cache_init by explicitly requiring a size parameter.
> 
> I can write a patch if you like the idea.
Nice idea. Send a patch please.

> 
> Andy
> 
> On Fri, Mar 22, 2013 at 2:57 PM, Gleb Natapov <gleb@redhat.com> wrote:
> > On Fri, Mar 22, 2013 at 06:19:47PM -0300, Marcelo Tosatti wrote:
> >>
> >> The alignment check is not necessary given that "KVM: x86: Convert
> >> MSR_KVM_SYSTEM_TIME to use gfn_to_hva_cache" (0b79459b482e85cb742)
> >> uses kvm_write_guest which is able to handle data across page
> >> properly.
> >>
> > It uses kvm_write_guest_cached which calls to copy_to_user() directly.
> > While this will not allow to overwrite page that does not belong to qemu
> > process, it is possible to write outside of memory slot. May be we
> > should use kvm_write_guest to do system time updates.
> >
> >> Signed-off-by: Marcelo Tosatti <mtosatti@redhat.com>
> >>
> >> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> >> index f19ac0a..ec830fa 100644
> >> --- a/arch/x86/kvm/x86.c
> >> +++ b/arch/x86/kvm/x86.c
> >> @@ -1952,10 +1952,6 @@ int kvm_set_msr_common(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
> >>
> >>               gpa_offset = data & ~(PAGE_MASK | 1);
> >>
> >> -             /* Check that the address is 32-byte aligned. */
> >> -             if (gpa_offset & (sizeof(struct pvclock_vcpu_time_info) - 1))
> >> -                     break;
> >> -
> >>               if (kvm_gfn_to_hva_cache_init(vcpu->kvm,
> >>                    &vcpu->arch.pv_time, data & ~1ULL))
> >>                       vcpu->arch.pv_time_enabled = false;
> >
> > --
> >                         Gleb.

--
			Gleb.