On Tue, 2011-05-10 at 16:50 +0100, Shriram Rajagopalan wrote:
> On Tue, May 10, 2011 at 10:02 AM, Jan Beulich <JBeulich@xxxxxxxxxx>
> wrote:
> >>> On 10.05.11 at 16:52, Shriram Rajagopalan
> <rshriram@xxxxxxxxx> wrote:
> > On Tue, May 10, 2011 at 3:41 AM, Ian Campbell
> <Ian.Campbell@xxxxxxxxxx>wrote:
>
>
> >> The most plausible looking EOPNOTSUPP from that code is in
> >> xen/arch/x86/domain.c:arch_set_info_guest() but that is on
> a PV only
> >> path.
> >>
> >> And that rings with the pv guests I am using. It makes
> perfect sense,
> > looking
> > at that function and especially at the code that returns
> EOPNOTSUPP (the
> > only
> > place in the entire file).
> > else
> > {
> > bool_t fail = v->arch.pv_vcpu.ctrlreg[3] !=
> c(ctrlreg[3]);
> >
> > #ifdef CONFIG_X86_64
> > fail |= v->arch.pv_vcpu.ctrlreg[1] != c(ctrlreg[1]);
> > #endif
> >
> > for ( i = 0; i <
> ARRAY_SIZE(v->arch.pv_vcpu.gdt_frames); ++i )
> > fail |= v->arch.pv_vcpu.gdt_frames[i] !=
> c(gdt_frames[i]);
> > fail |= v->arch.pv_vcpu.gdt_ents != c(gdt_ents);
> >
> > fail |= v->arch.pv_vcpu.ldt_base != c(ldt_base);
> > fail |= v->arch.pv_vcpu.ldt_ents != c(ldt_ents);
> >
> > if ( fail )
> > return -EOPNOTSUPP;
> > }
> >
> > This change was introduced by c/s
> > changeset: 23142:f5e8d152a565
> > user: Jan Beulich <jbeulich@xxxxxxxxxx>
> > date: Tue Apr 05 13:01:25 2011 +0100
> > x86: split struct vcpu
> >
> > I think I am missing something really obvious in this piece
> of code. The
> > xc_domain_resume code tries to modify the return value of
> shutdown hypercall
> > (i.e eax register is set to 1) and this code doesnt seem to
> check those
> > registers.
>
>
> Correct - the code here checks only for values where the logic
> needed to support changing the on an already initialized vCPU
> isn't
> implemented. Previously, actual vCPU state and what was
> tracked
> in struct vcpu could get out of sync in this case, potentially
> confusing things further down (including possible security
> issues).
>
> You'll want to figure out which part(s) actually differ, and
> why.
> Only then we'll be able to tell whether mentioned c/s
> introduced
> false positives.
>
> Bit confused. If I understand correctly, this piece of code checks new
> values
> of certain registers against old ones, for an already initialized
> VCPU. And AFAIT,
> it is checking the gdts, ldts & control registers. The
> xc_domain_resume code just
> changes one general purpose register eax. basically,
>
> get_vcpucontext()
> set_field(eax, 1) //to indicate SUSPEND_CANCEL
> set_vcpucontext()
>
> I dont understand what you mean by "which parts actually differ &
> why".
Either get_vcpucontext is returning something "odd" or set_vcpucontext
is being overzealous in what it checks (the "false positives" Jan refers
too)
> And just a trivial question:
> is the hypervisor binary always compiled to a 32-bit elf? somehow, the
> symbols file xen-syms-* is getting compiled to 64 bit ELF binary while
> the xen binary is getting compiled to 32-bit binary.
The hypervisor binary (32 or 64 bit) is always wrapped up as a 32 bit
ELF, see the use of xen/arch/x86/boot/mkelf32.c. Something to do with
keeping a bootloader happy, I think...
Ian.
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel
|
Home