[PATCH v11] xsm: refactor flask sid alloc and domain check

Daniel P. Smith posted 1 patch 1 year, 8 months ago
Test gitlab-ci failed
Patches applied successfully (tree, apply log)
git fetch https://gitlab.com/xen-project/patchew/xen tags/patchew/20220803151741.7826-1-dpsmith@apertussolutions.com
There is a newer version of this series
tools/flask/policy/modules/dom0.te |  3 +++
tools/flask/policy/modules/domU.te |  3 +++
xen/xsm/flask/hooks.c              | 35 +++++++++++++++++-------------
3 files changed, 26 insertions(+), 15 deletions(-)
[PATCH v11] xsm: refactor flask sid alloc and domain check
Posted by Daniel P. Smith 1 year, 8 months ago
The function flask_domain_alloc_security() allocates the security context and
assigns an initial SID for the domain under construction. When it came to SID
assignment of the initial domain, flask_domain_alloc_security() would assign
unlabeled_t. Then in flask_domain_create() it would be switched to dom0_t.
This logic worked under the assumption that the first domain constructed would
be the hypervisor constructing dom0 and all other domains would be constructed
by a toolstack, which would provide a SID. The introduction of dom0less and
subsequently hyperlaunch violates this assumption, as non-privileged domain may
be constructed before the initial domain or no initial domain may be
constructed at all. It is not possible currently for dom0less to express domain
labels in the domain configuration, as such the FLASK policy must employ a
sensible initial SID assignment that can differentiate between hypervisor and
toolstack domain construction.  With the introduction of xenboot_t it is now
possible to distinguish when the hypervisor is in the boot state, and thus any
domain construction happening at this time is being initiated by the
hypervisor.

This commit addresses the above situation by using a check to confirm if the
hypervisor is under the xenboot_t context in flask_domain_alloc_security().
When that is the case, it will inspect the domain's is_privileged field to
determine whether an initial label of dom0_t or domU_t should be set for the
domain. The logic for flask_domain_create() was changed to allow the incoming
SID to override the initial label.

The base policy was adjusted to allow the idle domain under the xenboot_t
context the ability to construct domains of both types, dom0_t and domu_t.

Signed-off-by: Daniel P. Smith <dpsmith@apertussolutions.com>
---

Split out from series "Adds starting the idle domain privileged", earlier patches
from series have been committed.

Changes in v11:
- put back dom0_created variable in flask_domain_create() to ensure the
  enforcement that dom0_t is a singleton label

Changes in v10:
- rewrote commit message
- fixed typos
- reworked logic in flask_domain_create() to be simpler and not result in
  changing the domain security struct before the access check fails

 tools/flask/policy/modules/dom0.te |  3 +++
 tools/flask/policy/modules/domU.te |  3 +++
 xen/xsm/flask/hooks.c              | 35 +++++++++++++++++-------------
 3 files changed, 26 insertions(+), 15 deletions(-)

diff --git a/tools/flask/policy/modules/dom0.te b/tools/flask/policy/modules/dom0.te
index 0a63ce15b6..f710ff9941 100644
--- a/tools/flask/policy/modules/dom0.te
+++ b/tools/flask/policy/modules/dom0.te
@@ -75,3 +75,6 @@ admin_device(dom0_t, ioport_t)
 admin_device(dom0_t, iomem_t)
 
 domain_comms(dom0_t, dom0_t)
+
+# Allow the hypervisor to build domains of type dom0_t
+xen_build_domain(dom0_t)
diff --git a/tools/flask/policy/modules/domU.te b/tools/flask/policy/modules/domU.te
index b77df29d56..3f269018f9 100644
--- a/tools/flask/policy/modules/domU.te
+++ b/tools/flask/policy/modules/domU.te
@@ -13,6 +13,9 @@ domain_comms(domU_t, domU_t)
 migrate_domain_out(dom0_t, domU_t)
 domain_self_comms(domU_t)
 
+# Allow the hypervisor to build domains of type domU_t
+xen_build_domain(domU_t)
+
 # Device model for domU_t.  You can define distinct types for device models for
 # domains of other types, or add more make_device_model lines for this type.
 declare_domain(dm_dom_t)
diff --git a/xen/xsm/flask/hooks.c b/xen/xsm/flask/hooks.c
index 8c9cd0f297..cb81e62c52 100644
--- a/xen/xsm/flask/hooks.c
+++ b/xen/xsm/flask/hooks.c
@@ -182,7 +182,15 @@ static int cf_check flask_domain_alloc_security(struct domain *d)
         dsec->sid = SECINITSID_DOMIO;
         break;
     default:
-        dsec->sid = SECINITSID_UNLABELED;
+        if ( domain_sid(current->domain) == SECINITSID_XENBOOT )
+        {
+            if ( d->is_privileged )
+                dsec->sid = SECINITSID_DOM0;
+            else
+                dsec->sid = SECINITSID_DOMU;
+        }
+        else
+            dsec->sid = SECINITSID_UNLABELED;
     }
 
     dsec->self_sid = dsec->sid;
