From mboxrd@z Thu Jan 1 00:00:00 1970 From: Wei Liu Subject: Re: QEMU bumping memory bug analysis Date: Mon, 8 Jun 2015 14:32:20 +0100 Message-ID: <20150608133220.GE29102@zion.uk.xensource.com> References: <20150605164354.GK29102@zion.uk.xensource.com> <20150605181014.GM29102@zion.uk.xensource.com> <20150608131000.GC29102@zion.uk.xensource.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Content-Disposition: inline In-Reply-To: List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Stefano Stabellini Cc: Wei Liu , Ian Campbell , George Dunlap , Andrew Cooper , Ian Jackson , dslutz@verizon.com, xen-devel@lists.xen.org List-Id: xen-devel@lists.xenproject.org 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. Also device-mode/$domid/state is writable by QEMU so we can't trust the content as indicator either. Wei.