Re: [RFC PATCH] xsm: Re-work domain_create and domain_alloc_security

[Jason Andryuk <jandryuk@xxxxxxxxx>]

On Mon, Oct 26, 2020 at 12:23 PM Daniel Smith
<dpsmith@xxxxxxxxxxxxxxxxxxxx> wrote:
>
> ---- On Mon, 26 Oct 2020 09:46:51 -0400 Jason Andryuk <jandryuk@xxxxxxxxx> 
> wrote ----
>
>  > Untested!
>  >
>  > This only really matters for flask, but all of xsm is updated.
>  >
>  > flask_domain_create() and flask_domain_alloc_security() are a strange
>  > pair.
>  >
>  > flask_domain_create() serves double duty.  It both assigns sid and
>  > self_sid values and checks if the calling domain has permission to
>  > create the target domain.  It also has special casing for handling dom0.
>  > Meanwhile flask_domain_alloc_security() assigns some special sids, but
>  > waits for others to be assigned in flask_domain_create.  This split
>  > seems to have come about so that the structures are allocated before
>  > calling flask_domain_create().  It also means flask_domain_create is
>  > called in the middle of domain_create.
>  >
>  > Re-arrange the two calls.  Let flask_domain_create just check if current
>  > has permission to create ssidref.  Then it can be moved out to do_domctl
>  > and gate entry into domain_create.  This avoids doing partial domain
>  > creation before the permission check.
>  >
>  > Have flask_domain_alloc_security() take a ssidref argument.  The ssidref
>  > was already permission checked earlier, so it can just be assigned.
>  > Then the self_sid can be calculated here as well rather than in
>  > flask_domain_create().
>  >
>  > The dom0 special casing is moved into flask_domain_alloc_security().
>  > Maybe this should be just a fall-through for the dom0 already created
>  > case.  This code may not be needed any longer.
>  >
>  > Signed-off-by: Jason Andryuk <jandryuk@xxxxxxxxx>
>  > ---

<snip>

>  > -static int flask_domain_alloc_security(struct domain *d)
>  > +static int flask_domain_alloc_security(struct domain *d, u32 ssidref)
>  >  {
>  >  struct domain_security_struct *dsec;
>  > +    static int dom0_created = 0;
>  > +    int rc;
>  >
>  >  dsec = xzalloc(struct domain_security_struct);
>  >  if ( !dsec )
>  > @@ -175,14 +177,24 @@ static int flask_domain_alloc_security(struct domain 
> *d)
>  >  case DOMID_IO:
>  >  dsec->sid = SECINITSID_DOMIO;
>  >  break;
>  > +    case 0:
>  > +        if ( !dom0_created ) {
>  > +            dsec->sid = SECINITSID_DOM0;
>  > +            dom0_created = 1;
>  > +        } else {
>  > +            dsec->sid = SECINITSID_UNLABELED;
>  > +        }
>
> While the handling of this case is not wrong, I have to wonder if there is a 
> better way to handle the dom0 creation case.

dom0_cfg.ssidref could be set to SECINITSID_DOM0.  I guess that would
need some xsm_ssid_dom0 wrapper.  Then maybe this logic can go away
and the default case used.

pv-shim doesn't necessarily use domid 0, so this may be broken there.
dom0_cfg.ssidref would fix that, I think.  But I'm not familiar with
pv-shim.

>  > +        break;
>  >  default:
>  > -        dsec->sid = SECINITSID_UNLABELED;
>  > +        dsec->sid = ssidref;
>  >  }
>  >
>  >  dsec->self_sid = dsec->sid;
>  > -    d->ssid = dsec;
>
> I don't think you meant to deleted that, without it domains will have no ssid 
> assigned to them.

Yes, this should be retained.

Thanks for looking.

-Jason