Re: [PATCH 2/2] xen/xsm: Add new SILO mode for XSM

["Xin Li (Talons)" <xin.li@citrix.com>]

Hello Jan,

> -----Original Message-----
> From: Jan Beulich [mailto:JBeulich@suse.com]
> Sent: Monday, July 2, 2018 3:29 PM
> To: Xin Li (Talons) <xin.li@citrix.com>; Xin Li <talons.lee@gmail.com>
> Cc: Andrew Cooper <Andrew.Cooper3@citrix.com>; George Dunlap
> <George.Dunlap@citrix.com>; Ming Lu <ming.lu@citrix.com>; Sergey Dyasli
> <sergey.dyasli@citrix.com>; Wei Liu <wei.liu2@citrix.com>; Stefano Stabellini
> <sstabellini@kernel.org>; xen-devel@lists.xen.org; Konrad Rzeszutek Wilk
> <konrad.wilk@oracle.com>; Daniel de Graaf <dgdegra@tycho.nsa.gov>; Tim
> (Xen.org) <tim@xen.org>
> Subject: RE: [PATCH 2/2] xen/xsm: Add new SILO mode for XSM
> 
> >>> On 02.07.18 at 08:57, <xin.li@citrix.com> wrote:
> >> From: Jan Beulich [mailto:JBeulich@suse.com]
> >> Sent: Friday, June 29, 2018 6:36 PM
> >> >>> On 29.06.18 at 11:28, <talons.lee@gmail.com> wrote:
> >> > When SILO is enabled, there would be no page-sharing between
> >> > unprivileged VMs (no grant tables or event channels).
> >>
> >> What is the relation between page sharing and event channels?
> >
> > They are the two mechanisms exist for inter-domain communication, And
> > we want to block them in SILO mode.
> 
> I understand this, but it doesn't answer my question. I agree that grant tables
> are a means to share pages, but the wording looks odd to me wrt event
> channels.
Do you mean add " or event notifications",
When SILO is enabled, there would be no page-sharing or event notifications
between unprivileged VMs (no grant tables or event channels).

> 
> >> > --- a/xen/common/Kconfig
> >> > +++ b/xen/common/Kconfig
> >> > @@ -143,6 +143,17 @@ config XSM_FLASK_POLICY
> >> >
> >> >  	  If unsure, say Y.
> >> >
> >> > +config XSM_SILO
> >> > +	def_bool y
> >> > +	prompt "SILO support"
> >> > +	depends on XSM
> >> > +	---help---
> >> > +	  Enables SILO as the access control mechanism used by the XSM
> >> framework.
> >> > +	  This will deny any unmediated communication channels between
> >> unprivileged
> >> > +	  VMs.
> >> > +
> >> > +	  If unsure, say Y.
> >>
> >> It would be helpful to clarify here that this is not the default mode of
> operation.
> >> In fact, another Kconfig (choice) might be useful to have to select
> >> the built-in default. In fact "deny any" suggests that this is what
> >> is going to happen regardless of command line options. At the very
> >> least I think this wants to be "This will allow to deny any ..." or "In this mode,
> any ... will by denied".
> >>
> >> Andrew, the chosen name here may underline the relevance of my
> >> comment regarding XSM_FLASK vs just FLASK, albeit things are
> >> unclear/ambiguous if I also take into account the code further down.
> >> The descriptions above make it sound as if this was an override to
> >> whatever access control mechanism was in place (dummy or flask
> >> currently). Code below suggests though that this is meant to be a
> >> clone of dummy, with just some minimal adjustments. I guess it's
> >> rather the description that needs adjustment, but the alternative of
> >> being a global override even in FLASK mode certainly exists.
> >>
> >> Furthermore it is unclear here what an "unmediated communication
> channel"
> >> is, and what "mediated communication channels" (if any) are still
> >> available in this new mode.
> >
> > Change to:
> >
> > config XSM_SILO
> >>-------def_bool y
> >>-------prompt "SILO support"
> >>-------depends on XSM
> >>----------help---
> >>-------  Enables SILO as the access control mechanism used by the XSM
> framework.
> >>-------  This is not the default module, add boot parameter xsm=silo
> >>to choose
> >>-------  it. This will deny any unmediated communication channels
> >>(grant tables
> >>-------  and event channels) between unprivileged VMs.
> 
> With s/module/mode/ this is an improvement, but continues to leave open in
> particular what an "unmediated communication channel" is.
This can't prevent side-channel attack.

