diff mbox

[07/17] flask: unify {get, set}vcpucontext permissions

Message ID 1466431466-28055-8-git-send-email-dgdegra@tycho.nsa.gov (mailing list archive)
State New, archived
Headers show

Commit Message

Daniel De Graaf June 20, 2016, 2:04 p.m. UTC 
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 <dgdegra@tycho.nsa.gov>
Reviewed-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
---
 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(-)

Comments

Douglas Goldstein June 20, 2016, 2:27 p.m. UTC | #1 
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 <dgdegra@tycho.nsa.gov>
> Reviewed-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>

I'm a:

Reviewed-by: Doug Goldstein <cardoe@cardoe.com>

But I'd like to see Andrew Cooper's R-b or comments as well.
Andrew Cooper June 20, 2016, 2:35 p.m. UTC | #2 
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 <dgdegra@tycho.nsa.gov>
>> Reviewed-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> I'm a:
>
> Reviewed-by: Doug Goldstein <cardoe@cardoe.com>
>
> 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 <andrew.cooper3@citrix.com>
Daniel De Graaf June 20, 2016, 2:50 p.m. UTC | #3 
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 <dgdegra@tycho.nsa.gov>
>>> Reviewed-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
>> I'm a:
>>
>> Reviewed-by: Doug Goldstein <cardoe@cardoe.com>
>>
>> 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 <andrew.cooper3@citrix.com>

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.
Andrew Cooper June 20, 2016, 2:58 p.m. UTC | #4 
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 <dgdegra@tycho.nsa.gov>
>>>> Reviewed-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
>>> I'm a:
>>>
>>> Reviewed-by: Doug Goldstein <cardoe@cardoe.com>
>>>
>>> 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 <andrew.cooper3@citrix.com>
>
> 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
diff mbox

Patch

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
+# XEN_DOMCTL_get_vcpu_msrs
     getvcpucontext
 # XEN_DOMCTL_max_mem
     setdomainmaxmem
@@ -166,16 +172,6 @@  class domain
     getaddrsize
 # XEN_DOMCTL_sendtrigger
     trigger
-# XEN_DOMCTL_get_ext_vcpucontext
-# XEN_DOMCTL_set_vcpu_msrs
-    getextvcpucontext
-# XEN_DOMCTL_set_ext_vcpucontext
-# XEN_DOMCTL_get_vcpu_msrs
-    setextvcpucontext
-# XEN_DOMCTL_getvcpuextstate
-    getvcpuextstate
-# XEN_DOMCTL_setvcpuextstate
-    setvcpuextstate
 # XENMEM_get_pod_target
     getpodtarget
 # XENMEM_set_pod_target