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

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

 Hi Daniel,

     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@suse.com]
> > Sent: Monday, July 2, 2018 5:39 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 11:22, <xin.li@citrix.com> wrote:
> > >> From: Jan Beulich [mailto:JBeulich@suse.com]
> > >> Sent: Monday, July 2, 2018 3:29 PM
> > >> >>> 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).
> >
> > 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@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel