>>> On 10.05.11 at 17:50, Shriram Rajagopalan <
rshriram@xxxxxxxxx> 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".