Re: [PATCH v3 1/2] xsm: create idle domain privileged and demote after setup

["Roger Pau Monné" <roger.pau@citrix.com>]

On Thu, Apr 28, 2022 at 10:57:42AM -0400, Daniel P. Smith wrote:
> On 4/26/22 03:12, Roger Pau Monné wrote:
> > On Mon, Apr 25, 2022 at 12:39:17PM -0400, Daniel P. Smith wrote:
> >> On 4/25/22 05:44, Roger Pau Monné wrote:
> >>> On Fri, Apr 22, 2022 at 12:34:57PM -0400, Daniel P. Smith wrote:
> >>>> diff --git a/xen/xsm/flask/hooks.c b/xen/xsm/flask/hooks.c
> >>>> index 0bf63ffa84..8a62de2fd6 100644
> >>>> --- a/xen/xsm/flask/hooks.c
> >>>> +++ b/xen/xsm/flask/hooks.c
> >>>> @@ -186,6 +186,26 @@ static int cf_check flask_domain_alloc_security(struct domain *d)
> >>>>      return 0;
> >>>>  }
> >>>>  
> >>>> +static int cf_check flask_set_system_active(void)
> >>>> +{
> >>>> +    struct domain *d = current->domain;
> >>>
> >>> Nit: you should also add the assert for d->is_privileged, I don't see
> >>> a reason for the xsm and flask functions to differ in that regard.
> >>
> >> This goes back to an issued I have raised before, is_privileged really
> >> encompasses two properties of a domain. Whether the domain is filling
> >> the special control domain role versus what accesses the domain has
> >> based on the context under which is_control_domain() is called. For
> >> instance the function init_domain_msr_policy() uses is_control_domain()
> >> not to make an access control decision but configure behavior. Under
> >> flask is_privileged no longer reflects the accesses a domain with it set
> >> will have, thus whether it is cleared when flask is enabled is
> >> irrelevant as far as flask is concerned. For the ASSERT, what matters is
> >> that the label was set to xenboot_t on construction and that it was not
> >> changed before reaching this point. Or in a short form, when under the
> >> default policy the expected state is concerned with is_privilege while
> >> for flask it is only the SID.
> > 
> > I certainly don't care that much, but you do set d->is_privileged =
> > false in flask_set_system_active, hence it would seem logic to expect
> > d->is_privileged == true also?
> 
> Yes, I did this just for consistency not because there is any
> significance of is_privilege on the idle domain, in both contexts for
> which is_privileged is used, when flask is the enforcing policy.
> 
> > If not for anything else, just to assert that the function is not
> > called twice.
> 
> Under this patch flask_set_system_active() is effectively a no-op, so
> calling it twice has no effect. In the next patch flask_set_system()
> becomes a real check and there is an ASSERT on the SID as that is the
> relevant context under flask and will ensure calling only once.
> 
> In the end I can add the ASSERT but it would be adding it for the sake
> of adding it as it would not be protecting the hypervisor from moving
> into an incorrect state.

If flask_set_system_active() is really a no-op then just adding a
comment in that regard and not touching is_privileged would be OK to
me, as otherwise I think it's confusing.

In any case I would leave that to the flask maintainers to decide.

Thanks, Roger.