[PATCH for-4.5] xsm/flask: add two missing domctls

[PATCH for-4.5] xsm/flask: add two missing domctls

[Daniel De Graaf]

Reported-by: Michael Young <m.a.young@durham.ac.uk>
Signed-off-by: Daniel De Graaf <dgdegra@tycho.nsa.gov>
---
 xen/xsm/flask/hooks.c               | 2 ++
 xen/xsm/flask/policy/access_vectors | 2 ++
 2 files changed, 4 insertions(+)

diff --git a/xen/xsm/flask/hooks.c b/xen/xsm/flask/hooks.c
index 0ba2ce9..d48463f 100644
--- a/xen/xsm/flask/hooks.c
+++ b/xen/xsm/flask/hooks.c
@@ -672,9 +672,11 @@ static int flask_domctl(struct domain *d, int cmd)
         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:
diff --git a/xen/xsm/flask/policy/access_vectors b/xen/xsm/flask/policy/access_vectors
index 1cd451e..1da9f63 100644
--- a/xen/xsm/flask/policy/access_vectors
+++ b/xen/xsm/flask/policy/access_vectors
@@ -151,8 +151,10 @@ class domain
 # 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
-- 
1.9.3

Re: [PATCH for-4.5] xsm/flask: add two missing domctls

[Andrew Cooper]

On 25/11/14 16:57, Daniel De Graaf wrote:
> Reported-by: Michael Young <m.a.young@durham.ac.uk>
> Signed-off-by: Daniel De Graaf <dgdegra@tycho.nsa.gov>

Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>

CC'd Konrad, as this should be accepted into Xen-4.5.  Without it,
migration/suspend fails with -EPERM in the default case when XSM is
compiled into Xen.

Daniel: there are 4 hypercalls for getting/setting bits of PV VCPU state:

XEN_DOMCTL_{get,set}vcpucontext
XEN_DOMCTL_{get,set}_ext_vcpucontext
XEN_DOMCTL_{get,set}vcpuextstate
XEN_DOMCTL_{get,set}_vcpu_msrs

I see no reason for these to have separate access vectors; you typically
either need to use all of them, or none, but I presume it is too late to
coalesce the vectors in a backwards compatible way?

~Andrew

> ---
>  xen/xsm/flask/hooks.c               | 2 ++
>  xen/xsm/flask/policy/access_vectors | 2 ++
>  2 files changed, 4 insertions(+)
>
> diff --git a/xen/xsm/flask/hooks.c b/xen/xsm/flask/hooks.c
> index 0ba2ce9..d48463f 100644
> --- a/xen/xsm/flask/hooks.c
> +++ b/xen/xsm/flask/hooks.c
> @@ -672,9 +672,11 @@ static int flask_domctl(struct domain *d, int cmd)
>          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:
> diff --git a/xen/xsm/flask/policy/access_vectors b/xen/xsm/flask/policy/access_vectors
> index 1cd451e..1da9f63 100644
> --- a/xen/xsm/flask/policy/access_vectors
> +++ b/xen/xsm/flask/policy/access_vectors
> @@ -151,8 +151,10 @@ class domain
>  # 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

Re: [PATCH for-4.5] xsm/flask: add two missing domctls

[Konrad Rzeszutek Wilk]

On Tue, Nov 25, 2014 at 06:19:05PM +0000, Andrew Cooper wrote:
> On 25/11/14 16:57, Daniel De Graaf wrote:
> > Reported-by: Michael Young <m.a.young@durham.ac.uk>
> > Signed-off-by: Daniel De Graaf <dgdegra@tycho.nsa.gov>
> 
> Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>
> 
> CC'd Konrad, as this should be accepted into Xen-4.5.  Without it,
> migration/suspend fails with -EPERM in the default case when XSM is
> compiled into Xen.

Yup. Release-Acked-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> 
> Daniel: there are 4 hypercalls for getting/setting bits of PV VCPU state:
> 
> XEN_DOMCTL_{get,set}vcpucontext
> XEN_DOMCTL_{get,set}_ext_vcpucontext
> XEN_DOMCTL_{get,set}vcpuextstate
> XEN_DOMCTL_{get,set}_vcpu_msrs
> 
> I see no reason for these to have separate access vectors; you typically
> either need to use all of them, or none, but I presume it is too late to
> coalesce the vectors in a backwards compatible way?
> 
> ~Andrew
> 
> > ---
> >  xen/xsm/flask/hooks.c               | 2 ++
> >  xen/xsm/flask/policy/access_vectors | 2 ++
> >  2 files changed, 4 insertions(+)
> >
> > diff --git a/xen/xsm/flask/hooks.c b/xen/xsm/flask/hooks.c
> > index 0ba2ce9..d48463f 100644
> > --- a/xen/xsm/flask/hooks.c
> > +++ b/xen/xsm/flask/hooks.c
> > @@ -672,9 +672,11 @@ static int flask_domctl(struct domain *d, int cmd)
> >          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:
> > diff --git a/xen/xsm/flask/policy/access_vectors b/xen/xsm/flask/policy/access_vectors
> > index 1cd451e..1da9f63 100644
> > --- a/xen/xsm/flask/policy/access_vectors
> > +++ b/xen/xsm/flask/policy/access_vectors
> > @@ -151,8 +151,10 @@ class domain
> >  # 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
> 
> 

Re: [PATCH for-4.5] xsm/flask: add two missing domctls

[Daniel De Graaf]

On 11/25/2014 01:19 PM, Andrew Cooper wrote:
> On 25/11/14 16:57, Daniel De Graaf wrote:
>> Reported-by: Michael Young <m.a.young@durham.ac.uk>
>> Signed-off-by: Daniel De Graaf <dgdegra@tycho.nsa.gov>
>
> Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>
>
> CC'd Konrad, as this should be accepted into Xen-4.5.  Without it,
> migration/suspend fails with -EPERM in the default case when XSM is
> compiled into Xen.

Thanks, for some reason I blanked on the CC for the freeze exception.

> Daniel: there are 4 hypercalls for getting/setting bits of PV VCPU state:
>
> XEN_DOMCTL_{get,set}vcpucontext
> XEN_DOMCTL_{get,set}_ext_vcpucontext
> XEN_DOMCTL_{get,set}vcpuextstate
> XEN_DOMCTL_{get,set}_vcpu_msrs
>
> I see no reason for these to have separate access vectors; you typically
> either need to use all of them, or none, but I presume it is too late to
> coalesce the vectors in a backwards compatible way?
>
> ~Andrew

Because the security policy in Xen is kept inside the tree, it is
possible to change them in the future - though this is certainly a topic
for v4.6.  This will cause anyone who has defined their own security
policy to need to modify it, but this is already true when new
permissions are being defined, and it is easier to remove permissions
(just fix the policy compile error) than it is to add them (which either
requires thought on who needs to be allowed access to a permission or
testing to discover the AVC denials).  If a custom policy is using the
macros defined in xen.if, these changes will be applied transparently.

I agree combining these four domctls into a single pair of permissions
is useful (retaining the get/set split); I cannot think of any case
where someone might have a use for one type of CPU/register state and at
the same time cannot be trusted with the others.  This would also
simplify future additions of new types of CPU state.

-- 
Daniel De Graaf
National Security Agency