Re: [PATCH 3/6] xen: add new domctl get_changed_domain

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

On 10/24/24 05:13, Jürgen Groß wrote:
> On 23.10.24 17:55, Daniel P. Smith wrote:
> > On 10/23/24 09:10, Juergen Gross wrote:
> > > Add a new domctl sub-function to get data of a domain having changed
> > > state (this is needed by Xenstore).
> > >
> > > The returned state just contains the domid, the domain unique id,
> > > and some flags (existing, shutdown, dying).
> > >
> > > In order to enable Xenstore stubdom being built for multiple Xen
> > > versions, make this domctl stable.  For stable domctls the
> > > interface_version is specific to the respective domctl op and it is an
> > > in/out parameter: On input the caller is specifying the desired version
> > > of the op, while on output the hypervisor will return the used version
> > > (this will be at max the caller supplied version, but might be lower in
> > > case the hypervisor doesn't support this version).
> > >
> > > Signed-off-by: Juergen Gross <jgross@xxxxxxxx>
> > > ---
> > > V1:
> > > - use a domctl subop for the new interface (Jan Beulich)
> > > ---
> > >   tools/flask/policy/modules/dom0.te  |  2 +-
> > >   xen/common/domain.c                 | 51 +++++++++++++++++++++++++++++
> > >   xen/common/domctl.c                 | 19 ++++++++++-
> > >   xen/common/event_channel.c          |  9 ++++-
> > >   xen/include/public/domctl.h         | 33 +++++++++++++++++++
> > >   xen/include/xen/event.h             |  6 ++++
> > >   xen/include/xen/sched.h             |  2 ++
> > >   xen/include/xsm/dummy.h             |  8 +++++
> > >   xen/include/xsm/xsm.h               |  6 ++++
> > >   xen/xsm/dummy.c                     |  1 +
> > >   xen/xsm/flask/hooks.c               |  7 ++++
> > >   xen/xsm/flask/policy/access_vectors |  2 ++
> > >   12 files changed, 143 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/tools/flask/policy/modules/dom0.te 
> > > b/tools/flask/policy/modules/dom0.te
> > > index 16b8c9646d..6043c01b12 100644
> > > --- a/tools/flask/policy/modules/dom0.te
> > > +++ b/tools/flask/policy/modules/dom0.te
> > > @@ -40,7 +40,7 @@ allow dom0_t dom0_t:domain {
> > >   };
> > >   allow dom0_t dom0_t:domain2 {
> > >       set_cpu_policy gettsc settsc setscheduler set_vnumainfo
> > > -    get_vnumainfo psr_cmt_op psr_alloc get_cpu_policy
> > > +    get_vnumainfo psr_cmt_op psr_alloc get_cpu_policy get_domain_state
> > I don't think that is where you want it, as that restricts dom0 to 
> > only being able to make that call against dom0. The question I have 
> > is, are you looking for this permission to be explicitly assigned to 
> > dom0 or to the domain type that was allowed to create the domain. 
> > IMHO, I think you would want the latter, so not only should the 
> > permission go here, but also added to xen.if:create_domain_common.
> > Additionally, I would also recommend adding the following to xenstore.te:
> >
> > allow xenstore_t domain_type:domain get_domain_state
> Okay, but shouldn't this be:
>
> allow xenstore_t domain_type:domain2 get_domain_state;
Apologies, yes that was a typo on my part.

> > >   allow dom0_t dom0_t:resource { add remove };
> > ...
> >
> > > @@ -866,6 +873,16 @@ long 
> > > do_domctl(XEN_GUEST_HANDLE_PARAM(xen_domctl_t) u_domctl)
> > >                   __HYPERVISOR_domctl, "h", u_domctl);
> > >           break;
> > > +    case XEN_DOMCTL_get_domain_state:
> > > +        ret = xsm_get_domain_state(XSM_HOOK, d);
> > XSM_HOOK will allow any domain to make this call on any domain. What I 
> > think you want here is XSM_XS_PRIV. That will allow either a domain 
> > flagged as the xenstore domain or dom0 to make the call.
> I thought so, too, but looking at the "getdomaininfo" example it seems
> to be okay this way, too. Especially with the addition to xsm_domctl()
> checking for XSM_XS_PRIV.
I know this has been done in the past, but imho it is not a very good 
practice. Access checks really should always restrict equal or down. 
There should be a strong reason, which probably would deserves a code 
comment, to allow a check to relax up. Restricting equal should not 
reduce access, and if it does, then it means there is an unintended 
access path which is now exposed.
> > > +        if ( ret )
> > > +            break;
> > > +
> > > +        copyback = 1;
> > > +        op->interface_version = XEN_DOMCTL_GETDOMSTATE_VERS_MAX;
> > > +        ret = get_domain_state(&op->u.get_domain_state, d);
> > > +        break;
> > > +
> > >       default:
> > >           ret = arch_do_domctl(op, d, u_domctl);
> > >           break;
> > ...
> >
> > > @@ -815,6 +816,13 @@ static XSM_INLINE int cf_check xsm_argo_send(
> > >   #endif /* CONFIG_ARGO */
> > > +static XSM_INLINE int cf_check xsm_get_domain_state(
> > > +    XSM_DEFAULT_ARG struct domain *d)
> > > +{
> > > +    XSM_ASSERT_ACTION(XSM_HOOK);
> > Per the above, this would need changed to XSM_XS_PRIV.
> >
> > > +    return xsm_default_action(action, current->domain, d);
> > > +}
> > > +
> > >   #include <public/version.h>
> > >   static XSM_INLINE int cf_check xsm_xen_version(XSM_DEFAULT_ARG 
> > > uint32_t op)
> > >   {
> > ...
> >
> > > @@ -1853,6 +1854,11 @@ static int cf_check flask_argo_send(
> > >   #endif
> > > +static int cf_check flask_get_domain_state(struct domain *d)
> > > +{
> > > +    return current_has_perm(d, SECCLASS_DOMAIN, 
> > > DOMAIN__GET_DOMAIN_STATE);
> > I believe you want SECCLASS_DOMAIN2 here.
> Oh, indeed. And probably DOMAIN2__GET_DOMAIN_STATE
>
>
> Thanks,
No problem.

v/r,
dps