Mailing List Archive

[PATCH v12] xsm: refactor flask sid alloc and domain check
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 v12:
- actually send the changes from v11

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 | 48 ++++++++++++++++++++++--------
3 files changed, 42 insertions(+), 12 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..2c2d393edf 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;
@@ -550,20 +558,36 @@ static int cf_check flask_domain_create(struct domain *d, uint32_t ssidref)
struct domain_security_struct *dsec = d->ssid;
static int dom0_created = 0;

- if ( is_idle_domain(current->domain) && !dom0_created )
+ /*
+ * If the null label is passed, then use the label from security context
+ * allocation. NB: if the label from the allocated security context is also
+ * null, the security server will use unlabeled_t for the domain.
+ */
+ if ( ssidref == 0 )
+ ssidref = dsec->sid;
+
+ /*
+ * First check if the current domain is allowed to create the target domain
+ * type before making changes to the current state.
+ */
+ rc = avc_current_has_perm(ssidref, SECCLASS_DOMAIN, DOMAIN__CREATE, NULL);
+ if ( rc )
+ return rc;
+
+ /*
+ * The dom0_t label is expressed as a singleton label in the base policy.
+ * This cannot be enforced by the security server, therefore it will be
+ * enforced here.
+ */
+ if ( ssidref == SECINITSID_DOM0 )
{
- dsec->sid = SECINITSID_DOM0;
- dom0_created = 1;
+ if ( !dom0_created )
+ dom0_created = 1;
+ else
+ return -EINVAL;
}
- else
- {
- rc = avc_current_has_perm(ssidref, SECCLASS_DOMAIN,
- DOMAIN__CREATE, NULL);
- if ( rc )
- return rc;

- dsec->sid = ssidref;
- }
+ dsec->sid = ssidref;
dsec->self_sid = dsec->sid;

rc = security_transition_sid(dsec->sid, dsec->sid, SECCLASS_DOMAIN,
--
2.20.1
Re: [PATCH v12] xsm: refactor flask sid alloc and domain check [ In reply to ]
Hi,

I think you should change the title to "xsm/flask: Boot-time labeling
for multiple domains". Refactor implies no functional change, and
this is a functional change. With this, I think the commit message
should be re-written to focus on the "why" of the new labeling policy.

On Tue, Aug 9, 2022 at 10:06 AM Daniel P. Smith
<dpsmith@apertussolutions.com> wrote:
>
> 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.

The problem this commit is addressing is "flask can only label a
single dom0_t at boot, and this is incompatible with dom0less and
hyperlaunch".

ISTM that dom0less device tree could gain a node for the security
label, and Hyperlaunch already supports labels. But a goal of this
patch is to make it work without changing dom0less? And it may be
worth more directly stating that dom0less panics today since the domU
fails to build with unlabeled_t.

Also a motivation was to align Flask labels to match the dummy policy
with dom0/domU, correct? That would be worth adding.

> 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.

AFAICT, the labeling policy needs to handle these three cases:
1) Traditional domain 0 (x86 or arm)
Single domain - domid == 0 && privileged

2) dom0less (arm)
Possibly a single dom0 - domid == 0 && privileged
Multiple domUs - domid > 0 && not privileged
Notably, it takes care not to create a domU with domid 0.

3) Hyperlaunch (x86 or arm)
Potentially anything? I don't know what you envision for this.

When it was only dom0, it was easy to put a heuristic in flask to
label the first domain as dom0_t. With dom0less, the heuristic can be
expanded to include domid > 0 -> domU_t. With hyperlaunch, I'm not
sure. Is there something it needs that wouldn't be covered?

dom0_t being a singleton emphasized for me that using only
is_privileged for the check isn't quite right. Does hyperlaunch need
domid != 0 && is_privileged to get assigned dom0_t? That could still
be done explicitly, but just not implicitly by the above.

> 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.

I suppose if someone doesn't want to use domU_t/dom0_t, then they
could remove the xenboot_t allow rules which would defacto require
explicit labels.

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


> --- 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 )

The policy outlined above would change this line to:
if ( d->is_privileged && d->domid == 0 )

> + dsec->sid = SECINITSID_DOM0;
> + else
> + dsec->sid = SECINITSID_DOMU;
> + }
> + else
> + dsec->sid = SECINITSID_UNLABELED;
> }
>
> dsec->self_sid = dsec->sid;
> @@ -550,20 +558,36 @@ static int cf_check flask_domain_create(struct domain *d, uint32_t ssidref)
> struct domain_security_struct *dsec = d->ssid;
> static int dom0_created = 0;
>
> - if ( is_idle_domain(current->domain) && !dom0_created )

This old check only applied at boot time to label the first domain as
dom0_t, but it didn't restrict runtime labeling...

