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

[Jason Andryuk <jandryuk@xxxxxxxxx>]

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@xxxxxxxxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxxxxxxxx>


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