Re: [Xen-devel] 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

[Daniel De Graaf <dgdegra@xxxxxxxxxxxxx>]

On 22/09/15 09:45, Konrad Rzeszutek Wilk wrote:
> On Tue, Sep 22, 2015 at 02:33:23PM +0100, Andrew Cooper wrote:
> > On 22/09/15 14:22, Konrad Rzeszutek Wilk wrote:
> > > 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.
> > The XSM check will fall into the noise, performance wise, compared to the
> > context switch to make the hypercall in the first place.  It is just another
> > switch statement.  Also, selectively applying XSM checks will incur even
> > more overhead than doing a blanket XSM check.
I would be surprised if placing the XSM check inside the individual case
statement is more expensive than a single XSM check outside the switch:
in either case, it's either a single call or a single if statement, and
since there's only 10 cases (6 of which were identified as not needing a
check) it's not much code added either.

You could also define a bitmask of operations that require an XSM check
and do "if ((1 << cmd) & NEEDS_XSM_CHECK_BITMASK) { do_xsm_check() }".

> I am worried about some spinlock in the depths of XSM code.
>
> But then I haven't looked in detail so perhaps this is not an issue after all.
The XSM code tries to use read locks where possible to avoid this type of
performance hit, and if the call is being made as often as it sounds like,
the check will almost always hit the AVC cache (avoiding the more expensive
parts of the FLASK security server).  However, it's always faster to return
success right away (and no less secure if you were going to do so anyway).

> > Also, I really don't care if you can measure a performance hit (not that I
> > reckon you could).  How Linux chooses to behave itself has absolutely no
> > bearing on how we go about securing the hypercall.
> But making something slower is surely not something we strive for.
>
> > ~Andrew

--
Daniel De Graaf
National Security Agency

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