Re: [Xen-devel] [PATCH v3 1/6] libxl: allow /local/domain/$LIBXL_TOOLSTACK_DOMID/device-model/$DOMID to be written by $DOMID

[Stefano Stabellini <stefano.stabellini@xxxxxxxxxxxxx>]

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).
> 
> 

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