|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel][Xense-devel][PATCH][1/4] Xen Security Modules: XSM
* George S. Coker, II (gscoker@xxxxxxxxxxxxxx) 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
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;
}
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@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |