Re: Is: Make XENVER_* use XSM, seperate the different ops in smaller security domains. Was:Re: [PATCH v1 5/5] xsplice: Use ld-embedded build-ids

[Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>]

On Fri, Sep 18, 2015 at 12:40:46PM +0100, Andrew Cooper wrote:
> On 17/09/15 19:45, Konrad Rzeszutek Wilk wrote:
> > . snip..
> >>>>>> 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.

That would introduce a performance regression I fear. Linux pvops does this:

                                                                              
/*                                                                              
 * Force a proper event-channel callback from Xen after clearing the            
 * callback mask. We do this in a very simple manner, by making a call          
 * down into Xen. The pending flag will be checked by Xen on return.            
 */                                                                             
void xen_force_evtchn_callback(void)                                            
{                                                                               
        (void)HYPERVISOR_xen_version(0, NULL);                                  
}                                          

quite often, which now will have to do the XSM check which is extra code.


I would say that the XENVER_compile_info (/sys/hypervisor/compilation),
XENVER_changeset (/sys/hypervisor/compilation) should go over
the XSM check.

While:XENVER_version, XENVER_extraversion,XENVER_capabilities,
XENVER_platform_parameters, XENVER_get_features,XENVER_pagesize

should have no XSM check.

> >> * 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.
> > Or we can just use 1024 bytes space the XENVER_* use.
> 
> What 1024 bytes?
> 
> Each subop currently assumes the guest handle is a pointer to an
> appropriately typed structure, which puts arbitrary and unnecessary
> length restrictions on items.
> 
> ~Andrew
> 
> >
> >> * 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.
> > CC-ing Daniel. Changing title.
> >> ~Andrew
>