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

[Ian Campbell <ian.campbell@citrix.com>]

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.

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