> + /*
> + * The dom0_t label is expressed as a singleton label in the base policy.
> + * This cannot be enforced by the security server, therefore it will be
> + * enforced here.
> + */
> + if ( ssidref == SECINITSID_DOM0 )
> {

...this new one restricts runtime labeling with dom0_t. It's an
unusual case, so making the code change is (probably) fine. But it
should at least be mentioned in the commit message.

However, if the boot time policy adds "domid == 0" to the dom0_t
assignment, then the dom0_created code can go away.

Regards,
Jason
Re: [PATCH v12] xsm: refactor flask sid alloc and domain check [ In reply to ]
On 8/16/22 13:43, Jason Andryuk wrote:
> Hi,
>
> I think you should change the title to "xsm/flask: Boot-time labeling
> for multiple domains". Refactor implies no functional change, and
> this is a functional change. With this, I think the commit message
> should be re-written to focus on the "why" of the new labeling policy.

I can rename and expand a bit further.

> On Tue, Aug 9, 2022 at 10:06 AM Daniel P. Smith
> <dpsmith@apertussolutions.com> wrote:
>>
>> 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.
>
> The problem this commit is addressing is "flask can only label a
> single dom0_t at boot, and this is incompatible with dom0less and
> hyperlaunch".
>
> ISTM that dom0less device tree could gain a node for the security
> label, and Hyperlaunch already supports labels. But a goal of this
> patch is to make it work without changing dom0less? And it may be
> worth more directly stating that dom0less panics today since the domU
> fails to build with unlabeled_t.
>
> Also a motivation was to align Flask labels to match the dummy policy
> with dom0/domU, correct? That would be worth adding.

okay

>> 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.
>
> AFAICT, the labeling policy needs to handle these three cases:
> 1) Traditional domain 0 (x86 or arm)
> Single domain - domid == 0 && privileged

On x86 it cannot be assumed that the domid for the initial domain is
zero(0). See get_initial_domain_id() for which, afaict, is not a valid
call under Arm.

> 2) dom0less (arm)
> Possibly a single dom0 - domid == 0 && privileged
> Multiple domUs - domid > 0 && not privileged
> Notably, it takes care not to create a domU with domid 0.

Just to be pedantic, this really should just be labeled as "arm domain
construction". As I discovered during all of this, Arm is always capable
of building multiple domains at boot. The dom0less construct is really
just a mode of the domain builder to never construct/start the initial
domain (dom0) if the dom0less flag is set, regardless if there is one
defined in the DTB.

> 3) Hyperlaunch (x86 or arm)
> Potentially anything? I don't know what you envision for this.

A simplistic way to state it is, remove all assumptions/conventions
about domain construction by the hypervisor. Instead, require explicit
declarations about what domains the hypervisor should construct.

> When it was only dom0, it was easy to put a heuristic in flask to
> label the first domain as dom0_t. With dom0less, the heuristic can be
> expanded to include domid > 0 -> domU_t. With hyperlaunch, I'm not
> sure. Is there something it needs that wouldn't be covered?

In the current HL series, there is no binding/enforcement/validation of
the boot configuration and requirements of the enforcing XSM module. For
the case when FLASK is the enforcing XSM module, the domain
configuration may not have provided labels for the domains. In this case
it would mean either FLASK refuses/fails the domain create check if a
SID ref is not provided or a sensible policy/heuristic must be codified.

> dom0_t being a singleton emphasized for me that using only
> is_privileged for the check isn't quite right. Does hyperlaunch need
> domid != 0 && is_privileged to get assigned dom0_t? That could still
> be done explicitly, but just not implicitly by the above.

I agree it is not quite right, but more so that it is leveraging the
assumption from the basic policy module (dummy policy) that only the
initial domain (dom0) will have is_privileged set. As stated above,
domid !=0 and is_privileged being set already exists for PV shim, not
something being introduced by HL. HL only expands the possibility for
the configuration to be built outside PV shim.

With that said, unless I am missing something, the heuristic below will
enforce the singleton. While it is possible that
flask_domain_alloc_security() would allocate a security context for more
than one domain containing the label of dom0_t. The
flask_domain_create() check will only allow the first domain with this
label to be created, regardless if the domain create was initiated by
the hypervisor or by a runtime toolstack.

>> 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.
>
> I suppose if someone doesn't want to use domU_t/dom0_t, then they
> could remove the xenboot_t allow rules which would defacto require
> explicit labels.

Correct, this is why I pushed for the rules to be in the policy instead
of in the code. If someone wants to use HL to do something custom, they
can define their desired types, including dropping the dom0_t type, in
policy and explicitly provided labels to domain in the boot configuration.

>> Signed-off-by: Daniel P. Smith <dpsmith@apertussolutions.com>
>
>
>> --- 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 )
>
> The policy outlined above would change this line to:
> if ( d->is_privileged && d->domid == 0 )

As mentioned above, d->domid == 0, is not a valid assumption even before
the introduction of HL.

