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

[Jason Andryuk <jandryuk@xxxxxxxxx>]

On Wed, Aug 17, 2022 at 10:16 AM Daniel P. Smith
<dpsmith@xxxxxxxxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxxxxxxxx> 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