From mboxrd@z Thu Jan 1 00:00:00 1970 From: Stefano Stabellini Subject: Re: [PATCH v3 1/6] libxl: allow /local/domain/$LIBXL_TOOLSTACK_DOMID/device-model/$DOMID to be written by $DOMID Date: Tue, 30 Jun 2015 16:00:29 +0100 Message-ID: References: <1433930994-32527-1-git-send-email-stefano.stabellini@eu.citrix.com> <1435248986.32500.110.camel@citrix.com> <1435654179.21469.17.camel@citrix.com> <1435673075.21469.183.camel@citrix.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <1435673075.21469.183.camel@citrix.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Ian Campbell Cc: wei.liu2@citrix.com, xen-devel@lists.xensource.com, Ian.Jackson@eu.citrix.com, Stefano Stabellini List-Id: xen-devel@lists.xenproject.org On Tue, 30 Jun 2015, Ian Campbell wrote: > On Tue, 2015-06-30 at 14:49 +0100, Stefano Stabellini wrote: > > > > The reason why I feel comfortable having such an high limit compared to > > > > the actual usage, is that it is only meant to prevent the guest from > > > > filling up dom0's disk, as this information is saved to file. As > > > > xenstore entries have a size limit themselves, we should be pretty safe. > > > > > > I'm not bothered by how high or low the limit is, just the mechanisms > > > which we intend to use in order to cope when it changes, which are much > > > easier to put in place now than trying to retrofit such a thing while > > > retaining support for existing released versions of qemu. > > > > > > > > There is an interesting general issue here which is that we have > > > > > XS_RESTRICT which changes a connection to be treated as having the > > > > > permissions of a given target domain instead of the originating > > > > > privileged domain. > > > > > > > > > > This means that in order to reduce the privileges of one thing we have > > > > > to increase the privilege of the guest itself (by granting access to > > > > > those paths), which seems rather counter-intuitive. > > > > > > > > > > What we really want is a new privilege type which is "read/write to > > > > > connections which are _privileged_ over $domid, but not $domid itself" > > > > > and for XS_RESTRICT to imply that. > > > > > > > > > > Retrofitting something like that to xenstored would be tricky I suspect. > > > > > > > > > > When the physmap stuff was added doing it via xenstore was convenient > > > > > because we weren't concerning ourselves with this deprivileging. How > > > > > that we are though perhaps we should think about whether this is still > > > > > appropriate and consider using a QMP command to request the list instead > > > > > for example. > > > > > > > > That is true, however this doesn't mean we cannot also add a QMP command > > > > in the future too, especially if we follow Wei's suggestion and we only > > > > change the permissions if xs_restrict. We could also implemented a > > > > different XS_RESTRICT. In other words, I don't think we have to come up > > > > with the best possible solution right now, we just need to improve over > > > > what we already have. I think that this is an improvement. > > > > > > It's not a strict improvement though, it trading off giving the guest > > > access to stuff which it never previously had a need to and which the > > > toolstack consumes. > > > > I understand that strictly speaking what you wrote is true, however I > > would like to point out that the information under device-model on > > xenstore is already exposed to the guest by other means (pci config > > space, etc.). > > It's the ability of the guest to change this info into fakery that the > restorer will consume which is new here and possibly dangerous. Has the > reader of this data (both on the save side moving it into the stream and > on the restore side taking it back out) been audited for safety now that > this data is untrusted? When I made this change, I gave a careful look both at the libxl side and the QEMU side. Indeed I would appreciate a second pair of eyes on this. These are my observations: tools/libxl/libxl_dom.c:libxl__toolstack_save is the crux. It calls: - libxl__xs_directory on /physmap this is safe - libxl__xs_read if the path is wrong (nothing is there), it returns NULL, and it is handled correctly - strtoll on the values read The values are under guest control but strtoll can handle bad formats. strtoll always returns an unsigned long long. In case of errors, it can return LLONG_MIN or LLONG_MAX. libxl__toolstack_save doesn't check for conversion errors, but it never uses the returned values anywhere. That's OK. tools/libxl/libxl_dom.c:libxl__toolstack_restore writes back the values to xenstore. So in case of errors: 1) libxl__toolstack_save could return early with an error, if the xenstore paths are wrong (nothing is on xenstore) 2) libxl__toolstack_restore could write back to xenstore any unsigned long long, including LLONG_MIN or LLONG_MAX Either way we are fine. >>From the QEMU side, the code is fairly similar and uses strtoll to parse the entries. The values are used to avoid memory allocations at restore time for memory that has already been allocated (video memory). If the values are wrong, QEMU will attempt another memory allocation, failing because the maxmem limit has been reached (the infamous maxmem increase commit has been reverted).