| Message ID | 46ce7d1a-2a04-726c-919b-2d8bb4844391@suse.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | XSA-292 follow-up | expand |
On 17/09/2019 07:15, Jan Beulich wrote: > While bits 11 and below are, if not used for other purposes, reserved > but ignored, bits beyond physical address width are supposed to raise > exceptions (at least in the non-nested case; I'm not convinced the > current nested SVM/VMX behavior of raising #GP(0) here is correct, but > that's not the subject of this change). > > Introduce currd as a local variable, and replace other v->domain > instances at the same time. > > Signed-off-by: Jan Beulich <jbeulich@suse.com> > Reviewed-by: Roger Pau Monné <roger.pau@citrix.com> > --- > v2: Simplify the expressions used for the reserved bit checks. > > --- a/xen/arch/x86/hvm/hvm.c > +++ b/xen/arch/x86/hvm/hvm.c > @@ -1004,6 +1004,13 @@ static int hvm_load_cpu_ctxt(struct doma > return -EINVAL; > } > > + if ( ctxt.cr3 >> d->arch.cpuid->extd.maxphysaddr ) > + { > + printk(XENLOG_G_ERR "HVM%d restore: bad CR3 %#" PRIx64 "\n", > + d->domain_id, ctxt.cr3); > + return X86EMUL_EXCEPTION; -EINVAL, surely? Everything else LGTM (although I guess it depends on the previous patch?) so with this fixed, Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>
On 17.09.2019 21:35, Andrew Cooper wrote: > On 17/09/2019 07:15, Jan Beulich wrote: >> --- a/xen/arch/x86/hvm/hvm.c >> +++ b/xen/arch/x86/hvm/hvm.c >> @@ -1004,6 +1004,13 @@ static int hvm_load_cpu_ctxt(struct doma >> return -EINVAL; >> } >> >> + if ( ctxt.cr3 >> d->arch.cpuid->extd.maxphysaddr ) >> + { >> + printk(XENLOG_G_ERR "HVM%d restore: bad CR3 %#" PRIx64 "\n", >> + d->domain_id, ctxt.cr3); >> + return X86EMUL_EXCEPTION; > > -EINVAL, surely? Oh, indeed. Thanks for spotting. > Everything else LGTM (although I guess it depends on the previous > patch?) It does, yes. > so with this fixed, Reviewed-by: Andrew Cooper > <andrew.cooper3@citrix.com> Thanks. Jan
--- a/xen/arch/x86/hvm/hvm.c +++ b/xen/arch/x86/hvm/hvm.c @@ -1004,6 +1004,13 @@ static int hvm_load_cpu_ctxt(struct doma return -EINVAL; } + if ( ctxt.cr3 >> d->arch.cpuid->extd.maxphysaddr ) + { + printk(XENLOG_G_ERR "HVM%d restore: bad CR3 %#" PRIx64 "\n", + d->domain_id, ctxt.cr3); + return X86EMUL_EXCEPTION; + } + if ( (ctxt.flags & ~XEN_X86_FPU_INITIALISED) != 0 ) { gprintk(XENLOG_ERR, "bad flags value in CPU context: %#x\n", @@ -2290,10 +2297,18 @@ int hvm_set_cr0(unsigned long value, boo int hvm_set_cr3(unsigned long value, bool noflush, bool may_defer) { struct vcpu *v = current; + struct domain *currd = v->domain; struct page_info *page; unsigned long old = v->arch.hvm.guest_cr[3]; - if ( may_defer && unlikely(v->domain->arch.monitor.write_ctrlreg_enabled & + if ( value >> currd->arch.cpuid->extd.maxphysaddr ) + { + HVM_DBG_LOG(DBG_LEVEL_1, + "Attempt to set reserved CR3 bit(s): %lx", value); + return X86EMUL_EXCEPTION; + } + + if ( may_defer && unlikely(currd->arch.monitor.write_ctrlreg_enabled & monitor_ctrlreg_bitmask(VM_EVENT_X86_CR3)) ) { ASSERT(v->arch.vm_event); @@ -2309,13 +2324,12 @@ int hvm_set_cr3(unsigned long value, boo } } - if ( hvm_paging_enabled(v) && !paging_mode_hap(v->domain) && + if ( hvm_paging_enabled(v) && !paging_mode_hap(currd) && (value != v->arch.hvm.guest_cr[3]) ) { /* Shadow-mode CR3 change. Check PDBR and update refcounts. */ HVM_DBG_LOG(DBG_LEVEL_VMMU, "CR3 value = %lx", value); - page = get_page_from_gfn(v->domain, value >> PAGE_SHIFT, - NULL, P2M_ALLOC); + page = get_page_from_gfn(currd, value >> PAGE_SHIFT, NULL, P2M_ALLOC); if ( !page ) goto bad_cr3; @@ -2331,7 +2345,7 @@ int hvm_set_cr3(unsigned long value, boo bad_cr3: gdprintk(XENLOG_ERR, "Invalid CR3\n"); - domain_crash(v->domain); + domain_crash(currd); return X86EMUL_UNHANDLEABLE; }