@@ -548,22 +556,19 @@ static int cf_check flask_domain_create(struct domain *d, uint32_t ssidref)
 {
     int rc;
     struct domain_security_struct *dsec = d->ssid;
-    static int dom0_created = 0;
 
-    if ( is_idle_domain(current->domain) && !dom0_created )
-    {
-        dsec->sid = SECINITSID_DOM0;
-        dom0_created = 1;
-    }
-    else
-    {
-        rc = avc_current_has_perm(ssidref, SECCLASS_DOMAIN,
-                          DOMAIN__CREATE, NULL);
-        if ( rc )
-            return rc;
+    /*
+     * If the null label is passed, then use the label from security context
+     * allocation.
+     */
+    if ( ssidref == 0 )
+        ssidref = dsec->sid;
 
-        dsec->sid = ssidref;
-    }
+    rc = avc_current_has_perm(ssidref, SECCLASS_DOMAIN, DOMAIN__CREATE, NULL);
+    if ( rc )
+        return rc;
+
+    dsec->sid = ssidref;
     dsec->self_sid = dsec->sid;
 
     rc = security_transition_sid(dsec->sid, dsec->sid, SECCLASS_DOMAIN,
-- 
2.20.1


Re: [PATCH v11] xsm: refactor flask sid alloc and domain check
Posted by Jan Beulich 1 year, 8 months ago
On 03.08.2022 17:17, Daniel P. Smith wrote:
> Changes in v11:
> - put back dom0_created variable in flask_domain_create() to ensure the
>   enforcement that dom0_t is a singleton label

Stale patch or bad rev log?

> @@ -548,22 +556,19 @@ static int cf_check flask_domain_create(struct domain *d, uint32_t ssidref)
>  {
>      int rc;
>      struct domain_security_struct *dsec = d->ssid;
> -    static int dom0_created = 0;

The variable is going away here, and it is not re-appearing elsewhere.

Jan

> -    if ( is_idle_domain(current->domain) && !dom0_created )
> -    {
> -        dsec->sid = SECINITSID_DOM0;
> -        dom0_created = 1;
> -    }
> -    else
> -    {
> -        rc = avc_current_has_perm(ssidref, SECCLASS_DOMAIN,
> -                          DOMAIN__CREATE, NULL);
> -        if ( rc )
> -            return rc;
> +    /*
> +     * If the null label is passed, then use the label from security context
> +     * allocation.
> +     */
> +    if ( ssidref == 0 )
> +        ssidref = dsec->sid;
>  
> -        dsec->sid = ssidref;
> -    }
> +    rc = avc_current_has_perm(ssidref, SECCLASS_DOMAIN, DOMAIN__CREATE, NULL);
> +    if ( rc )
> +        return rc;
> +
> +    dsec->sid = ssidref;
>      dsec->self_sid = dsec->sid;
>  
>      rc = security_transition_sid(dsec->sid, dsec->sid, SECCLASS_DOMAIN,
Re: [PATCH v11] xsm: refactor flask sid alloc and domain check
Posted by Daniel P. Smith 1 year, 8 months ago
On 8/3/22 11:26, Jan Beulich wrote:
> On 03.08.2022 17:17, Daniel P. Smith wrote:
>> Changes in v11:
>> - put back dom0_created variable in flask_domain_create() to ensure the
>>   enforcement that dom0_t is a singleton label
> 
> Stale patch or bad rev log?

Nope, having a bad day. Staged the change but didn't amend commit it
before my usual sequence of test and cut patch. My apologies.

>> @@ -548,22 +556,19 @@ static int cf_check flask_domain_create(struct domain *d, uint32_t ssidref)
>>  {
>>      int rc;
>>      struct domain_security_struct *dsec = d->ssid;
>> -    static int dom0_created = 0;
> 
> The variable is going away here, and it is not re-appearing elsewhere.
> 
> Jan
> 
>> -    if ( is_idle_domain(current->domain) && !dom0_created )
>> -    {
>> -        dsec->sid = SECINITSID_DOM0;
>> -        dom0_created = 1;
>> -    }
>> -    else
>> -    {
>> -        rc = avc_current_has_perm(ssidref, SECCLASS_DOMAIN,
>> -                          DOMAIN__CREATE, NULL);
>> -        if ( rc )
>> -            return rc;
>> +    /*
>> +     * If the null label is passed, then use the label from security context
>> +     * allocation.
>> +     */
>> +    if ( ssidref == 0 )
>> +        ssidref = dsec->sid;
>>  
>> -        dsec->sid = ssidref;
>> -    }
>> +    rc = avc_current_has_perm(ssidref, SECCLASS_DOMAIN, DOMAIN__CREATE, NULL);
>> +    if ( rc )
>> +        return rc;
>> +
>> +    dsec->sid = ssidref;
>>      dsec->self_sid = dsec->sid;
>>  
>>      rc = security_transition_sid(dsec->sid, dsec->sid, SECCLASS_DOMAIN,
>