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: Thu, 23 Jul 2015 18:13:40 +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> <1435934231.9447.140.camel@citrix.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <1435934231.9447.140.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 Fri, 3 Jul 2015, Ian Campbell wrote: > On Tue, 2015-06-30 at 16:00 +0100, Stefano Stabellini wrote: > > 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: > > I think the request for secondary close review and more importantly > these observations (which == a rationale about why this is safe) deserve > top billing in the commit message. > > IMO any commit which deliberately relaxes some security cordon should > have as much information and analysis as possible about why this is OK > to do prominently in the logs. Both so that we can tell that due > diligence has been done and so that we can look back in six months and > see what we thought the security properties of this change were. You are right. I write all this down. > > 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). > >