> 
> Btw, thinking about it again - do we need a Kconfig option here in the first
> place, when the mode isn't the default, and it's not a whole lot of code that gets
> added?
The existing XSM code use Kconfig,
I just want to follow the similar style for new module.
And yes, we can handle it in CONFIG_XSM like dummy.
Which way is better?

> 
> >> > +static bool silo_mode_dom_check(domid_t ldom, domid_t rdom) {
> >> > +    domid_t hd_dom = hardware_domain->domain_id;
> >>
> >> I don't think you mean the hardware domain here, but the control
> >> domain (of which in theory there may be multiple).
> >
> > I mean the one and only dom0.
> 
> No, for the purpose here you don't mean Dom0, which just happens to be both
> hardware and (the only) control domain in most setups. From a security pov
> though you need to distinguish all of these.

Yes! thanks.
I will use 
is_control_domain(d)
instead of comparing the hardware domain id.

This comment is misleading then:
/* Is this guest fully privileged (aka dom0)? */
   bool             is_privileged;

> >> > +    domid_t cur_dom = current->domain->domain_id;
> >> > +
> >> > +    if ( ldom == DOMID_SELF )
> >> > +        ldom = cur_dom;
> >> > +    if ( rdom == DOMID_SELF )
> >> > +        rdom = cur_dom;
> >> > +
> >> > +    return (hd_dom == cur_dom || hd_dom == ldom || hd_dom == rdom
> ||
> >> > +            ldom == rdom);
> >> > +}
> >> > +
> >> > +static int silo_evtchn_unbound(struct domain *d1, struct evtchn *chn,
> >> > +                               domid_t id2) {
> >> > +    if ( silo_mode_dom_check(d1->domain_id, id2) )
> >> > +        return dummy_xsm_ops.evtchn_unbound(d1, chn, id2);
> >>
> >> Urgh. Why is this not xsm_evtchn_unbound() from dummy.h? It would be
> >> really nice to avoid such extra indirect calls here.
> >
> > This makes it clearer that we are calling the counterpart of dummy
> > ops(overriding).
> 
> Yes, but the same level of clarity could be achieved when naming the function
> in dummy.h dummy_evtchn_unbound() (aliased to
> xsm_evtchn_unbound() for satisfying needs elsewhere).
> 
> > This indirect calls should not introduce any runtime penalty.
> 
> How does it not, when indirect calls are more expensive than direct ones
> already without the Spectre v2 mitigations?
I only mean it's not runtime binding.

And I ran some performance test before, seems no performance penalty.

The names in dummy.h are the same as xsm.h.
If I call xsm_evtchn_unbound, that's from xsm.h,
It probably call silo_evtchn_unbound, ends up in a loop.

So I may have to rename all the functions in dummy.h,
And remove static...

is it necessary?

> 
> >> Furthermore, this hook is called in two contexts. Is the above really
> > appropriate
> >> also in the alloc_unbound_xen_event_channel() case?
> >>
> >> > +static int silo_grant_mapref(struct domain *d1, struct domain *d2,
> >> > +                             uint32_t flags) {
> >> > +    if ( silo_mode_dom_check(d1->domain_id, d2->domain_id) )
> >> > +        return dummy_xsm_ops.grant_mapref(d1, d2, flags);
> >> > +    return -EPERM;
> >> > +}
> >>
> >> What about the unmap counterpart?
> >
> > This is unnecessary, since we are blocking it from setting up, Those
> > calling unmap must pass the check of maping.
> 
> Taking that position, the unmap XSM hook's existence is questionable
> altogether.

I think that's for completeness.
We can override it only when necessary.
But this change don't have to include that part.

> 
> Jan


Best regards

Xin(Talons) Li

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel