Re: [RFC PATCH] flask: Remove magic SID setting

["Daniel P. Smith" <dpsmith@xxxxxxxxxxxxxxxxxxxx>]

On 7/7/22 08:45, Jason Andryuk wrote:
> On Thu, Jul 7, 2022 at 6:14 AM Daniel P. Smith
> <dpsmith@xxxxxxxxxxxxxxxxxxxx> wrote:
>>
>> On 7/6/22 15:13, Jason Andryuk wrote:
>>> flask_domain_alloc_security and flask_domain_create has special code to
>>> magically label dom0 as dom0_t.  This can all be streamlined by making
>>> create_dom0 set ssidref before creating dom0.
>>
>> Hmm, I wouldn't call it magical, it is the initialization policy for a
>> domain labeling, which is specific to each policy module. I considered
>> this approach already and my concern here is two fold. First, it now
>> hard codes the concept of dom0 vs domU into the XSM API. There is an
>> ever growing desire by solution providers to not have a dom0 and at most
>> have a hardware domain if at all and this is a step backwards from that
>> movement. Second, and related, is this now pushes the initial label
>> policy up into the domain builder code away from the policy module and
>> spreads it out. Hopefully Xen will evolve to have a richer set of
>> initial domains and an appropriate initial label policy will be needed
>> for this case. This approach will result in having to continually expand
>> the XSM API for each new initial domain type.
> 
> Yeah, adding dom0 vs. domU into the XSM API isn't nice.  My original
> idea was just for dom0, but I added the domU hook after you basically
> said in your other email that dom0less had to work.  There should not
> be any more of these since they are just to provide backwards
> compatibility.

Help me understand, why is it considered magic/undesirable to assign a
label for Dom0/DomU in flask_domain_alloc_security() when the context is
clearly discernible, yet it is acceptable to assign SECINITSID_XENBOOT,
SECINITSID_DOMXEN, and SECINITSID_DOMIO? Specifically, when
flask_domain_alloc_security() is called with the current domain labeled
with SECINITSID_XENBOOT, we know it is creating either a system domain
or a Dom0/DomU. At no time should there be a domain created at this time
that needs to be labeled with SECINITSID_UNLABELED. When the current
domain is no longer SECINITSID_XENBOOT, then the context is no longer
understood and the only safe SID to initializat with is
SECINITSID_UNLABELED.

While I am more than open to listening as to why my opinion/approach may
be flawed, codifying dom0 and/or domU into the XSM API is really a
non-starter for me.

> A dom0/domU flask policy is not interesting for dom0less/hyperlaunch.
> So I don't see why xen/flask needs support for determining sids for
> domains.  If you have dom0less/hyperlaunch + flask, every domain
> should have a ssidref defined in its config when building.  If you
> require ssidrefs for dom0less/hyperlaunch + flask, then there is less
> initial label policy.  An unspecified ssidref defaulting to
> unlabeled_t is fine.

Actually, a Dom0/DomU policy is very interesting for those of us that
would like to see XSM/Flask to be the default policy regardless of the
method of construction for the initial system. A specific test case I
would run was a configuration containing a Dom0 and a DomU without XSM
labels specified. This configuration should Just Work(tm).

> I saw your other patch as adding more "initial label policy" since it
> adds more special cases.  I see requiring an explicit ssidref or
> getting unlabeled_t as a feature.  Automatic labeling seems like a
> misfeature to me.

This is the crux of the problem, you view the XSM API Expansion as label
or fail while viewing the Default Initial Assignment as being automatic
labeling. The reality is that this is not the case and that the end
result between them is exactly the same, just with slightly different
flows to get there. The difference being that the XSM API Expansion has
codified Dom0/DomU into the XSM API and incurred an additional XSM call
on each construction path.

Consider the state sequence for struct domain_security_struct{} of the
two under dom0less where labels cannot be specified,

XSM API Expansion:
 1. xsm_ssidref_{dom0,domU}() -> config->ssidref = SECINITSID_DOM0 or
    SECINITSID_DOMU respectively
 2. xsm_alloc_security_domain() -> d->ssid->sid = SECINITSID_UNLABELED
 3. xsm_domain_create() will always test config->ssidref,
    SECINITSID_DOM0 or SECINITSID_DOMU, because (1) will always set it
    and never as SECINITSID_UNLABELED

Default Initial Assignment:
 1. xsm_alloc_security_domain() -> d->ssid->sid = SECINITSID_DOM0 or
    SECINITSID_DOMU
 2. config->ssidref = NULL
 3. xsm_domain_create() will always test d->ssid->sid, SECINITSID_DOM0
    or SECINITSID_DOMU because of (1) and never as SECINITSID_UNLABELED

Hyperlaunch domain construction works differently, Dom0/DomU was not
codified into the API and where possible the existing Dom0 API
codifications were eliminated. The XSM API Expansion approach would
result in a similar if statement that is in xsm_alloc_security_domain()
under the Default Initial Assignment approach. It would likely occur in
builder_create_domains(). There a check of the domain'a permissions and
functions would occur to then call the appropriate
xsm_ssid_{dom0,domu}() hook.

Maybe some day it will be reasonable to expect labeling as a standard
part of a domain's configuration, and thus acceptable to panic during
boot when it is missing. Unfortunately, that is not today and no matter
how it is dressed, the current model has to be a default label
assignment based on the understanding that the context is boot-time
domain construction.

v/r,
dps