Re: [PATCH v1 5/5] xsplice: Use ld-embedded build-ids

[Andrew Cooper <andrew.cooper3@citrix.com>]

On 17/09/15 07:41, Martin Pohlack wrote:
> On 17.09.2015 00:31, Andrew Cooper wrote:
>> On 16/09/2015 22:59, Konrad Rzeszutek Wilk wrote:
>>> On September 16, 2015 5:41:26 PM EDT, Andrew Cooper <andrew.cooper3@citrix.com> wrote:
>>>> On 16/09/2015 22:01, Konrad Rzeszutek Wilk wrote:
>>>>> From: Martin Pohlack <mpohlack@amazon.de>
>>>>>
>>>>> The mechanism to get this is via the XSPLICE_OP and
>>>>> we add a new subsequent hypercall to retrieve the
>>>>> binary build-id. The hypercall allows an arbirarty
>>>>> size (the buffer is provided to the hypervisor) - however
>>>>> by default the toolstack will allocate it up to 128
>>>>> bytes.
>>>>>
>>>>> We also add two places for the build-id to be printed:
>>>>>  - xsplice keyhandler. We cannot use 'hh' in the hypervisor
>>>>>    snprintf handler (as it is not implemented) so instead
>>>>>    we use an simpler way to print it.
>>>>>  - In the 'xen-xsplice' tool add an extra parameter - build-id
>>>>>    to print this as an human readable value.
>>>>>
>>>>> Note that one can also retrieve the value by 'readelf -h xen-syms'.
>>>>>
>>>>> Signed-off-by: Martin Pohlack <mpohlack@amazon.de>
>>>>> Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
>>>>> ---
>>>>>  tools/libxc/include/xenctrl.h |  1 +
>>>>>  tools/libxc/xc_misc.c         | 26 +++++++++++++
>>>>>  tools/misc/xen-xsplice.c      | 39 ++++++++++++++++++++
>>>>>  xen/arch/x86/Makefile         |  4 +-
>>>>>  xen/arch/x86/xen.lds.S        |  5 +++
>>>>>  xen/common/xsplice.c          | 86
>>>> +++++++++++++++++++++++++++++++++++++++++++
>>>>>  xen/include/public/sysctl.h   | 18 +++++++++
>>>>>  xen/include/xen/version.h     |  1 +
>>>>>  8 files changed, 178 insertions(+), 2 deletions(-)
>>>>>
>>>>> diff --git a/tools/libxc/include/xenctrl.h
>>>> b/tools/libxc/include/xenctrl.h
>>>>> index 2cd982d..946ddc0 100644
>>>>> --- a/tools/libxc/include/xenctrl.h
>>>>> +++ b/tools/libxc/include/xenctrl.h
>>>>> @@ -2860,6 +2860,7 @@ int xc_xsplice_apply(xc_interface *xch, char
>>>> *id);
>>>>>  int xc_xsplice_revert(xc_interface *xch, char *id);
>>>>>  int xc_xsplice_unload(xc_interface *xch, char *id);
>>>>>  int xc_xsplice_check(xc_interface *xch, char *id);
>>>>> +int xc_xsplice_build_id(xc_interface *xch, char *build_id, unsigned
>>>> int max);
>>>>
>>>> The build id of the current running hypervisor should belong in the
>>>> xeninfo hypercall.  It is not specific to xsplice.
>>> However in the previous reviews it was pointed out that it should only be accessible to dom0.
>>>
>>> Or to any domains as long as the XSM allows (and is turned on) - so not the default dummy one.
>>>
>>> That is a bit of 'if' extra complexity which I am not sure is worth it?
>> DomU can already read the build information such as changeset, compile
>> time, etc.  Build-id is no more special or revealing.
> I would take this as an argument *against* giving DomU access to those
> pieces of information in details and not as an argument for
> *additionally* giving it access to build-id.
>
> With build-id we have the chance to shape a not-yet-established API and
> I would like to follow the Principle of least privilege wherever it
> makes sense.
>
> To reach a similar security level with the existing API, it might make
> sense to limit DomU access to compile date, compile time, compiled by,
> compiled domain, compiler version and command line details, xen extra
> version, and xen changeset.  Basically anything that might help DomUs to
> uniquely identify a Xen build.
>
> The approach can not directly drop access to those fields, as that would
> break an existing API, but it could restrict the detail level handed out
> to DomU.

These are all valid arguments to be made, but please lets fix the issue
properly rather than hacking build-id on the side of an unrelated component.

>From my point of view, the correct course of action is this:

* Split the current XSM model to contain separate attributes for general
and privileged information.
** For current compatibility, all existing XENVER_* subops fall into general
* Apply an XSM check at the very start of the hypercall.
* Extend do_xen_version() to take 3 parameters.  (It is curious that it
didn't take a length parameter before)
** This is still ABI compatible, as existing subops simply ignore the
parameter.
* Introduce new XENVER_build_id subop which is documented to require the
3-parameter version of the hypercall.
** This subop falls into straight into privileged information.

This will introduce build-id in its correct location, with appropriate
restrictions.

Moving forwards, we really should have an audit of the existing XENVER_*
subops.  Some are clearly safe/required for domU to read, but some such
as XENVER_commandline have no business being accessible.  A separate
argument, from the repeatable build point of view, says that compilation
information isn't useful at all.

Depending on how we wish to fix the issue, we could either do a blanket
move of the subops into the privileged XSM catagory, or introduce a 3rd
"legacy privileged" category to allow the admin to control access on a
per-vm basis.

~Andrew