Thanks for reviewing
On Tue, Jul 2, 2013 at 4:30 PM, Gleb Natapov <gleb@xxxxxxxxxx> wrote:
>
> On Tue, Jul 02, 2013 at 11:31:31AM +0800, Zhouyi Zhou wrote:
> > From: Zhouyi Zhou <yizhouzhou@xxxxxxxxx>
> >
> > Currently the callers of emulator_read_write_onepage do not check the value range
> > of the argument "bytes". If bytes is greater than the guest page size, many
> > unexpected things will happen.
> >
> The caller of emulator_read_write_onepage() explicitly checks that bytes
> do not cross page boundaries. Since emulator_read_write() should not
> be called to write more then 4k at a time this is enough. But even if
Yes, current Linux kernel will not trigger this marginal condition, so my patch
is just a suggestion :-)
> it is kvm_write_guest() makes sure that each write does not cross page
> boundary too.
kvm_write_guest increase the guest physical page number but not guest virtual
page, gfn++ will point to else where in guest.
>
> > This patch performs a check on the access size.
> >
> > Tested on my x86_64 machine
> What is tested? The patch fixes nothing. Can you trigger this WARN_ON()?
> How?
My research kernel originally use emulator_read_write to read write
lot of bytes,(now
I replace emulator_read_write with kvm_read_guest_virt)
official linux kernel has nowhere to trigger this WARN_ON. Can this patch
serves as a future safeguard?
>
> > Signed-off-by: Zhouyi Zhou <yizhouzhou@xxxxxxxxx>
> > ---
> > arch/x86/include/asm/kvm_host.h | 6 ++++++
> > arch/x86/kvm/paging_tmpl.h | 4 ++++
> > arch/x86/kvm/x86.c | 8 ++++++++
> > 3 files changed, 18 insertions(+)
> >
> > diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
> > index 3741c65..e28e6fe 100644
> > --- a/arch/x86/include/asm/kvm_host.h
> > +++ b/arch/x86/include/asm/kvm_host.h
> > @@ -239,6 +239,11 @@ struct kvm_pio_request {
> > int size;
> > };
> >
> > +enum guest_page_size {
> > + GUEST_PGSZ_4K = 0x0,
> > + GUEST_PGSZ_2M = 0x1,
> > + GUEST_PGSZ_4M = 0x2,
> > +};
> > /*
> > * x86 supports 3 paging modes (4-level 64-bit, 3-level 64-bit, and 2-level
> > * 32-bit). The kvm_mmu structure abstracts the details of the current mmu
> > @@ -289,6 +294,7 @@ struct kvm_mmu {
> > bool nx;
> >
> > u64 pdptrs[4]; /* pae */
> > + enum guest_page_size last_guest_page_size; /* last guest page size in walk*/
> > };
> >
> > enum pmc_type {
> > diff --git a/arch/x86/kvm/paging_tmpl.h b/arch/x86/kvm/paging_tmpl.h
> > index da20860..6eb0471 100644
> > --- a/arch/x86/kvm/paging_tmpl.h
> > +++ b/arch/x86/kvm/paging_tmpl.h
> > @@ -244,6 +244,10 @@ retry_walk:
> > real_gpa = mmu->translate_gpa(vcpu, gfn_to_gpa(gfn), access);
> > if (real_gpa == UNMAPPED_GVA)
> > return 0;
> > +
> > + vcpu->arch.walk_mmu->last_guest_page_size
> > + = !is_large_pte(pte) ? GUEST_PGSZ_4K :
> > + ((!is_long_mode(vcpu) && !is_pae(vcpu)) ? GUEST_PGSZ_4M : GUEST_PGSZ_2M);
> >
> > walker->gfn = real_gpa >> PAGE_SHIFT;
> >
> > diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> > index e8ba99c..de196f7 100644
> > --- a/arch/x86/kvm/x86.c
> > +++ b/arch/x86/kvm/x86.c
> > @@ -4130,6 +4130,8 @@ static int emulator_read_write_onepage(unsigned long addr, void *val,
> > bool write = ops->write;
> > struct kvm_mmio_fragment *frag;
> >
> > + vcpu->arch.walk_mmu->last_guest_page_size = GUEST_PGSZ_4K;
> > +
> > ret = vcpu_mmio_gva_to_gpa(vcpu, addr, &gpa, exception, write);
> >
> > if (ret < 0)
> > @@ -4139,7 +4141,12 @@ static int emulator_read_write_onepage(unsigned long addr, void *val,
> > if (ret)
> > goto mmio;
> >
> > + WARN_ON((vcpu->arch.walk_mmu->last_guest_page_size == GUEST_PGSZ_4K) ?
> > + (((gpa&(PAGE_SIZE-1)) + bytes) > PAGE_SIZE) :
> > + (vcpu->arch.walk_mmu->last_guest_page_size == GUEST_PGSZ_2M) ?
> > + (((gpa&(PAGE_SIZE-1)) + bytes) > HPAGE_SIZE) :
> > + (((gpa&(PAGE_SIZE-1)) + bytes) > HPAGE_SIZE*2));
> > +
> > if (ops->read_write_emulate(vcpu, gpa, val, bytes))
> > return X86EMUL_CONTINUE;
> >
> > --
> > 1.7.10.4
>
> --
> Gleb.
Thanks again
Zhouyi
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
