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

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

On 24.10.24 15:35, Daniel P. Smith wrote:
> 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.
Sounds logical.

I'll check if it works the way you are suggesting. If so, I'll send a patch
fixing the getdomaininfo case.


Juergen

Attachment: OpenPGP_0xB0DE9DD628BF132F.asc
Description: OpenPGP public key

Attachment: OpenPGP_signature.asc
Description: OpenPGP digital signature