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

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

Hi
    I think the main questions here are:
    1. Do we need a separated KConfig option for SILO
    2. Can we use indirect call like "dummy_xsm_ops.grant_copy"
    Any suggestion?

Best regards

Xin(Talons) Li

> -----Original Message-----
> From: Jan Beulich [mailto:JBeulich@xxxxxxxx]
> Sent: Monday, July 2, 2018 5:39 PM
> To: Xin Li (Talons) <xin.li@xxxxxxxxxx>; Xin Li <talons.lee@xxxxxxxxx>
> Cc: Andrew Cooper <Andrew.Cooper3@xxxxxxxxxx>; George Dunlap
> <George.Dunlap@xxxxxxxxxx>; Ming Lu <ming.lu@xxxxxxxxxx>; Sergey Dyasli
> <sergey.dyasli@xxxxxxxxxx>; Wei Liu <wei.liu2@xxxxxxxxxx>; Stefano Stabellini
> <sstabellini@xxxxxxxxxx>; xen-devel@xxxxxxxxxxxxx; Konrad Rzeszutek Wilk
> <konrad.wilk@xxxxxxxxxx>; Daniel de Graaf <dgdegra@xxxxxxxxxxxxx>; Tim
> (Xen.org) <tim@xxxxxxx>
> Subject: RE: [PATCH 2/2] xen/xsm: Add new SILO mode for XSM
> 
> >>> On 02.07.18 at 11:22, <xin.li@xxxxxxxxxx> wrote:
> >> From: Jan Beulich [mailto:JBeulich@xxxxxxxx]
> >> Sent: Monday, July 2, 2018 3:29 PM
> >> >>> On 02.07.18 at 08:57, <xin.li@xxxxxxxxxx> wrote:
> >> >> From: Jan Beulich [mailto:JBeulich@xxxxxxxx]
> >> >> Sent: Friday, June 29, 2018 6:36 PM
> >> >> >>> On 29.06.18 at 11:28, <talons.lee@xxxxxxxxx> 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).
> 
> Yes, that's one way to clarify things.
> 
> >> > 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?
> 
> Daniel, Andrew?
> 
> >> >> > +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;
> 
> Yes, but it's likely going to remain that way until further disaggregation 
> work
> would happen.
> 
> >> >> > +    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.
> 
> Sure, these paths aren't normally performance critical. But by doing what you
> do we'd have a bad precedent, and if someone later cloned your solution into
> something that does sit on a performance critical path, we'd have a problem.
> 
> > 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,
> 
> Note how I've said "naming the function in dummy.h
> dummy_evtchn_unbound() (aliased to xsm_evtchn_unbound() for satisfying
> needs elsewhere)."
> 
> > And remove static...
> 
> I don't think so, no.
> 
> > is it necessary?
> 
> It should imo be at least considered. But Daniel as the maintainer may have
> something to say here as well...
> 
> Jan


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