Andres Lagar-Cavilla
2012-Sep-12 16:20 UTC
Re: [PATCH 18/22] arch/x86: Add missing mem_sharing XSM hooks
> This patch adds splits up the mem_sharing and mem_event XSM hooks to > better cover what the code is doing. It also completes the support for > arch-specific domctls in FLASK. > > Signed-off-by: Daniel De Graaf <dgdegra@tycho.nsa.gov> > Cc: Keir Fraser <keir@xen.org> > Cc: Jan Beulich <jbeulich@suse.com> > Cc: Tim Deegan <tim@xen.org> > --- > tools/flask/policy/policy/flask/access_vectors | 1 + > xen/arch/x86/domctl.c | 8 ++--- > xen/arch/x86/mm/mem_event.c | 45 +++++++++----------------- > xen/arch/x86/mm/mem_sharing.c | 25 +++++++++++--- > xen/include/asm-x86/mem_event.h | 1 - > xen/include/xsm/dummy.h | 23 ++++++++++++- > xen/include/xsm/xsm.h | 24 ++++++++++++-- > xen/xsm/dummy.c | 5 ++- > xen/xsm/flask/hooks.c | 25 ++++++++++++-- > xen/xsm/flask/include/av_perm_to_string.h | 1 + > xen/xsm/flask/include/av_permissions.h | 1 + > 11 files changed, 113 insertions(+), 46 deletions(-) > > diff --git a/tools/flask/policy/policy/flask/access_vectors b/tools/flask/policy/policy/flask/access_vectors > index ea65e45..45ac437 100644 > --- a/tools/flask/policy/policy/flask/access_vectors > +++ b/tools/flask/policy/policy/flask/access_vectors > @@ -102,6 +102,7 @@ class hvm > mem_sharing > audit_p2m > send_irq > + share_mem > } > > class event > diff --git a/xen/arch/x86/domctl.c b/xen/arch/x86/domctl.c > index a0ecd95..4a188e5 100644 > --- a/xen/arch/x86/domctl.c > +++ b/xen/arch/x86/domctl.c > @@ -1449,10 +1449,8 @@ long arch_do_domctl( > d = rcu_lock_domain_by_id(domctl->domain); > if ( d != NULL ) > { > - ret = xsm_mem_event(d); > - if ( !ret ) > - ret = mem_event_domctl(d, &domctl->u.mem_event_op, > - guest_handle_cast(u_domctl, void)); > + ret = mem_event_domctl(d, &domctl->u.mem_event_op, > + guest_handle_cast(u_domctl, void)); > rcu_unlock_domain(d); > copy_to_guest(u_domctl, domctl, 1); > } > @@ -1508,7 +1506,7 @@ long arch_do_domctl( > d = rcu_lock_domain_by_id(domctl->domain); > if ( d != NULL ) > { > - ret = xsm_mem_event(d); > + ret = xsm_mem_event_setup(d); > if ( !ret ) { > p2m = p2m_get_hostp2m(d); > p2m->access_required = domctl->u.access_required.access_required; > diff --git a/xen/arch/x86/mm/mem_event.c b/xen/arch/x86/mm/mem_event.c > index d728889..d574541 100644 > --- a/xen/arch/x86/mm/mem_event.c > +++ b/xen/arch/x86/mm/mem_event.c > @@ -29,6 +29,7 @@ > #include <asm/mem_paging.h> > #include <asm/mem_access.h> > #include <asm/mem_sharing.h> > +#include <xsm/xsm.h> > > /* for public/io/ring.h macros */ > #define xen_mb() mb() > @@ -439,34 +440,22 @@ static void mem_sharing_notification(struct vcpu *v, unsigned int port) > mem_sharing_sharing_resume(v->domain); > } > > -struct domain *get_mem_event_op_target(uint32_t domain, int *rc) > -{ > - struct domain *d; > - > - /* Get the target domain */ > - *rc = rcu_lock_remote_target_domain_by_id(domain, &d); > - if ( *rc != 0 ) > - return NULL; > - > - /* Not dying? */ > - if ( d->is_dying ) > - { > - rcu_unlock_domain(d); > - *rc = -EINVAL; > - return NULL; > - } > - > - return d; > -} > - > int do_mem_event_op(int op, uint32_t domain, void *arg) > { > int ret; > struct domain *d; >This hunk and the two hunks below in men_sharing share a lot of common code. Can we refactor a bit? Proposal (I don''t really care about the exact function naming or prototype but I do care about duplicate code): static inline int struct domain *get_mem_event_op_target(uint32_t domain, int *rc) { struct domain *d = rcu_lock_domain_by_any_id(domain); if ( !d ) { *rc = -ESRCH; return d; } *rc = -EINVAL; if ( d->is_dying || d == current->domain ) { rcu_unlock_domain(d); return NULL; } *rc = 0; return d; } Then in do_mem_event_op d = get_mem_event_op_target(domain, &ret) if (!d) return ret; ret = xsm_mem_event_op(d, op); And then in men_sharing_c. static inline struct domain * get_mem_sharing_target(uint32_t domain, int *rc) { struct domain *d = get_mem_event_op_target(domain, rc) if (!d) return NULL; if (!mem_sharing_enabled(d)) { *rc = -EINVAL; rcu_unlock_domain(d); return NULL; } *rc = xsm_mem_sharing_op(d, op) if (*rc) { rcu_unlock_domain(d); return NULL; } return d; } And you can call get_mem_sharing_target in the two spots where you have code duplication.> - d = get_mem_event_op_target(domain, &ret); > + d = rcu_lock_domain_by_any_id(domain); > if ( !d ) > - return ret; > + return -ESRCH; > + > + ret = -EINVAL; > + if ( d->is_dying || d == current->domain ) > + goto out; > + > + ret = xsm_mem_event_op(d, op); > + if ( ret ) > + goto out; > > switch (op) > { > @@ -483,6 +472,7 @@ int do_mem_event_op(int op, uint32_t domain, void *arg) > ret = -ENOSYS; > } > > + out: > rcu_unlock_domain(d); > return ret; > } > @@ -516,6 +506,10 @@ int mem_event_domctl(struct domain *d, xen_domctl_mem_event_op_t *mec, > { > int rc; > > + rc = xsm_mem_event_control(d, mec->mode, mec->op); > + if ( rc ) > + return rc; > + > if ( unlikely(d == current->domain) ) > { > gdprintk(XENLOG_INFO, "Tried to do a memory event op on itself.\n"); > @@ -537,13 +531,6 @@ int mem_event_domctl(struct domain *d, xen_domctl_mem_event_op_t *mec, > return -EINVAL; > } > > - /* TODO: XSM hook */ > -#if 0 > - rc = xsm_mem_event_control(d, mec->op); > - if ( rc ) > - return rc; > -#endif > - > rc = -ENOSYS; > > switch ( mec->mode ) > diff --git a/xen/arch/x86/mm/mem_sharing.c b/xen/arch/x86/mm/mem_sharing.c > index 5103285..eff8ae5 100644 > --- a/xen/arch/x86/mm/mem_sharing.c > +++ b/xen/arch/x86/mm/mem_sharing.c > @@ -34,6 +34,7 @@ > #include <asm/atomic.h> > #include <xen/rcupdate.h> > #include <asm/event.h> > +#include <xsm/xsm.h> > > #include "mm-locks.h" > > @@ -1345,11 +1346,19 @@ int mem_sharing_memop(struct domain *d, xen_mem_sharing_op_t *mec) > if ( !mem_sharing_enabled(d) ) > return -EINVAL; >Even if there is a good reason to not go for my suggestion above, there is an obvious chance to refactor the code duplication below. Thanks! Andres> - cd = get_mem_event_op_target(mec->u.share.client_domain, &rc); > + cd = rcu_lock_domain_by_any_id(mec->u.share.client_domain); > if ( !cd ) > + return -ESRCH; > + > + rc = xsm_mem_sharing_op(d, cd, mec->op); > + if ( rc ) > + { > + rcu_unlock_domain(cd); > return rc; > + } > > - if ( !mem_sharing_enabled(cd) ) > + if ( cd == current->domain || cd->is_dying || > + !mem_sharing_enabled(cd) ) > { > rcu_unlock_domain(cd); > return -EINVAL; > @@ -1401,11 +1410,19 @@ int mem_sharing_memop(struct domain *d, xen_mem_sharing_op_t *mec) > if ( !mem_sharing_enabled(d) ) > return -EINVAL; > > - cd = get_mem_event_op_target(mec->u.share.client_domain, &rc); > + cd = rcu_lock_domain_by_any_id(mec->u.share.client_domain); > if ( !cd ) > + return -ESRCH; > + > + rc = xsm_mem_sharing_op(d, cd, mec->op); > + if ( rc ) > + { > + rcu_unlock_domain(cd); > return rc; > + } > > - if ( !mem_sharing_enabled(cd) ) > + if ( cd == current->domain || cd->is_dying || > + !mem_sharing_enabled(cd) ) > { > rcu_unlock_domain(cd); > return -EINVAL; > diff --git a/xen/include/asm-x86/mem_event.h b/xen/include/asm-x86/mem_event.h > index 23d71c1..448be4f 100644 > --- a/xen/include/asm-x86/mem_event.h > +++ b/xen/include/asm-x86/mem_event.h > @@ -62,7 +62,6 @@ void mem_event_put_request(struct domain *d, struct mem_event_domain *med, > int mem_event_get_response(struct domain *d, struct mem_event_domain *med, > mem_event_response_t *rsp); > > -struct domain *get_mem_event_op_target(uint32_t domain, int *rc); > int do_mem_event_op(int op, uint32_t domain, void *arg); > int mem_event_domctl(struct domain *d, xen_domctl_mem_event_op_t *mec, > XEN_GUEST_HANDLE(void) u_domctl); > diff --git a/xen/include/xsm/dummy.h b/xen/include/xsm/dummy.h > index a9784f5..1760cd9 100644 > --- a/xen/include/xsm/dummy.h > +++ b/xen/include/xsm/dummy.h > @@ -551,16 +551,37 @@ static XSM_DEFAULT(int, hvm_inject_msi) (struct domain *d) > return 0; > } > > -static XSM_DEFAULT(int, mem_event) (struct domain *d) > +static XSM_DEFAULT(int, mem_event_setup) (struct domain *d) > { > return 0; > } > > +static XSM_DEFAULT(int, mem_event_control) (struct domain *d, int mode, int op) > +{ > + if ( !IS_PRIV(current->domain) ) > + return -EPERM; > + return 0; > +} > + > +static XSM_DEFAULT(int, mem_event_op) (struct domain *d, int op) > +{ > + if ( !IS_PRIV_FOR(current->domain, d) ) > + return -EPERM; > + return 0; > +} > + > static XSM_DEFAULT(int, mem_sharing) (struct domain *d) > { > return 0; > } > > +static XSM_DEFAULT(int, mem_sharing_op) (struct domain *d, struct domain *cd, int op) > +{ > + if ( !IS_PRIV_FOR(current->domain, cd) ) > + return -EPERM; > + return 0; > +} > + > static XSM_DEFAULT(int, apic) (struct domain *d, int cmd) > { > if ( !IS_PRIV(d) ) > diff --git a/xen/include/xsm/xsm.h b/xen/include/xsm/xsm.h > index ef3329f..c2c1efa 100644 > --- a/xen/include/xsm/xsm.h > +++ b/xen/include/xsm/xsm.h > @@ -151,8 +151,11 @@ struct xsm_operations { > int (*hvm_set_isa_irq_level) (struct domain *d); > int (*hvm_set_pci_link_route) (struct domain *d); > int (*hvm_inject_msi) (struct domain *d); > - int (*mem_event) (struct domain *d); > + int (*mem_event_setup) (struct domain *d); > + int (*mem_event_control) (struct domain *d, int mode, int op); > + int (*mem_event_op) (struct domain *d, int op); > int (*mem_sharing) (struct domain *d); > + int (*mem_sharing_op) (struct domain *d, struct domain *cd, int op); > int (*apic) (struct domain *d, int cmd); > int (*xen_settime) (void); > int (*memtype) (uint32_t access); > @@ -663,9 +666,19 @@ static inline int xsm_hvm_inject_msi (struct domain *d) > return xsm_ops->hvm_inject_msi(d); > } > > -static inline int xsm_mem_event (struct domain *d) > +static inline int xsm_mem_event_setup (struct domain *d) > { > - return xsm_ops->mem_event(d); > + return xsm_ops->mem_event_setup(d); > +} > + > +static inline int xsm_mem_event_control (struct domain *d, int mode, int op) > +{ > + return xsm_ops->mem_event_control(d, mode, op); > +} > + > +static inline int xsm_mem_event_op (struct domain *d, int op) > +{ > + return xsm_ops->mem_event_op(d, op); > } > > static inline int xsm_mem_sharing (struct domain *d) > @@ -673,6 +686,11 @@ static inline int xsm_mem_sharing (struct domain *d) > return xsm_ops->mem_sharing(d); > } > > +static inline int xsm_mem_sharing_op (struct domain *d, struct domain *cd, int op) > +{ > + return xsm_ops->mem_sharing_op(d, cd, op); > +} > + > static inline int xsm_apic (struct domain *d, int cmd) > { > return xsm_ops->apic(d, cmd); > diff --git a/xen/xsm/dummy.c b/xen/xsm/dummy.c > index c76212d..c82c464 100644 > --- a/xen/xsm/dummy.c > +++ b/xen/xsm/dummy.c > @@ -135,8 +135,11 @@ void xsm_fixup_ops (struct xsm_operations *ops) > set_to_dummy_if_null(ops, hvm_set_isa_irq_level); > set_to_dummy_if_null(ops, hvm_set_pci_link_route); > set_to_dummy_if_null(ops, hvm_inject_msi); > - set_to_dummy_if_null(ops, mem_event); > + set_to_dummy_if_null(ops, mem_event_setup); > + set_to_dummy_if_null(ops, mem_event_control); > + set_to_dummy_if_null(ops, mem_event_op); > set_to_dummy_if_null(ops, mem_sharing); > + set_to_dummy_if_null(ops, mem_sharing_op); > set_to_dummy_if_null(ops, apic); > set_to_dummy_if_null(ops, xen_settime); > set_to_dummy_if_null(ops, memtype); > diff --git a/xen/xsm/flask/hooks.c b/xen/xsm/flask/hooks.c > index 421087f..f993696 100644 > --- a/xen/xsm/flask/hooks.c > +++ b/xen/xsm/flask/hooks.c > @@ -1275,7 +1275,17 @@ static int flask_hvm_inject_msi(struct domain *d) > return current_has_perm(d, SECCLASS_HVM, HVM__SEND_IRQ); > } > > -static int flask_mem_event(struct domain *d) > +static int flask_mem_event_setup(struct domain *d) > +{ > + return current_has_perm(d, SECCLASS_HVM, HVM__MEM_EVENT); > +} > + > +static int flask_mem_event_control(struct domain *d, int mode, int op) > +{ > + return current_has_perm(d, SECCLASS_HVM, HVM__MEM_EVENT); > +} > + > +static int flask_mem_event_op(struct domain *d, int op) > { > return current_has_perm(d, SECCLASS_HVM, HVM__MEM_EVENT); > } > @@ -1285,6 +1295,14 @@ static int flask_mem_sharing(struct domain *d) > return current_has_perm(d, SECCLASS_HVM, HVM__MEM_SHARING); > } > > +static int flask_mem_sharing_op(struct domain *d, struct domain *cd, int op) > +{ > + int rc = current_has_perm(cd, SECCLASS_HVM, HVM__MEM_SHARING); > + if ( rc ) > + return rc; > + return domain_has_perm(d, cd, SECCLASS_HVM, HVM__SHARE_MEM); > +} > + > static int flask_apic(struct domain *d, int cmd) > { > u32 perm; > @@ -1734,8 +1752,11 @@ static struct xsm_operations flask_ops = { > .hvm_set_isa_irq_level = flask_hvm_set_isa_irq_level, > .hvm_set_pci_link_route = flask_hvm_set_pci_link_route, > .hvm_inject_msi = flask_hvm_inject_msi, > - .mem_event = flask_mem_event, > + .mem_event_setup = flask_mem_event_setup, > + .mem_event_control = flask_mem_event_control, > + .mem_event_op = flask_mem_event_op, > .mem_sharing = flask_mem_sharing, > + .mem_sharing_op = flask_mem_sharing_op, > .apic = flask_apic, > .xen_settime = flask_xen_settime, > .memtype = flask_memtype, > diff --git a/xen/xsm/flask/include/av_perm_to_string.h b/xen/xsm/flask/include/av_perm_to_string.h > index 894910c..186f1fa 100644 > --- a/xen/xsm/flask/include/av_perm_to_string.h > +++ b/xen/xsm/flask/include/av_perm_to_string.h > @@ -84,6 +84,7 @@ > S_(SECCLASS_HVM, HVM__MEM_SHARING, "mem_sharing") > S_(SECCLASS_HVM, HVM__AUDIT_P2M, "audit_p2m") > S_(SECCLASS_HVM, HVM__SEND_IRQ, "send_irq") > + S_(SECCLASS_HVM, HVM__SHARE_MEM, "share_mem") > S_(SECCLASS_EVENT, EVENT__BIND, "bind") > S_(SECCLASS_EVENT, EVENT__SEND, "send") > S_(SECCLASS_EVENT, EVENT__STATUS, "status") > diff --git a/xen/xsm/flask/include/av_permissions.h b/xen/xsm/flask/include/av_permissions.h > index 1bdb515..b3831f6 100644 > --- a/xen/xsm/flask/include/av_permissions.h > +++ b/xen/xsm/flask/include/av_permissions.h > @@ -87,6 +87,7 @@ > #define HVM__MEM_SHARING 0x00001000UL > #define HVM__AUDIT_P2M 0x00002000UL > #define HVM__SEND_IRQ 0x00004000UL > +#define HVM__SHARE_MEM 0x00008000UL > > #define EVENT__BIND 0x00000001UL > #define EVENT__SEND 0x00000002UL > -- > 1.7.11.4