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

[Jürgen Groß <jgross@xxxxxxxx>]

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;

> >   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.

> > +        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,

Juergen

Attachment: OpenPGP_0xB0DE9DD628BF132F.asc
Description: OpenPGP public key

Attachment: OpenPGP_signature.asc
Description: OpenPGP digital signature