George S. Coker, II
2007-May-07 21:41 UTC
[Xen-devel][Xense-devel][PATCH][1/4] Xen Security Modules: XSM
Updates in this patch set include:
- adaptation to new create secure interface for domain_create
- cleanup of xsm enable/disable framework through xsm_call macro
- ifdef architecture/config specific hooks
Signed-off-by: George Coker <gscoker@alpha.ncsc.mil>
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel
Chris Wright
2007-May-07 23:10 UTC
Re: [Xen-devel][Xense-devel][PATCH][1/4] Xen Security Modules: XSM
* George S. Coker, II (gscoker@alpha.ncsc.mil) wrote:> Updates in this patch set include: > - adaptation to new create secure interface for domain_create > - cleanup of xsm enable/disable framework through xsm_call macro > - ifdef architecture/config specific hooksThanks, I looked at that quickly, only comment I had there was it could be: struct xsm_ops { generic ones... struct xsm_arch_ops arch_ops; } To avoid a bunch of ifdefs there. Some of them looked arch neutral (like add_to_physmap), but I can see they aren''t exactly. Still be nice to make the do_xsm_op hypercall be more than direct pass-thru to module. Other than that, a quick review got me looking at evtchn calls.> diff -r e370c94bd6fd -r 62b752969edf xen/common/event_channel.c > --- a/xen/common/event_channel.c Sat May 05 13:48:05 2007 +0100 > +++ b/xen/common/event_channel.c Mon May 07 14:50:16 2007 -0400 > @@ -30,6 +30,7 @@ > #include <public/xen.h> > #include <public/event_channel.h> > #include <acm/acm_hooks.h> > +#include <xsm/xsm.h> > > #define bucket_from_port(d,p) \ > ((d)->evtchn[(p)/EVTCHNS_PER_BUCKET]) > @@ -89,8 +90,15 @@ static int get_free_port(struct domain * > chn = xmalloc_array(struct evtchn, EVTCHNS_PER_BUCKET); > if ( unlikely(chn == NULL) ) > return -ENOMEM; > + > memset(chn, 0, EVTCHNS_PER_BUCKET * sizeof(*chn)); > bucket_from_port(d, port) = chn; > + > + if ( xsm_alloc_security_evtchn(chn) ) > + { > + xfree(chn); > + return -ENOMEM; > + }Oops, this is a domain triggerable use-after free bug-in-waiting. Now the bucket is perceived valid, but the memory isn''t. In fact, this is not the right interface. You are only allocating the an opaque security blob per bucket, not per channel. In effect it''s like: struct evtchn chn[EVTCHNS_PER_BUCKET]; xsm_alloc_security(&chn[0]); When I believe you want smth effectively like: struct evtchn chn[EVTCHNS_PER_BUCKET]; for (i=0; i < EVTCHNS_PER_BUCKET; i++) xsm_alloc_security(&chn[i]);> return port; > } > @@ -120,6 +128,10 @@ static long evtchn_alloc_unbound(evtchn_ > if ( (port = get_free_port(d)) < 0 ) > ERROR_EXIT(port); > chn = evtchn_from_port(d, port); > + > + rc = xsm_evtchn_unbound(d, chn, alloc->remote_dom); > + if ( rc ) > + goto out; > > chn->state = ECS_UNBOUND; > if ( (chn->u.unbound.remote_domid = alloc->remote_dom) == DOMID_SELF ) > @@ -176,6 +188,10 @@ static long evtchn_bind_interdomain(evtc > if ( (rchn->state != ECS_UNBOUND) || > (rchn->u.unbound.remote_domid != ld->domain_id) ) > ERROR_EXIT(-EINVAL); > + > + rc = xsm_evtchn_interdomain(ld, lchn, rd, rchn); > + if ( rc ) > + goto out; > > lchn->u.interdomain.remote_dom = rd; > lchn->u.interdomain.remote_port = (u16)rport; > @@ -231,6 +247,7 @@ static long evtchn_bind_virq(evtchn_bind > ERROR_EXIT(port); > > chn = evtchn_from_port(d, port); > + > chn->state = ECS_VIRQ; > chn->notify_vcpu_id = vcpu; > chn->u.virq = virq; > @@ -261,14 +278,15 @@ static long evtchn_bind_ipi(evtchn_bind_ > ERROR_EXIT(port); > > chn = evtchn_from_port(d, port); > + > chn->state = ECS_IPI; > chn->notify_vcpu_id = vcpu; > > bind->port = port; > > + spin_unlock(&d->evtchn_lock); > + > out: > - spin_unlock(&d->evtchn_lock); > -This is incorrect, leaves domain locked on error path (yes, ERROR_EXIT is mean macro abuse!).> return rc; > } > > @@ -427,6 +445,8 @@ static long __evtchn_close(struct domain > chn1->state = ECS_FREE; > chn1->notify_vcpu_id = 0; > > + xsm_evtchn_close_post(chn1); > + > out: > if ( d2 != NULL ) > { > @@ -470,6 +490,10 @@ long evtchn_send(unsigned int lport) > spin_unlock(&ld->evtchn_lock); > return -EINVAL; > } > + > + ret = xsm_evtchn_send(ld, lchn); > + if ( ret ) > + goto out; > > switch ( lchn->state ) > { > @@ -500,6 +524,7 @@ long evtchn_send(unsigned int lport) > ret = -EINVAL; > } > > +out: > spin_unlock(&ld->evtchn_lock); > > return ret; > @@ -618,6 +643,11 @@ static long evtchn_status(evtchn_status_ > } > > chn = evtchn_from_port(d, port); > + > + rc = xsm_evtchn_status(d, chn); > + if ( rc ) > + goto out; > + > switch ( chn->state ) > { > case ECS_FREE: > @@ -714,6 +744,8 @@ static long evtchn_unmask(evtchn_unmask_ > shared_info_t *s = d->shared_info; > int port = unmask->port; > struct vcpu *v; > + int ret = 0; > + struct evtchn *chn; > > spin_lock(&d->evtchn_lock); > > @@ -723,7 +755,8 @@ static long evtchn_unmask(evtchn_unmask_ > return -EINVAL; > } > > - v = d->vcpu[evtchn_from_port(d, port)->notify_vcpu_id]; > + chn = evtchn_from_port(d, port); > + v = d->vcpu[chn->notify_vcpu_id]; > > /* > * These operations must happen in strict order. Based on > @@ -739,7 +772,7 @@ static long evtchn_unmask(evtchn_unmask_ > > spin_unlock(&d->evtchn_lock); > > - return 0; > + return ret; > } > > > @@ -748,6 +781,7 @@ static long evtchn_reset(evtchn_reset_t > domid_t dom = r->dom; > struct domain *d; > int i; > + int rc; > > if ( dom == DOMID_SELF ) > dom = current->domain->domain_id; > @@ -757,6 +791,13 @@ static long evtchn_reset(evtchn_reset_t > if ( (d = rcu_lock_domain_by_id(dom)) == NULL ) > return -ESRCH; > > + rc = xsm_evtchn_reset(current->domain, d); > + if ( rc ) > + { > + rcu_unlock_domain(d); > + return rc; > + } > + > for ( i = 0; port_is_valid(d, i); i++ ) > (void)__evtchn_close(d, i); > > @@ -948,10 +989,14 @@ void notify_via_xen_event_channel(int lp > > int evtchn_init(struct domain *d) > { > + struct evtchn *chn; > + > spin_lock_init(&d->evtchn_lock); > if ( get_free_port(d) != 0 ) > return -EINVAL; > - evtchn_from_port(d, 0)->state = ECS_RESERVED; > + chn = evtchn_from_port(d, 0); > + chn->state = ECS_RESERVED; > + > return 0; > } > > @@ -967,7 +1012,10 @@ void evtchn_destroy(struct domain *d) > } > > for ( i = 0; i < NR_EVTCHN_BUCKETS; i++ ) > + { > + xsm_free_security_evtchn(d->evtchn[i]);Yeah, like this. Got it right on destroy.> xfree(d->evtchn[i]); > + } > }_______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
George S. Coker, II
2007-May-08 00:59 UTC
Re: [Xen-devel][Xense-devel][PATCH][1/4] Xen Security Modules: XSM
> Thanks, I looked at that quickly, only comment I had there was it > could be: > > struct xsm_ops { > generic ones... > struct xsm_arch_ops arch_ops; > } >I guess I really don''t see how this makes it any better since there will likely always be a mix of XSM "supported" ops and module specific ops. If you are trying to tighten down the interface, any command, support or specific could be abused. Please remind me what advantage you are thinking about here.> To avoid a bunch of ifdefs there. Some of them looked arch neutral > (like add_to_physmap), but I can see they aren''t exactly. > > Still be nice to make the do_xsm_op hypercall be more than > direct pass-thru to module. > > Other than that, a quick review got me looking at evtchn calls. > > > diff -r e370c94bd6fd -r 62b752969edf xen/common/event_channel.c > > --- a/xen/common/event_channel.c Sat May 05 13:48:05 2007 +0100 > > +++ b/xen/common/event_channel.c Mon May 07 14:50:16 2007 -0400 > > @@ -30,6 +30,7 @@ > > #include <public/xen.h> > > #include <public/event_channel.h> > > #include <acm/acm_hooks.h> > > +#include <xsm/xsm.h> > > > > #define bucket_from_port(d,p) \ > > ((d)->evtchn[(p)/EVTCHNS_PER_BUCKET]) > > @@ -89,8 +90,15 @@ static int get_free_port(struct domain * > > chn = xmalloc_array(struct evtchn, EVTCHNS_PER_BUCKET); > > if ( unlikely(chn == NULL) ) > > return -ENOMEM; > > + > > memset(chn, 0, EVTCHNS_PER_BUCKET * sizeof(*chn)); > > bucket_from_port(d, port) = chn; > > + > > + if ( xsm_alloc_security_evtchn(chn) ) > > + { > > + xfree(chn); > > + return -ENOMEM; > > + } > > Oops, this is a domain triggerable use-after free bug-in-waiting. > Now the bucket is perceived valid, but the memory isn''t. > > In fact, this is not the right interface. You are only allocating the > an opaque security blob per bucket, not per channel. In effect it''s like: > > struct evtchn chn[EVTCHNS_PER_BUCKET]; > xsm_alloc_security(&chn[0]); > > When I believe you want smth effectively like: > > struct evtchn chn[EVTCHNS_PER_BUCKET]; > for (i=0; i < EVTCHNS_PER_BUCKET; i++) > xsm_alloc_security(&chn[i]); > > > return port; > > }This is a mistake in the framework because the behavior between alloc and free is inconsistent. The Flask module does this correctly, but this is a gotcha for a new module. I''ll fix this and repost tomorrow.> > @@ -120,6 +128,10 @@ static long evtchn_alloc_unbound(evtchn_ > > if ( (port = get_free_port(d)) < 0 ) > > ERROR_EXIT(port); > > chn = evtchn_from_port(d, port); > > + > > + rc = xsm_evtchn_unbound(d, chn, alloc->remote_dom); > > + if ( rc ) > > + goto out; > > > > chn->state = ECS_UNBOUND; > > if ( (chn->u.unbound.remote_domid = alloc->remote_dom) == DOMID_SELF ) > > @@ -176,6 +188,10 @@ static long evtchn_bind_interdomain(evtc > > if ( (rchn->state != ECS_UNBOUND) || > > (rchn->u.unbound.remote_domid != ld->domain_id) ) > > ERROR_EXIT(-EINVAL); > > + > > + rc = xsm_evtchn_interdomain(ld, lchn, rd, rchn); > > + if ( rc ) > > + goto out; > > > > lchn->u.interdomain.remote_dom = rd; > > lchn->u.interdomain.remote_port = (u16)rport; > > @@ -231,6 +247,7 @@ static long evtchn_bind_virq(evtchn_bind > > ERROR_EXIT(port); > > > > chn = evtchn_from_port(d, port); > > + > > chn->state = ECS_VIRQ; > > chn->notify_vcpu_id = vcpu; > > chn->u.virq = virq; > > @@ -261,14 +278,15 @@ static long evtchn_bind_ipi(evtchn_bind_ > > ERROR_EXIT(port); > > > > chn = evtchn_from_port(d, port); > > + > > chn->state = ECS_IPI; > > chn->notify_vcpu_id = vcpu; > > > > bind->port = port; > > > > + spin_unlock(&d->evtchn_lock); > > + > > out: > > - spin_unlock(&d->evtchn_lock); > > - > > This is incorrect, leaves domain locked on error path (yes, ERROR_EXIT > is mean macro abuse!). >Absolutely this is a problem, when did ERROR_EXIT come about?> > return rc; > > } > > > > @@ -427,6 +445,8 @@ static long __evtchn_close(struct domain > > chn1->state = ECS_FREE; > > chn1->notify_vcpu_id = 0; > > > > + xsm_evtchn_close_post(chn1); > > + > > out: > > if ( d2 != NULL ) > > { > > @@ -470,6 +490,10 @@ long evtchn_send(unsigned int lport) > > spin_unlock(&ld->evtchn_lock); > > return -EINVAL; > > } > > + > > + ret = xsm_evtchn_send(ld, lchn); > > + if ( ret ) > > + goto out; > > > > switch ( lchn->state ) > > { > > @@ -500,6 +524,7 @@ long evtchn_send(unsigned int lport) > > ret = -EINVAL; > > } > > > > +out: > > spin_unlock(&ld->evtchn_lock); > > > > return ret; > > @@ -618,6 +643,11 @@ static long evtchn_status(evtchn_status_ > > } > > > > chn = evtchn_from_port(d, port); > > + > > + rc = xsm_evtchn_status(d, chn); > > + if ( rc ) > > + goto out; > > + > > switch ( chn->state ) > > { > > case ECS_FREE: > > @@ -714,6 +744,8 @@ static long evtchn_unmask(evtchn_unmask_ > > shared_info_t *s = d->shared_info; > > int port = unmask->port; > > struct vcpu *v; > > + int ret = 0; > > + struct evtchn *chn; > > > > spin_lock(&d->evtchn_lock); > > > > @@ -723,7 +755,8 @@ static long evtchn_unmask(evtchn_unmask_ > > return -EINVAL; > > } > > > > - v = d->vcpu[evtchn_from_port(d, port)->notify_vcpu_id]; > > + chn = evtchn_from_port(d, port); > > + v = d->vcpu[chn->notify_vcpu_id]; > > > > /* > > * These operations must happen in strict order. Based on > > @@ -739,7 +772,7 @@ static long evtchn_unmask(evtchn_unmask_ > > > > spin_unlock(&d->evtchn_lock); > > > > - return 0; > > + return ret; > > } > > > > > > @@ -748,6 +781,7 @@ static long evtchn_reset(evtchn_reset_t > > domid_t dom = r->dom; > > struct domain *d; > > int i; > > + int rc; > > > > if ( dom == DOMID_SELF ) > > dom = current->domain->domain_id; > > @@ -757,6 +791,13 @@ static long evtchn_reset(evtchn_reset_t > > if ( (d = rcu_lock_domain_by_id(dom)) == NULL ) > > return -ESRCH; > > > > + rc = xsm_evtchn_reset(current->domain, d); > > + if ( rc ) > > + { > > + rcu_unlock_domain(d); > > + return rc; > > + } > > + > > for ( i = 0; port_is_valid(d, i); i++ ) > > (void)__evtchn_close(d, i); > > > > @@ -948,10 +989,14 @@ void notify_via_xen_event_channel(int lp > > > > int evtchn_init(struct domain *d) > > { > > + struct evtchn *chn; > > + > > spin_lock_init(&d->evtchn_lock); > > if ( get_free_port(d) != 0 ) > > return -EINVAL; > > - evtchn_from_port(d, 0)->state = ECS_RESERVED; > > + chn = evtchn_from_port(d, 0); > > + chn->state = ECS_RESERVED; > > + > > return 0; > > } > > > > @@ -967,7 +1012,10 @@ void evtchn_destroy(struct domain *d) > > } > > > > for ( i = 0; i < NR_EVTCHN_BUCKETS; i++ ) > > + { > > + xsm_free_security_evtchn(d->evtchn[i]); > > Yeah, like this. Got it right on destroy. > > > xfree(d->evtchn[i]); > > + } > > } > > _______________________________________________ > Xen-devel mailing list > Xen-devel@lists.xensource.com > http://lists.xensource.com/xen-devel >_______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
George S. Coker, II
2007-May-08 01:12 UTC
Re: [Xen-devel][Xense-devel][PATCH][1/4] Xen Security Modules: XSM
> > > chn->state = ECS_VIRQ; > > > chn->notify_vcpu_id = vcpu; > > > chn->u.virq = virq; > > > @@ -261,14 +278,15 @@ static long evtchn_bind_ipi(evtchn_bind_ > > > ERROR_EXIT(port); > > > > > > chn = evtchn_from_port(d, port); > > > + > > > chn->state = ECS_IPI; > > > chn->notify_vcpu_id = vcpu; > > > > > > bind->port = port; > > > > > > + spin_unlock(&d->evtchn_lock); > > > + > > > out: > > > - spin_unlock(&d->evtchn_lock); > > > - > > > > This is incorrect, leaves domain locked on error path (yes, ERROR_EXIT > > is mean macro abuse!). > > > Absolutely this is a problem, when did ERROR_EXIT come about? >The patch was generated incorrectly and contains some old edits. I don''t even mean to be touching this code region. _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Mark Williamson
2007-May-08 02:54 UTC
Re: [Xen-devel][Xense-devel][PATCH][1/4] Xen Security Modules: XSM
Hi George, I''ve been working on some automated checking tools for the Xen tree and I''m afraid your patches came at a convenient time for you to be a test case - sorry :-)> Updates in this patch set include: > - adaptation to new create secure interface for domain_create > - cleanup of xsm enable/disable framework through xsm_call macro > - ifdef architecture/config specific hooksRight now I''ve got a load of coding style nits that you might be interested in. I''m attaching the relevant output of the Xen style checking tool (derived from a pre-existing Linux style checker) below; I''ve given it a look through to verify that it appears sane. It''s largely based on my interpretation of what the Xen codebase style appears to be, so there may be some things you disagree with. I realise you''re still iterating on the design, but I hope this is useful for getting ready for the merge. Please feel free to give feedback if you disagree with of the details. Cheers, Mark -- Dave: Just a question. What use is a unicyle with no seat? And no pedals! Mark: To answer a question with a question: What use is a skateboard? Dave: Skateboards have wheels. Mark: My wheel has a wheel! _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Chris Wright
2007-May-08 03:08 UTC
Re: [Xen-devel][Xense-devel][PATCH][1/4] Xen Security Modules: XSM
* George S. Coker, II (george.coker@gmail.com) wrote:> >Thanks, I looked at that quickly, only comment I had there was it > >could be: > > > >struct xsm_ops { > >generic ones... > >struct xsm_arch_ops arch_ops; > >} > > > I guess I really don''t see how this makes it any better since there > will likely always be a mix of XSM "supported" ops and module specific > ops. If you are trying to tighten down the interface, any command, > support or specific could be abused. Please remind me what advantage > you are thinking about here.I think we are talking about 2 different things. I just mean the code that is currently #ifdef CONFIG_X86 in xsm_operations structure. Typical is to embed arch specific data within the toplevel generic structure. It offers no benefit in code generation or tightening of the interface, etc. It''s simply a readability/maintainability thing to avoid: struct xsm_operations{ generic bits ... #ifdef CONFIG_X86 ... #endif #ifdef CONFIG_IA64 ... #endif #ifdef CONFIG_PPC ... #endif }> >To avoid a bunch of ifdefs there. Some of them looked arch neutral > >(like add_to_physmap), but I can see they aren''t exactly. > > > >Still be nice to make the do_xsm_op hypercall be more than > >direct pass-thru to module.I think this is what you were referring to. The advantage only comes about if there are things that are common to modules (likely) and really shines if there are functions that also use common data structures (unlikely) for type safety and general interface sanity.> >Other than that, a quick review got me looking at evtchn calls. > > > >> diff -r e370c94bd6fd -r 62b752969edf xen/common/event_channel.c > >> --- a/xen/common/event_channel.c Sat May 05 13:48:05 2007 +0100 > >> +++ b/xen/common/event_channel.c Mon May 07 14:50:16 2007 -0400 > >> @@ -30,6 +30,7 @@ > >> #include <public/xen.h> > >> #include <public/event_channel.h> > >> #include <acm/acm_hooks.h> > >> +#include <xsm/xsm.h> > >> > >> #define bucket_from_port(d,p) \ > >> ((d)->evtchn[(p)/EVTCHNS_PER_BUCKET]) > >> @@ -89,8 +90,15 @@ static int get_free_port(struct domain * > >> chn = xmalloc_array(struct evtchn, EVTCHNS_PER_BUCKET); > >> if ( unlikely(chn == NULL) ) > >> return -ENOMEM; > >> + > >> memset(chn, 0, EVTCHNS_PER_BUCKET * sizeof(*chn)); > >> bucket_from_port(d, port) = chn; > >> + > >> + if ( xsm_alloc_security_evtchn(chn) ) > >> + { > >> + xfree(chn); > >> + return -ENOMEM; > >> + } > > > >Oops, this is a domain triggerable use-after free bug-in-waiting. > >Now the bucket is perceived valid, but the memory isn''t. > > > >In fact, this is not the right interface. You are only allocating the > >an opaque security blob per bucket, not per channel. In effect it''s like: > > > >struct evtchn chn[EVTCHNS_PER_BUCKET]; > >xsm_alloc_security(&chn[0]); > > > >When I believe you want smth effectively like: > > > >struct evtchn chn[EVTCHNS_PER_BUCKET]; > >for (i=0; i < EVTCHNS_PER_BUCKET; i++) > > xsm_alloc_security(&chn[i]); > > > >> return port; > >> } > > This is a mistake in the framework because the behavior between alloc > and free is inconsistent. The Flask module does this correctly, but > this is a gotcha for a new module. I''ll fix this and repost tomorrow.Actually Flask got it wrong as well (you probably saw that email later). I agree, the framework is inconsistent, and I think it makes most sense to have the alloc/free operate on a single channel. You may need to introduce a simple helper that does the loop and handles the error condition in the framework which is fine.> >> @@ -120,6 +128,10 @@ static long evtchn_alloc_unbound(evtchn_ > >> if ( (port = get_free_port(d)) < 0 ) > >> ERROR_EXIT(port); > >> chn = evtchn_from_port(d, port); > >> + > >> + rc = xsm_evtchn_unbound(d, chn, alloc->remote_dom); > >> + if ( rc ) > >> + goto out; > >> > >> chn->state = ECS_UNBOUND; > >> if ( (chn->u.unbound.remote_domid = alloc->remote_dom) == > >DOMID_SELF ) > >> @@ -176,6 +188,10 @@ static long evtchn_bind_interdomain(evtc > >> if ( (rchn->state != ECS_UNBOUND) || > >> (rchn->u.unbound.remote_domid != ld->domain_id) ) > >> ERROR_EXIT(-EINVAL); > >> + > >> + rc = xsm_evtchn_interdomain(ld, lchn, rd, rchn); > >> + if ( rc ) > >> + goto out; > >> > >> lchn->u.interdomain.remote_dom = rd; > >> lchn->u.interdomain.remote_port = (u16)rport; > >> @@ -231,6 +247,7 @@ static long evtchn_bind_virq(evtchn_bind > >> ERROR_EXIT(port); > >> > >> chn = evtchn_from_port(d, port); > >> + > >> chn->state = ECS_VIRQ; > >> chn->notify_vcpu_id = vcpu; > >> chn->u.virq = virq; > >> @@ -261,14 +278,15 @@ static long evtchn_bind_ipi(evtchn_bind_ > >> ERROR_EXIT(port); > >> > >> chn = evtchn_from_port(d, port); > >> + > >> chn->state = ECS_IPI; > >> chn->notify_vcpu_id = vcpu; > >> > >> bind->port = port; > >> > >> + spin_unlock(&d->evtchn_lock); > >> + > >> out: > >> - spin_unlock(&d->evtchn_lock); > >> - > > > >This is incorrect, leaves domain locked on error path (yes, ERROR_EXIT > >is mean macro abuse!). > > > Absolutely this is a problem, when did ERROR_EXIT come about?It''s been there a long time. I think the issue is about your followup. Given a couple gratuitous newlines, and this one, etc. it looks like there''s just a small bit of leftover cruft in the diff. thanks, -chris _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
John McDermott (US Navy Employee)
2007-May-08 12:44 UTC
Re: [Xen-devel][Xense-devel][PATCH][1/4] Xen Security Modules: XSM
Mark,
Does your tool have a pretty-printing capability? It seems like it would
be more efficient to just reformat the source to comply with whatever
standards Xen follows. Everyone could apply the tool before building
patches or submitting code, but still use their own locally preferred
style while developing.
Sincerely,
John
--
What is the formal meaning of the one-line program
#include "/dev/tty"
----
.~. _ _
/V\ -o) -o)
/( )\ /\\ /\\
^^^^^ _\_v _\_v
J. P. McDermott Building 12
Code 5542 John.McDermott@NRL.Navy.mil
Naval Research Laboratory Tel: +1 202.404.8301
Washington, DC 20375, USA Fax: +1 202.404.7942
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel
Mark Williamson
2007-May-08 13:12 UTC
Re: [Xen-devel][Xense-devel][PATCH][1/4] Xen Security Modules: XSM
> Does your tool have a pretty-printing capability? It seems like it would > be more efficient to just reformat the source to comply with whatever > standards Xen follows. Everyone could apply the tool before building > patches or submitting code, but still use their own locally preferred > style while developing.It doesn''t have this capability at the moment. It''s just a version of the Linux patchstylecheckemail.pl that I''ve modified to understand the different coding conventions in Xen. GNU indent can quite possibly be configured to reformat the code as required - I''ll look into finding a configuration setup that works for this (I''ll probably start with the Linux lindent script and work from there - we''re reasonably close to Linux conventions in many ways). The style checker does various checks beyond indenting, though, so it''ll still be worth running it. I hope to get the it into mainline Xen at some point so that folks can just "make stylecheck" or similar before they submit patches. Thanks for the feedback! Cheers, Mark -- Dave: Just a question. What use is a unicyle with no seat? And no pedals! Mark: To answer a question with a question: What use is a skateboard? Dave: Skateboards have wheels. Mark: My wheel has a wheel! _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Derek Murray
2007-May-09 14:04 UTC
Re: [Xen-devel][Xense-devel][PATCH][1/4] Xen Security Modules: XSM
I''m interested in whether this code could be used to supersede IS_PRIV (dom), particularly when doing an mmu_update operation. As far as I can see, the xsm_mmu_normal_update() hook is called after set_foreigndom(). set_foreigndom() will fail if the calling domain is not privileged (!IS_PRIV(current->domain)) and the operation specifies a different domain as the foreigndom. For disaggregation of the domain builder, we would like to be able to delegate this privilege to a small, trusted domain (domB): it seems to me that XSM would be the cleanest way to do this. Would it therefore be possible to add a hook in set_foreigndom() on the ! IS_PRIV(d) branch, or is there some security consequence that I am overlooking? Regards, Derek Murray. On 7 May 2007, at 22:41, George S. Coker, II wrote:> Updates in this patch set include: > - adaptation to new create secure interface for domain_create > - cleanup of xsm enable/disable framework through xsm_call macro > - ifdef architecture/config specific hooks > > Signed-off-by: George Coker <gscoker@alpha.ncsc.mil> > <xsm-050707-xen-15011.diff> > _______________________________________________ > Xen-devel mailing list > Xen-devel@lists.xensource.com > http://lists.xensource.com/xen-devel_______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Keir Fraser
2007-May-09 14:23 UTC
Re: [Xen-devel][Xense-devel][PATCH][1/4] Xen Security Modules: XSM
On 9/5/07 15:04, "Derek Murray" <Derek.Murray@cl.cam.ac.uk> wrote:> For disaggregation of the domain builder, we would like to be able to > delegate this privilege to a small, trusted domain (domB): it seems > to me that XSM would be the cleanest way to do this. Would it > therefore be possible to add a hook in set_foreigndom() on the ! > IS_PRIV(d) branch, or is there some security consequence that I am > overlooking?Better to get rid of the IS_PRIV() altogether? If we''re adding these hooks all over the place we may as well get some benefit from them, even if it means adding extra ones. Only question then is whether conflating security constraints required for correct operation of the Xen platform (i.e., capabilities of the disaggregated dom0 domains) with the constraints required for domU''s, and trying to express these in the same security policy module actually makes sense. It might make sense to ''shim in'' a static built-in policy at those hook points as a pre-filter on the dynamically-loaded policy for ordinary domUs. -- Keir _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
George S. Coker, II
2007-May-09 17:04 UTC
Re: [Xen-devel][Xense-devel][PATCH][1/4] Xen Security Modules: XSM
On Wed, 2007-05-09 at 15:23 +0100, Keir Fraser wrote:> On 9/5/07 15:04, "Derek Murray" <Derek.Murray@cl.cam.ac.uk> wrote: > > > For disaggregation of the domain builder, we would like to be able to > > delegate this privilege to a small, trusted domain (domB): it seems > > to me that XSM would be the cleanest way to do this. Would it > > therefore be possible to add a hook in set_foreigndom() on the ! > > IS_PRIV(d) branch, or is there some security consequence that I am > > overlooking? > > Better to get rid of the IS_PRIV() altogether? If we''re adding these hooks > all over the place we may as well get some benefit from them, even if it > means adding extra ones. Only question then is whether conflating security > constraints required for correct operation of the Xen platform (i.e., > capabilities of the disaggregated dom0 domains) with the constraints > required for domU''s, and trying to express these in the same security policy > module actually makes sense. It might make sense to ''shim in'' a static > built-in policy at those hook points as a pre-filter on the > dynamically-loaded policy for ordinary domUs. >We could easily move the IS_PRIV() checks to the default/dummy modules. This would ensure that a static security policy is enforced either in the XSM enabled/disabled cases or in the case of a security module neglecting not to implement a specific hook. In all cases, Xen would be protected by some security policy. This is how XSM is setup to behave today, but it is up to the community to commit to making IS_PRIV() the default XSM module. Some review of the current hooks is needed to ensure that existing uses of IS_PRIV() are completely covered. I believe this is the case for almost all of the XSM hooks. In most code paths where there is an IS_PRIV() call there is an XSM hook immediately following. mmu_update is perhaps an exception. I believe the hook is in the right place to control the use of mmu_update and probably does not require an extra hook in set_foreigndom() but there is a side effect from set_foreigndom() (FOREIGNDOM is set to some value in the absense of IS_PRIV()) that must be dealt with in the mmu_update hook. _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
George S. Coker, II
2007-May-09 17:13 UTC
Re: [Xen-devel][Xense-devel][PATCH][1/4] Xen Security Modules: XSM
On Wed, 2007-05-09 at 15:04 +0100, Derek Murray wrote:> I''m interested in whether this code could be used to supersede IS_PRIV > (dom), particularly when doing an mmu_update operation. > > As far as I can see, the xsm_mmu_normal_update() hook is called after > set_foreigndom(). set_foreigndom() will fail if the calling domain is > not privileged (!IS_PRIV(current->domain)) and the operation > specifies a different domain as the foreigndom. > > For disaggregation of the domain builder, we would like to be able to > delegate this privilege to a small, trusted domain (domB): it seems > to me that XSM would be the cleanest way to do this. Would it > therefore be possible to add a hook in set_foreigndom() on the ! > IS_PRIV(d) branch, or is there some security consequence that I am > overlooking? >I believe we have similar goals here. It should be possible through the XSM framework and the Flask-XSM module to define a policy that enables a fine grain usage model such as disaggregation of the domain builder as well as others. The benefit to Flask-XSM is the flexibility provided is completely general as it is derived through definition of a policy not a specific module. I realize my posts here may be out of order, but look on in this same thread at my other posts regarding management of IS_PRIV() under the XSM default/dummy module. I encourage you to look at the existing hooks and see if the interfaces are suitable for your work as well as determine the kinds of policy you would like to enforce in the hooks. _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Derek Murray
2007-May-11 13:32 UTC
Re: [Xen-devel][Xense-devel][PATCH][1/4] Xen Security Modules: XSM
On 9 May 2007, at 18:04, George S. Coker, II wrote:> Some review of the current hooks is needed to ensure that existing > uses > of IS_PRIV() are completely covered. I believe this is the case for > almost all of the XSM hooks. In most code paths where there is an > IS_PRIV() call there is an XSM hook immediately following. mmu_update > is perhaps an exception. I believe the hook is in the right place to > control the use of mmu_update and probably does not require an extra > hook in set_foreigndom() but there is a side effect from > set_foreigndom() (FOREIGNDOM is set to some value in the absense of > IS_PRIV()) that must be dealt with in the mmu_update hook.I''ve checked through changeset 15011 with your latest XSM patchset, and looked at each instance of IS_PRIV(). I''ve attached the report with this message. The uses of IS_PRIV() boil down to a few categories: 1. Allow privileged domain only. 2. Allow self and privileged domain only. 3. set_foreigndom() 4. Allow self only, but with a different return code (EINVAL instead of EPERM) for privileged domain. 5. Make no access control decision; instead use IS_PRIV() to modify a register value. The presence of XSM hooks can also be categorised: 1. IS_PRIV() directly followed by XSM hook. 2. IS_PRIV() at start of do_*_op function, followed by XSM hooks for the individual cases. 3. IS_PRIV() at start of list-processing function, followed by XSM hook for each item in the list. 4. IS_PRIV() followed by no XSM hook (mostly in IA64 code). The untidiest cases are where set_foreigndom() is involved. These involve do_mmu_update(), do_update_va_otherdomain() and some mmuext_ops. In particular, on the do_update_va_otherdomain() path, IS_PRIV is checked twice. It would seem to me that the cleanest way to do this is to have the permission check first (can domain X access MFN Y of domain Z?), then carry out the set_foreigndom() logic. I am unsure about the location of the xsm_mmu_normal_update() hook. What sort of policy do you envisage being enforced here? In particular, the hook is based on the current domain and the value that is to be written into the page table. In mod_l1_entry(), get_page_from_l1e() is called, which ensures that the page belongs to FOREIGNDOM, so would it be possible just to place the hook before set_foreigndom()? This would have the added benefit of fewer calls to the hook, when multiple updates have been batched together. I am assuming that there are no latent examples of privilege-as-being- dom0 in the code, but I haven''t confirmed this. On 9 May 2007, at 18:13, George S. Coker, II wrote:> I believe we have similar goals here. It should be possible > through the > XSM framework and the Flask-XSM module to define a policy that > enables a > fine grain usage model such as disaggregation of the domain builder as > well as others. The benefit to Flask-XSM is the flexibility > provided is > completely general as it is derived through definition of a policy > not a > specific module.It sounds to me that Flask-XSM provides the flexibility that would be needed for defining a disaggregation policy. I wonder, though, whether the Chinese Wall and Simple Type Enforcement ACM modules (which, if I understand correctly, are separate Xen Security Modules in this framework) would best be written with the IS_PRIV()- replacement code separated out into a "shim" policy as Keir suggested in this thread. Perhaps these modules should delegate to the dummy policy, and, if they pass these hooks, then try the dynamic policy? This would ensure that the Xen static privilege code is in a single location and would hence be kept consistent. Thanks for your input on this, and if I can be of any more help, please let me know. Regards, Derek Murray. _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
George S. Coker, II
2007-May-11 15:10 UTC
Re: [Xen-devel][Xense-devel][PATCH][1/4] Xen Security Modules: XSM
On Fri, 2007-05-11 at 14:32 +0100, Derek Murray wrote:> On 9 May 2007, at 18:04, George S. Coker, II wrote: > > Some review of the current hooks is needed to ensure that existing > > uses > > of IS_PRIV() are completely covered. I believe this is the case for > > almost all of the XSM hooks. In most code paths where there is an > > IS_PRIV() call there is an XSM hook immediately following. mmu_update > > is perhaps an exception. I believe the hook is in the right place to > > control the use of mmu_update and probably does not require an extra > > hook in set_foreigndom() but there is a side effect from > > set_foreigndom() (FOREIGNDOM is set to some value in the absense of > > IS_PRIV()) that must be dealt with in the mmu_update hook. > > I''ve checked through changeset 15011 with your latest XSM patchset, > and looked at each instance of IS_PRIV(). I''ve attached the report > with this message. > > > The uses of IS_PRIV() boil down to a few categories: > > 1. Allow privileged domain only. > 2. Allow self and privileged domain only. > 3. set_foreigndom() > 4. Allow self only, but with a different return code (EINVAL instead > of EPERM) for privileged domain. > 5. Make no access control decision; instead use IS_PRIV() to modify a > register value. > > The presence of XSM hooks can also be categorised: > > 1. IS_PRIV() directly followed by XSM hook. > 2. IS_PRIV() at start of do_*_op function, followed by XSM hooks for > the individual cases. > 3. IS_PRIV() at start of list-processing function, followed by XSM > hook for each item in the list. > 4. IS_PRIV() followed by no XSM hook (mostly in IA64 code).We''re not there yet for IA64, but we will be....any testers out there? So I think XSM is "there" for all of the IS_PRIV uses, with the exception of set_foreigndom() and modification of a register value in traps.c. (I honestly haven''t spent any time thinking about the XSM equivalent to the use of IS_PRIV in traps.c)> The untidiest cases are where set_foreigndom() is involved. These > involve do_mmu_update(), do_update_va_otherdomain() and some > mmuext_ops. In particular, on the do_update_va_otherdomain() path, > IS_PRIV is checked twice. It would seem to me that the cleanest way > to do this is to have the permission check first (can domain X access > MFN Y of domain Z?), then carry out the set_foreigndom() logic. >I think I agree.> I am unsure about the location of the xsm_mmu_normal_update() hook. > What sort of policy do you envisage being enforced here? In > particular, the hook is based on the current domain and the value > that is to be written into the page table. In mod_l1_entry(), > get_page_from_l1e() is called, which ensures that the page belongs to > FOREIGNDOM, so would it be possible just to place the hook before > set_foreigndom()? This would have the added benefit of fewer calls to > the hook, when multiple updates have been batched together. > > I am assuming that there are no latent examples of privilege-as-being- > dom0 in the code, but I haven''t confirmed this.I believe you are correct, although this was not always the case, I think.> On 9 May 2007, at 18:13, George S. Coker, II wrote: > > I believe we have similar goals here. It should be possible > > through the > > XSM framework and the Flask-XSM module to define a policy that > > enables a > > fine grain usage model such as disaggregation of the domain builder as > > well as others. The benefit to Flask-XSM is the flexibility > > provided is > > completely general as it is derived through definition of a policy > > not a > > specific module. > > It sounds to me that Flask-XSM provides the flexibility that would be > needed for defining a disaggregation policy. I wonder, though, > whether the Chinese Wall and Simple Type Enforcement ACM modules > (which, if I understand correctly, are separate Xen Security Modules > in this framework) would best be written with the IS_PRIV()- > replacement code separated out into a "shim" policy as Keir suggested > in this thread. Perhaps these modules should delegate to the dummy > policy, and, if they pass these hooks, then try the dynamic policy? > This would ensure that the Xen static privilege code is in a single > location and would hence be kept consistent. >Currently the existing ACM module is implemented as a single XSM module which stacks (internally) the Chinese Wall and Simple Type Enforcement functionality. (This is the preferred approach for stacking.) ACM-XSM is one module with the flexibility to enforce STE and/or CW policy. The existing ACM was designed to be complementary to Xen''s IS_PRIV(). Moving IS_PRIV() to the default/dummy XSM module does not alter this relationship as the hooks used by ACM are orthogonal to the IS_PRIV() hooks. On init of the XSM (because ACM-XSM does not define replacements for these IS_PRIV() hooks), the hooks from the dummy/default module are integrated (or "shimmed") in to the ACM-XSM module. So I think XSM can do what you and Keir are suggesting.> Thanks for your input on this, and if I can be of any more help, > please let me know. > > Regards, > > Derek Murray.-- George S. Coker, II <gscoker@alpha.ncsc.mil> 443-479-6944 _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Keir Fraser
2007-May-11 16:51 UTC
Re: [Xen-devel][Xense-devel][PATCH][1/4] Xen Security Modules: XSM
On 11/5/07 16:10, "George S. Coker, II" <gscoker@alpha.ncsc.mil> wrote:>> The untidiest cases are where set_foreigndom() is involved. These >> involve do_mmu_update(), do_update_va_otherdomain() and some >> mmuext_ops. In particular, on the do_update_va_otherdomain() path, >> IS_PRIV is checked twice. It would seem to me that the cleanest way >> to do this is to have the permission check first (can domain X access >> MFN Y of domain Z?), then carry out the set_foreigndom() logic. >> > > I think I agree.In this case you theoretically race reuse of the domid, don''t you? Actually you are saved by the RCU mechanism, but why is doing the check after set_foreigndom() hard? The error path out of e.g., do_mmu_update() will correctly give up the foreign reference. -- Keir _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
George S. Coker, II
2007-May-11 17:19 UTC
Re: [Xen-devel][Xense-devel][PATCH][1/4] Xen Security Modules: XSM
On Fri, 2007-05-11 at 17:51 +0100, Keir Fraser wrote:> > > On 11/5/07 16:10, "George S. Coker, II" <gscoker@alpha.ncsc.mil> wrote: > > >> The untidiest cases are where set_foreigndom() is involved. These > >> involve do_mmu_update(), do_update_va_otherdomain() and some > >> mmuext_ops. In particular, on the do_update_va_otherdomain() path, > >> IS_PRIV is checked twice. It would seem to me that the cleanest way > >> to do this is to have the permission check first (can domain X access > >> MFN Y of domain Z?), then carry out the set_foreigndom() logic. > >> > > > > I think I agree. > > In this case you theoretically race reuse of the domid, don''t you? Actually > you are saved by the RCU mechanism, but why is doing the check after > set_foreigndom() hard? The error path out of e.g., do_mmu_update() will > correctly give up the foreign reference. >I guess it''s not, my only concern was for the cleanup of set_foreigndom which is cleaned up as you point out on the error path. _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Stefan Berger
2007-May-18 03:56 UTC
Re: [Xen-devel][Xense-devel][PATCH][1/4] Xen Security Modules: XSM
xen-devel-bounces@lists.xensource.com wrote on 05/11/2007 11:10:08 AM:> On Fri, 2007-05-11 at 14:32 +0100, Derek Murray wrote: > > On 9 May 2007, at 18:04, George S. Coker, II wrote:[...]> Currently the existing ACM module is implemented as a single XSM module > which stacks (internally) the Chinese Wall and Simple Type Enforcement > functionality. (This is the preferred approach for stacking.) ACM-XSM > is one module with the flexibility to enforce STE and/or CW policy. > > The existing ACM was designed to be complementary to Xen''s IS_PRIV(). > Moving IS_PRIV() to the default/dummy XSM module does not alter this > relationship as the hooks used by ACM are orthogonal to the IS_PRIV() > hooks. On init of the XSM (because ACM-XSM does not define replacements > for these IS_PRIV() hooks), the hooks from the dummy/default module are > integrated (or "shimmed") in to the ACM-XSM module. So I think XSM canIf ACM-XSM does not define replacements for the IS_PRIV() hooks, how are you going to integrate them into ACM-XSM? If so, based on what information from the current ACM policy would ACM-XSM enforce the IS_PRIV() check? What if ACM is not active, what enforces IS_PRIV() then? Stefan> do what you and Keir are suggesting.> > > Thanks for your input on this, and if I can be of any more help, > > please let me know. > > > > Regards, > > > > Derek Murray. > -- > George S. Coker, II <gscoker@alpha.ncsc.mil> 443-479-6944 > > > _______________________________________________ > Xen-devel mailing list > Xen-devel@lists.xensource.com > http://lists.xensource.com/xen-devel_______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
George S. Coker, II
2007-May-18 20:00 UTC
Re: [Xen-devel][Xense-devel][PATCH][1/4] Xen Security Modules: XSM
On Thu, 2007-05-17 at 23:56 -0400, Stefan Berger wrote:> > xen-devel-bounces@lists.xensource.com wrote on 05/11/2007 11:10:08 AM: > > > On Fri, 2007-05-11 at 14:32 +0100, Derek Murray wrote: > > > On 9 May 2007, at 18:04, George S. Coker, II wrote: > [...] > > Currently the existing ACM module is implemented as a single XSM > module > > which stacks (internally) the Chinese Wall and Simple Type > Enforcement > > functionality. (This is the preferred approach for stacking.) > ACM-XSM > > is one module with the flexibility to enforce STE and/or CW policy. > > > > The existing ACM was designed to be complementary to Xen''s > IS_PRIV(). > > Moving IS_PRIV() to the default/dummy XSM module does not alter this > > relationship as the hooks used by ACM are orthogonal to the > IS_PRIV() > > hooks. On init of the XSM (because ACM-XSM does not define > replacements > > for these IS_PRIV() hooks), the hooks from the dummy/default module > are > > integrated (or "shimmed") in to the ACM-XSM module. So I think XSM > can > > If ACM-XSM does not define replacements for the IS_PRIV() hooks, how > are you going to integrate them into ACM-XSM? If so, based on what > information from the current ACM policy would ACM-XSM enforce the > IS_PRIV() check? What if ACM is not active, what enforces IS_PRIV() > then? >I think I am being misunderstood here. In this discussion, I have suggested that we could move the management of the IS_PRIV() checks under XSM. If this were to happen, these checks would be managed as the default behavior of the XSM dummy module as well as be the default behavior of XSM when disabled. If XSM is not enabled, Xen will still behave as expected. On initialization, XSM does a check of a module''s defined hooks, if a module does not define a hook, the equivalent hook from the dummy module is registered. With the exception of the domain_create hook, the hooks used by ACM-XSM are orthogonal to IS_PRIV(). For domain_create, the corresponding ACM-XSM hook would need to add an IS_PRIV() call under this proposal. For the other hooks not used by ACM-XSM, the XSM init process registers the dummy hooks containing the default IS_PRIV() checks. This is how XSM works today, except that the default/dummy hooks currently return "0". George _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
George S. Coker, II
2007-Jun-04 19:06 UTC
[Xen-devel][Xense-devel][PATCH][1/4] Xen Security Modules: XSM
Updates in this patch set include:
- coding style cleanups
- adjustment of interface for xsm_update_va_mapping
(enables direct use of xen macros for pte/pfn/mfn translations)
- implement event channel security_alloc/dealloc to operate on a
single event channel
- remove patch cruft
TODO:
- documentation of the hook interfaces
TBD:
- additional cleanups & management of xsm ops beyond current ifdef
- performance impact of levels of indirection vs. manageability
- compile time removal of all xsm indirection
Signed-off-by: George Coker <gscoker@alpha.ncsc.mil>
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel