Re: [Xen-devel] QEMU bumping memory bug analysis

[Stefano Stabellini <stefano.stabellini@xxxxxxxxxxxxx>]

On Mon, 8 Jun 2015, Wei Liu wrote:
> On Mon, Jun 08, 2015 at 02:27:11PM +0100, Stefano Stabellini wrote:
> > On Mon, 8 Jun 2015, Wei Liu wrote:
> > > On Mon, Jun 08, 2015 at 12:39:52PM +0100, Stefano Stabellini wrote:
> > > [...]
> > > > > > > 3. Add a libxl layer that wraps necessary information, take over
> > > > > > >    Andrew's work on libxl migration v2.  Having a libxl layer 
> > > > > > > that's not
> > > > > > >    part of migration v2 is a waste of effort.
> > > > > > > 
> > > > > > > There are several obstacles for libxl migration v2 at the moment. 
> > > > > > > Libxl
> > > > > > > layer in migration v2 still has unresolved issues. It has
> > > > > > > inter-dependency with Remus / COLO.
> > > > > > > 
> > > > > > > Most importantly it doesn't inherently solve the problem. It still
> > > > > > > requires the current libxl JSON blob to contain information about 
> > > > > > > max
> > > > > > > pages (or information used to derive max pages).
> > > > > > > 
> > > > > > > Andrew, correct me if I'm wrong.
> > > > > > > 
> > > > > > > 4. Add a none user configurable field in current libxl JSON 
> > > > > > > structure to
> > > > > > >    record max pages information.
> > > > > > > 
> > > > > > > This is not desirable. All fields in libxl JSON should be user
> > > > > > > configurable.
> > > > > > > 
> > > > > > > 5. Add a user configurable field in current libxl JSON structure 
> > > > > > > to
> > > > > > >    record how much more memory this domain needs. Admin is 
> > > > > > > required to
> > > > > > >    fill in that value manually. In the mean time we revert the 
> > > > > > > change in
> > > > > > >    QEMU and declare QEMU with that change buggy.
> > > > > > 
> > > > > > QEMU 2.3.0 was released with that change in it, so it is not quite
> > > > > > possible to revert it. Also I think it is the right change for QEMU.
> > > > > > 
> > > > > 
> > > > > It has security implications. Here is my reply copied from my mail to
> > > > > Ian:
> > > > > 
> > > > > I'm considering removing xc_domain_setmaxmem needs regardless of this
> > > > > bug because that's going to cause problem in QEMU upstream stubdom 
> > > > > with
> > > > > strict XSM policy and deprivileged QEMU (may not have privilege to 
> > > > > call
> > > > > setmaxmem).
> > > > 
> > > > QEMU running in the stubdom should be able to set the maxmem for its
> > > > target domain, but not for the others.
> > > > 
> > > 
> > > Right, but this is still not safe. I will explain below.
> > > 
> > > > 
> > > > > The security implication as it is now is big enough. One malicious 
> > > > > guest
> > > > > that controls QEMU has a vector to DoS hypervisor by setting its own
> > > > > max_pages to -1;
> > > > 
> > > > Is that an issue in the implementation of XEN_DOMCTL_max_mem on the Xen
> > > > side?  Could you please give me a bit more info on this security issue?
> > > > 
> > > 
> > > Consider now we have a malicious guest. It has gained control of QEMU.
> > > It then calls xc_domain_setmaxmem to set it's own max_pages inside
> > > hypervisor to be -1 and start calling populate_physmap inside the guest
> > > kernel. This is going to make hypervisor OOM.
> > > 
> > > XEN_DOMCTL_max_mem only sets the limit of domain at the moment. I don't
> > > think there is sensible way of distinguishing a valid setmaxmem call
> > > from a malicious setmaxmem call from QEMU. It's untrusted code base
> > > after all.
> > 
> > Valid maxmem happens before device-model/$DOMID/state is set to running.
> > 
> 
> But Xen wouldn't know about that. It couldn't read xenstore.

The way I would do it would be to drop the max_mem write priviledge from
the stubdom at the time QEMU writes to device-mode/$domid/state, in a
similar way to QEMU in dom0 dropping from root to unprivileged uid.
Alternatively, as libxl is watching for changes to
device-model/$DOMID/state, libxl could be the one that takes maxmem
writing privileges away from the stubdom at that time.


> Also device-mode/$domid/state is writable by QEMU so we can't trust
> the content as indicator either.

We can because the write happens before we unpause the guest

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