Re: [Xen-devel] [PATCH v3 1/4] xen/libxc: Allow changes to hypervisor CPUID leaf from config file

[Boris Ostrovsky <boris.ostrovsky@xxxxxxxxxx>]

On 03/14/2014 04:09 AM, Jan Beulich wrote:
> > > > On 13.03.14 at 19:08, Boris Ostrovsky <boris.ostrovsky@xxxxxxxxxx> wrote:
> > @@ -43,22 +45,29 @@ static int hypervisor_is_64bit(xc_interface *xch)
> >   static void cpuid(const unsigned int *input, unsigned int *regs)
> >   {
> >       unsigned int count = (input[1] == XEN_CPUID_INPUT_UNUSED) ? 0 : input[1];
> > +    uint8_t is_hyp = IS_HYPERVISOR_LEAF(input[0]);
> >   #ifdef __i386__
> >       /* Use the stack to avoid reg constraint failures with some gcc flags */
> >       asm (
> >           "push %%ebx; push %%edx\n\t"
> > -        "cpuid\n\t"
> > +        "testb $0xff,%5\n\t"
> > +        "jz 1f\n\t"
> > +        XEN_EMULATE_PREFIX
> > +        "1: cpuid\n\t"
> >           "mov %%ebx,4(%4)\n\t"
> >           "mov %%edx,12(%4)\n\t"
> >           "pop %%edx; pop %%ebx\n\t"
> >           : "=a" (regs[0]), "=c" (regs[2])
> > -        : "0" (input[0]), "1" (count), "S" (regs)
> > +        : "0" (input[0]), "1" (count), "S" (regs), "m" (is_hyp)
> All inputs must be in registers here, since memory references might
> use %esp and hence be off by 2 stack slots due to the pushes/pops
> surrounding the actual operation. Since you evaluate the flag prior
> to the CPUID, using "db" as constraint would seem possible here.
gcc for some reason rejects "b" as inconsistent (but "d" works fine).


> > @@ -555,6 +564,15 @@ static int xc_cpuid_policy(
> >   {
> >       xc_dominfo_t        info;
> >   
> > +    if ( IS_HYPERVISOR_LEAF(input[0]) )
> > +    {
> > +        /* Only leaf 1 can be modified */
> > +        if ( input[0] == 0x40000000 )
> > +            return 0;
> > +        else
> > +            return -EACCES;
> And I'm still worried about altering this in uncontrolled ways: No
> good can result from allowing ecx/edx/ebx to be modified, and
> improperly modifying the eax value (namely putting in place
> wrong upper bits, the more that those aren't statically
> determined) won't help much either. IOW it should really only
> be the low 8 bits of eax that can be overridden, and it shouldn't
> be possible to clear these 8 bits.
I'll add mask array to xc_cpuid_policy (and probably other policy
routines that it calls) that will specify bits that are permitted to be
overwritten.

-boris


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel