[Xen-devel] [PATCH 07/17] flask: unify {get, set}vcpucontext permissions
These permissions were initially split because they were in separate domctls, but this split is very unlikely to actually provide security benefits: it would require a carefully contrived situation for a domain to both need access to one type of CPU register and also need to be prohibited from accessing another type. Signed-off-by: Daniel De Graaf Reviewed-by: Konrad Rzeszutek Wilk --- tools/flask/policy/modules/dom0.te | 1 - tools/flask/policy/modules/xen.if | 7 +++ xen/xsm/flask/hooks.c | 20 ++-- xen/xsm/flask/policy/access_vectors | 16 ++-- 4 files changed, 15 insertions(+), 29 deletions(-) diff --git a/tools/flask/policy/modules/dom0.te b/tools/flask/policy/modules/dom0.te index ef6a986..d228b24 100644 --- a/tools/flask/policy/modules/dom0.te +++ b/tools/flask/policy/modules/dom0.te @@ -34,7 +34,6 @@ allow dom0_t dom0_t:domain { setvcpucontext max_vcpus setaffinity getaffinity getscheduler getdomaininfo getvcpuinfo getvcpucontext setdomainmaxmem setdomainhandle setdebugging hypercall settime setaddrsize getaddrsize trigger - getextvcpucontext setextvcpucontext getvcpuextstate setvcpuextstate getpodtarget setpodtarget set_misc_info set_virq_handler }; allow dom0_t dom0_t:domain2 { diff --git a/tools/flask/policy/modules/xen.if b/tools/flask/policy/modules/xen.if index 00d1bbb..fd96303 100644 --- a/tools/flask/policy/modules/xen.if +++ b/tools/flask/policy/modules/xen.if @@ -47,9 +47,8 @@ define(`declare_build_label', ` define(`create_domain_common', ` allow $1 $2:domain { create max_vcpus setdomainmaxmem setaddrsize - getdomaininfo hypercall setvcpucontext setextvcpucontext - getscheduler getvcpuinfo getvcpuextstate getaddrsize - getaffinity setaffinity setvcpuextstate }; + getdomaininfo hypercall setvcpucontext getscheduler + getvcpuinfo getaddrsize getaffinity setaffinity }; allow $1 $2:domain2 { set_cpuid settsc setscheduler setclaim set_max_evtchn set_vnumainfo get_vnumainfo cacheflush psr_cmt_op psr_cat_op soft_reset }; @@ -94,7 +93,7 @@ define(`migrate_domain_out', ` allow $1 domxen_t:mmu map_read; allow $1 $2:hvm { gethvmc getparam irqlevel }; allow $1 $2:mmu { stat pageinfo map_read }; - allow $1 $2:domain { getaddrsize getvcpucontext getextvcpucontext getvcpuextstate pause destroy }; + allow $1 $2:domain { getaddrsize getvcpucontext pause destroy }; allow $1 $2:domain2 gettsc; allow $1 $2:shadow { enable disable logdirty }; ') diff --git a/xen/xsm/flask/hooks.c b/xen/xsm/flask/hooks.c index 20d46c8..a8d45e7 100644 --- a/xen/xsm/flask/hooks.c +++ b/xen/xsm/flask/hooks.c @@ -630,10 +630,16 @@ static int flask_domctl(struct domain *d, int cmd) case XEN_DOMCTL_setdomainhandle: return current_has_perm(d, SECCLASS_DOMAIN, DOMAIN__SETDOMAINHANDLE); +case XEN_DOMCTL_set_ext_vcpucontext: +case XEN_DOMCTL_set_vcpu_msrs: case XEN_DOMCTL_setvcpucontext: +case XEN_DOMCTL_setvcpuextstate: return current_has_perm(d, SECCLASS_DOMAIN, DOMAIN__SETVCPUCONTEXT); +case XEN_DOMCTL_get_ext_vcpucontext: +case XEN_DOMCTL_get_vcpu_msrs: case XEN_DOMCTL_getvcpucontext: +case XEN_DOMCTL_getvcpuextstate: return current_has_perm(d, SECCLASS_DOMAIN, DOMAIN__GETVCPUCONTEXT); case XEN_DOMCTL_getvcpuinfo: @@ -675,20 +681,6 @@ static int flask_domctl(struct domain *d, int cmd) case XEN_DOMCTL_pin_mem_cacheattr: return current_has_perm(d, SECCLASS_HVM, HVM__CACHEATTR); -case XEN_DOMCTL_set_ext_vcpucontext: -case XEN_DOMCTL_set_vcpu_msrs: -return current_has_perm(d, SECCLASS_DOMAIN, DOMAIN__SETEXTVCPUCONTEXT); - -case XEN_DOMCTL_get_ext_vcpucontext: -case XEN_DOMCTL_get_vcpu_msrs: -return current_has_perm(d, SECCLASS_DOMAIN, DOMAIN__GETEXTVCPUCONTEXT); - -case XEN_DOMCTL_setvcpuextstate: -return current_has_perm(d, SECCLASS_DOMAIN, DOMAIN__SETVCPUEXTSTATE); - -case XEN_DOMCTL_getvcpuextstate: -return current_has_perm(d, SECCLASS_DOMAIN, DOMAIN__GETVCPUEXTSTATE); - case XEN_DOMCTL_sendtrigger: return current_has_perm(d, SECCLASS_DOMAIN, DOMAIN__TRIGGER); diff --git a/xen/xsm/flask/policy/access_vectors b/xen/xsm/flask/policy/access_vectors index 3d29042..7e69ede 100644 --- a/xen/xsm/flask/policy/access_vectors +++ b/xen/xsm/flask/policy/access_vectors @@ -111,6 +111,9 @@ class xen2 class domain { # XEN_DOMCTL_setvcpucontext +# XEN_DOMCTL_setvcpuextstate +# XEN_DOMCTL_set_ext_vcpucontext +# XEN_DOMCTL_set_vcpu_msrs setvcpucontext # XEN_DOMCTL_pausedomain pause @@ -142,6 +145,9 @@ class domain # XEN_DOMCTL_getvcpuinfo getvcpuinfo # XEN_DOMCTL_getvcpucontext +# XEN_DOMCTL_get_ext_vcpucontext +# XEN_DOMCTL_getvcpuextstate +#
Re: [Xen-devel] [PATCH 07/17] flask: unify {get, set}vcpucontext permissions
On 6/20/16 9:04 AM, Daniel De Graaf wrote: > These permissions were initially split because they were in separate > domctls, but this split is very unlikely to actually provide security > benefits: it would require a carefully contrived situation for a domain > to both need access to one type of CPU register and also need to be > prohibited from accessing another type. > > Signed-off-by: Daniel De Graaf > Reviewed-by: Konrad Rzeszutek Wilk I'm a: Reviewed-by: Doug Goldstein But I'd like to see Andrew Cooper's R-b or comments as well. -- Doug Goldstein signature.asc Description: OpenPGP digital signature ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH 07/17] flask: unify {get, set}vcpucontext permissions
On 20/06/16 15:27, Doug Goldstein wrote: > On 6/20/16 9:04 AM, Daniel De Graaf wrote: >> These permissions were initially split because they were in separate >> domctls, but this split is very unlikely to actually provide security >> benefits: it would require a carefully contrived situation for a domain >> to both need access to one type of CPU register and also need to be >> prohibited from accessing another type. >> >> Signed-off-by: Daniel De Graaf >> Reviewed-by: Konrad Rzeszutek Wilk > I'm a: > > Reviewed-by: Doug Goldstein > > But I'd like to see Andrew Cooper's R-b or comments as well. > I agree. I can't see a plausible usecase for an entity being entitled to read vcpu content, but not to modify it. Reviewed-by: Andrew Cooper ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH 07/17] flask: unify {get, set}vcpucontext permissions
On 06/20/2016 10:35 AM, Andrew Cooper wrote: On 20/06/16 15:27, Doug Goldstein wrote: On 6/20/16 9:04 AM, Daniel De Graaf wrote: These permissions were initially split because they were in separate domctls, but this split is very unlikely to actually provide security benefits: it would require a carefully contrived situation for a domain to both need access to one type of CPU register and also need to be prohibited from accessing another type. Signed-off-by: Daniel De Graaf Reviewed-by: Konrad Rzeszutek Wilk I'm a: Reviewed-by: Doug Goldstein But I'd like to see Andrew Cooper's R-b or comments as well. I agree. I can't see a plausible usecase for an entity being entitled to read vcpu content, but not to modify it. Reviewed-by: Andrew Cooper That's not exactly what this patch does: the get and set permissions are still split, but unified across the different types of registers. Where previously there were 6 permissions, now there are 2. A use case where you would be entitled to read but not modify is a monitoring domain (remote virus scanner, for example) which needs read access to scan but does not do remediation itself. -- Daniel De Graaf National Security Agency ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH 07/17] flask: unify {get, set}vcpucontext permissions
On 20/06/16 15:50, Daniel De Graaf wrote: > On 06/20/2016 10:35 AM, Andrew Cooper wrote: >> On 20/06/16 15:27, Doug Goldstein wrote: >>> On 6/20/16 9:04 AM, Daniel De Graaf wrote: These permissions were initially split because they were in separate domctls, but this split is very unlikely to actually provide security benefits: it would require a carefully contrived situation for a domain to both need access to one type of CPU register and also need to be prohibited from accessing another type. Signed-off-by: Daniel De Graaf Reviewed-by: Konrad Rzeszutek Wilk >>> I'm a: >>> >>> Reviewed-by: Doug Goldstein >>> >>> But I'd like to see Andrew Cooper's R-b or comments as well. >>> >> >> I agree. I can't see a plausible usecase for an entity being entitled >> to read vcpu content, but not to modify it. >> >> Reviewed-by: Andrew Cooper > > That's not exactly what this patch does: the get and set permissions > are still split, but unified across the different types of registers. > Where previously there were 6 permissions, now there are 2. The boundaries for those hypercalls were somewhat arbitrary, and definitely awkward to use. Some information is duplicated between them. I plan to make them all disappear, in favour of something more consistent when altering the migration stream semantics. ~Andrew ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel