Re: [PATCH 3/3] hvm/pirq: allow control domains usage of PHYSDEVOP_{un,}map_pirq

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

On Thu, Mar 03, 2022 at 10:45:54AM +0000, Andrew Cooper wrote:
> On 03/03/2022 10:30, Roger Pau Monne wrote:
> > Control domains (including domains having control over a single other
> > guest) need access to PHYSDEVOP_{un,}map_pirq in order to setup
> > bindings of interrupts from devices assigned to the controlled guest.
> >
> > As such relax the check for HVM based guests and allow the usage of
> > the hypercalls for any control domains. Note that further safety
> > checks will be performed in order to assert that the current domain
> > has the right permissions against the target of the hypercall.
> >
> > Reported-by: Alex Olson <this.is.a0lson@xxxxxxxxx>
> > Reported-by: Andrew Cooper <Andrew.Cooper3@xxxxxxxxxx>
> > Signed-off-by: Roger Pau Monné <roger.pau@xxxxxxxxxx>
> > ---
> >  xen/arch/x86/hvm/hypercall.c | 7 +++++++
> >  1 file changed, 7 insertions(+)
> >
> > diff --git a/xen/arch/x86/hvm/hypercall.c b/xen/arch/x86/hvm/hypercall.c
> > index 030243810e..9128e4d025 100644
> > --- a/xen/arch/x86/hvm/hypercall.c
> > +++ b/xen/arch/x86/hvm/hypercall.c
> > @@ -87,6 +87,13 @@ static long hvm_physdev_op(int cmd, 
> > XEN_GUEST_HANDLE_PARAM(void) arg)
> >      {
> >      case PHYSDEVOP_map_pirq:
> >      case PHYSDEVOP_unmap_pirq:
> > +        /*
> > +         * Control domain (and domains controlling others) need to use
> > +         * PHYSDEVOP_{un,}map_pirq in order to setup interrupts for 
> > passthrough
> > +         * devices on behalf of other guests.
> > +         */
> > +        if ( is_control_domain(currd) || currd->target )
> > +            break;
> 
> Hmm.  In a split control/hardware domain model, then qemu is in the
> hardware domain rather than the control domain.  I suspect this wants
> extending with || is_hardware_domain(currd).

The binding of GSIs is exclusively done by the control domain because
those are static and known at domain creation.  The mapping and
binding of MSI interrupts is however done by QEMU at runtime, so it
needs extending to the hardware domain.

However just extending here won't be enough: we would also need to
modify xsm_default_action, as currently XSM_DM_PRIV will only be
allowed if src->target == target or is_control_domain(src).

diff --git a/xen/include/xsm/dummy.h b/xen/include/xsm/dummy.h
index 58afc1d589..ac40a24a22 100644
--- a/xen/include/xsm/dummy.h
+++ b/xen/include/xsm/dummy.h
@@ -88,7 +88,8 @@ static always_inline int xsm_default_action(
         }
         /* fall through */
     case XSM_DM_PRIV:
-        if ( target && evaluate_nospec(src->target == target) )
+        if ( is_hardware_domain(src) ||
+             (target && evaluate_nospec(src->target == target)) )
             return 0;
         /* fall through */
     case XSM_PRIV:

That would however also give the hardware domain access to XSM_TARGET
and XSM_XS_PRIV operations that where previously forbidden.

Or do we just require people with split control/hardware domain model
to also use a different XSM policy?

> Also, the sentence about later safety checks really ought to be in this
> source comment too.

Will add.

Thanks, Roger.