>> + dsec->sid = SECINITSID_DOM0;
>> + else
>> + dsec->sid = SECINITSID_DOMU;
>> + }
>> + else
>> + dsec->sid = SECINITSID_UNLABELED;
>> }
>>
>> dsec->self_sid = dsec->sid;
>> @@ -550,20 +558,36 @@ static int cf_check flask_domain_create(struct domain *d, uint32_t ssidref)
>> struct domain_security_struct *dsec = d->ssid;
>> static int dom0_created = 0;
>>
>> - if ( is_idle_domain(current->domain) && !dom0_created )
>
> This old check only applied at boot time to label the first domain as
> dom0_t, but it didn't restrict runtime labeling...
>
>> + /*
>> + * The dom0_t label is expressed as a singleton label in the base policy.
>> + * This cannot be enforced by the security server, therefore it will be
>> + * enforced here.
>> + */
>> + if ( ssidref == SECINITSID_DOM0 )
>> {
>
> ...this new one restricts runtime labeling with dom0_t. It's an
> unusual case, so making the code change is (probably) fine. But it
> should at least be mentioned in the commit message.

Correct, this makes a stronger/hard enforcement that only one domain can
ever be labeled as dom0_t. As mentioned, previously it was only checked
when the idle domain was doing the construction. Which made it possible
to construct multiple domains labeled as dom0_t, as long as the policy
was altered to allow a domain type to construction domains of type
dom0_t. IMHO, if there is a desire to enable configurations with
multiple all-privileged domains, then that configuration should define a
different domain type and use HL to have the initial domain constructed
with that type. The type dom0_t is special, mostly to align with
historical expectations/conventions, and should not be repurposed by
custom FLASK policies.

> However, if the boot time policy adds "domid == 0" to the dom0_t
> assignment, then the dom0_created code can go away.

I disagree because "domid == 0" is not a valid check and IMHO the
enforcement of dom0_t being a singleton should be as strong as possible.

> Regards,
> Jason

v/r,
dps
Re: [PATCH v12] xsm: refactor flask sid alloc and domain check [ In reply to ]
On Wed, Aug 17, 2022 at 10:16 AM Daniel P. Smith
<dpsmith@apertussolutions.com> wrote:
>
> On 8/16/22 13:43, Jason Andryuk wrote:
> > Hi,
> >
> > I think you should change the title to "xsm/flask: Boot-time labeling
> > for multiple domains". Refactor implies no functional change, and
> > this is a functional change. With this, I think the commit message
> > should be re-written to focus on the "why" of the new labeling policy.
>
> I can rename and expand a bit further.

Thanks.

> > On Tue, Aug 9, 2022 at 10:06 AM Daniel P. Smith
> > <dpsmith@apertussolutions.com> wrote:

> > dom0_t being a singleton emphasized for me that using only
> > is_privileged for the check isn't quite right. Does hyperlaunch need
> > domid != 0 && is_privileged to get assigned dom0_t? That could still
> > be done explicitly, but just not implicitly by the above.
>
> I agree it is not quite right, but more so that it is leveraging the
> assumption from the basic policy module (dummy policy) that only the
> initial domain (dom0) will have is_privileged set. As stated above,
> domid !=0 and is_privileged being set already exists for PV shim, not
> something being introduced by HL. HL only expands the possibility for
> the configuration to be built outside PV shim.

get_initial_domain_id() had slipped my mind. I had thought a little
bit about the PV shim case in the past, and my guess is no one has
built a PV shim with Flask. Running flask for a single domain under
the PV shim is a little silly. If you did, the domain running under
PV shim would get dom0_t, but it is a domU - A little weird.

Oh, this is interesting:
/* Create initial domain. Not d0 for pvshim. */
domid = get_initial_domain_id();
d = domain_create(domid, &dom0_cfg, pv_shim ? 0 : CDF_privileged);

So the PV shim domain is not privileged, and get_initial_domain_id
will return != 0 for the pv-shim domain. Your change would actually
assign it domU_t.

I think this means the domid == 0 check could still be used. I guess
I'm approaching the problem by trying to restrict the assignment of
dom0_t as much as possible. It's definitely needed for the
traditional dom0 case, but any other use seems suspect.

> With that said, unless I am missing something, the heuristic below will
> enforce the singleton. While it is possible that
> flask_domain_alloc_security() would allocate a security context for more
> than one domain containing the label of dom0_t. The
> flask_domain_create() check will only allow the first domain with this
> label to be created, regardless if the domain create was initiated by
> the hypervisor or by a runtime toolstack.

It's inconsistent to hand out dom0_t twice when it cannot be used
twice. It does the right thing, so it's fine. It just seems
inconsistent.

Anyway, if you really want to move forward with not using the domid, I
guess it's okay.

And after writing all that, dom0_t could be modified to not be non-singleton...

Regards,
Jason