Re: KVM: x86: drop alignment checks from KVM_MSR_SYSTEM_TIME address

[Gleb Natapov <gleb@redhat.com>]

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.