Re: [PATCH v3 4/4] x86/PV: enable the emulated PIT

[Ian Campbell <ian.campbell@citrix.com>]

On Tue, 2016-01-19 at 10:09 +0000, Andrew Cooper wrote:
> On 19/01/16 09:24, Ian Campbell wrote:
> > On Mon, 2016-01-18 at 18:03 +0000, Andrew Cooper wrote:
> > > On 18/01/16 17:58, Roger Pau Monné wrote:
> > > > El 18/01/16 a les 11.41, Andrew Cooper ha escrit:
> > > > > On 18/01/16 09:44, Jan Beulich wrote:
> > > > > > > > > On 18.01.16 at 10:29, <andrew.cooper3@citrix.com> wrote:
> > > > > > > On 18/01/2016 07:43, Jan Beulich wrote:
> > > > > > > > > > > On 15.01.16 at 18:45, <roger.pau@citrix.com> wrote:
> > > > > > > > > Changes since v2:
> > > > > > > > >  - Change 'if ( (a && b) || (!a && c) )' into 'if ( a ? b
> > > > > > > > > : c
> > > > > > > > > )'.
> > > > > > > > Thanks, but after some more thinking about it I'm afraid
> > > > > > > > there
> > > > > > > > are
> > > > > > > > a few more aspects to consider here:
> > > > > > > > 
> > > > > > > > > --- a/xen/arch/x86/domain.c
> > > > > > > > > +++ b/xen/arch/x86/domain.c
> > > > > > > > > @@ -542,8 +542,9 @@ int arch_domain_create(struct domain
> > > > > > > > > *d,
> > > > > > > > > unsigned int 
> > > > > > > domcr_flags,
> > > > > > > > >                     d->domain_id, config-
> > > > > > > > > >emulation_flags);
> > > > > > > > >              return -EINVAL;
> > > > > > > > >          }
> > > > > > > > > -        if ( config->emulation_flags != 0 &&
> > > > > > > > > -             (!is_hvm_domain(d) || config-
> > > > > > > > > >emulation_flags
> > > > > > > > > != XEN_X86_EMU_ALL) 
> > > > > > > )
> > > > > > > > > +        if ( is_hvm_domain(d) ? (config->emulation_flags 
> > > > > > > > > !=
> > > > > > > > > XEN_X86_EMU_ALL &&
> > > > > > > > > +             config->emulation_flags != 0) :
> > > > > > > > > +             (config->emulation_flags !=
> > > > > > > > > XEN_X86_EMU_PIT) )
> > > > > > > > >          {
> > > > > > > > For one I think it would be a good idea to allow zero for
> > > > > > > > PV
> > > > > > > > domains,
> > > > > > > > and perhaps even default new DomU-s to have the PIT flag
> > > > > > > > clear.
> > > > > > > > (Also - indentation.)
> > > > > > > > 
> > > > > > > > Which gets us to the second, broader issue: These flags
> > > > > > > > shouldn't
> > > > > > > > be forced to a particular value during migration, but
> > > > > > > > instead
> > > > > > > > they
> > > > > > > > should be part of the state getting migrated. Incoming
> > > > > > > > domains
> > > > > > > > then would - if the field is missing due to coming from an
> > > > > > > > older
> > > > > > > > hypervisor - have the flag default to 1.
> > > > > > > There is sadly another ratsnest here.
> > > > > > I've been afraid of that.
> > > > > > 
> > > > > > > These values are needed for domain creation, which means that
> > > > > > > putting
> > > > > > > them anywhere in the migration stream is already too late, as
> > > > > > > the
> > > > > > > domain
> > > > > > > has been created before the stream header is read.
> > > > > > Is that an inherent requirement, or just a result of current
> > > > > > code
> > > > > > structure?
> > > > > Depends.  As far as libxc/libxl migration levels go, current code
> > > > > structure.
> > > > > 
> > > > > Whatever (eventually) gets used to set these values will however
> > > > > be
> > > > > present in the xl configuration, which is at the very start of
> > > > > the
> > > > > stream, and is what is used to create the new domain.
> > > > > 
> > > > > We really don't want the libxc migrate code to be making the
> > > > > DOMCTL_createdomain hypercall itself; it opens up a whole new
> > > > > attack
> > > > > surface via cunningly-crafted save image.  The best we can do is
> > > > > have
> > > > > a
> > > > > sanity check later on.
> > > > > 
> > > > > >  I ask because migrating the emulation flags is going to
> > > > > > be a requirement for relaxing the current (almost) all-or-
> > > > > > nothing
> > > > > > policy on those flags.
> > > > > > 
> > > > > > > In principle, the best which could occur is that a value gets
> > > > > > > stashed in
> > > > > > > the stream and used as a sanity check.  That will at least
> > > > > > > catch
> > > > > > > the
> > > > > > > case when they are different.
> > > > > > That'd be a minimal first step.
> > > > > This is a substantial quantity of work to do properly.  As the
> > > > > emulation
> > > > > flags are just one in a very long list of fields handed like
> > > > > this, I
> > > > > don't think this issue should block the series.
> > > > You certainly are more familiar with the migration code than me,
> > > > but
> > > > wouldn't it be enough to add a new field to libxl_domain_build_info
> > > > (uint32_t emulation_flags), and teach
> > > > libxl_domain_build_info_gen_json/libxl__domain_build_info_parse_jso
> > > > n
> > > >  how to properly parse it?
> > > That would let it be configured from an xl.cfg file, and would
> > > normally
> > > be moved in the migration stream.  However, there is a specific
> > > option
> > > in xl to restore but using a brand new configuration file.
> > > 
> > > What it doesn't do it check that the settings for the domain in the
> > > stream match the settings of the domid being restored into.
> > That would be the responsibility of the user who has chosen to override
> > the
> > configuration in this way.
> 
> It is the responsibility of Xen to ensure there are no exploitable holes
> due to partial or misconfiguration.

Indeed, but it only needs to check things and fail, not work in the face of
a bogus save file + cfg file configuration. Perhaps I misunderstood what
was being contended here.

Ian.

> In particular, this PIT emulation patch fixes an accidental NULL pointer
> dereference in Xen, due to the accidental disabling of the PIT in PV
> guests.
> 
> ~Andrew

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