On Fri, Jan 25, 2019 at 08:43:59PM +0100, Marek Marczykowski-Górecki wrote:
> On Wed, Jan 16, 2019 at 11:52:21AM +0100, Marek Marczykowski-Górecki wrote:
> > On Wed, Jan 16, 2019 at 10:21:29AM +0100, Roger Pau Monné wrote:
> > > On Tue, Jan 15, 2019 at 04:36:31PM +0100, Marek Marczykowski-Górecki wrote:
> > > > From: Simon Gaiser <simon@invisiblethingslab.com>
> > > > 
> > > > Stubdomains need to be given sufficient privilege over the guest which it
> > > > provides emulation for in order for PCI passthrough to work correctly.
> > > > When a HVM domain try to enable MSI, QEMU in stubdomain calls
> > > > PHYSDEVOP_map_pirq, but later it needs to call XEN_DOMCTL_bind_pt_irq as
> > > > part of xc_domain_update_msi_irq. Allow for that as part of
> > > > PHYSDEVOP_map_pirq.
> > > 
> > > I see, that's not a problem AFAICT for PCI INTx because the IRQ in
> > > that case is known beforehand, and the stubdomain is given permissions
> > > over this IRQ by libxl__device_pci_add (there's a do_pci_add against
> > > the stubdomain).
> > 
> > Exactly.
> > 
> > > > 
> > > > Based on https://github.com/OpenXT/xenclient-oe/blob/5e0e7304a5a3c75ef01240a1e3673665b2aaf05e/recipes-extended/xen/files/stubdomain-msi-irq-access.patch by Eric Chanudet <chanudete@ainfosec.com>.
> > > > 
> > > > Signed-off-by: Simon Gaiser <simon@invisiblethingslab.com>
> > > > Signed-off-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com>
> > > > ---
> (...)
> > > > diff --git a/xen/arch/x86/irq.c b/xen/arch/x86/irq.c
> > > > index 8b44d6c..123ca69 100644
> > > > --- a/xen/arch/x86/irq.c
> > > > +++ b/xen/arch/x86/irq.c
> > > > @@ -2674,6 +2674,21 @@ int allocate_and_map_msi_pirq(struct domain *d, int index, int *pirq_p,
> > > >          {
> > > >      case MAP_PIRQ_TYPE_MULTI_MSI:
> > > >              irq = create_irq(NUMA_NO_NODE);
> > > > +            if ( !(irq < nr_irqs_gsi || irq >= nr_irqs) &&
> > > 
> > > This check is already performed below, maybe you could re-arrange the
> > > code as:
> > > 
> > > case MAP_PIRQ_TYPE_MULTI_MSI:
> > >         irq = create_irq(NUMA_NO_NODE);
> > >     }
> > > 
> > >     if ( irq < nr_irqs_gsi || irq >= nr_irqs )
> > >     {
> > >         dprintk(XENLOG_G_ERR, "dom%d: can't create irq for msi!\n",
> > >                 d->domain_id);
> > >         return -EINVAL;
> > >     }
> > >     if ( current->domain->target == d )
> > >         ...

Oh, and this won't fly either, as irq_permit_access() should happen only
when create_irq() was called, otherwise it will give access to arbitrary
irq.

> > > 
> > > But I wonder whether it would be better to place the irq_permit_access
> > > in map_domain_pirq, together with the existing irq_permit_access that
> > > grant the target domain permissions over the irq.
> > 
> > That may be a good idea. Let me try that in v3. But I'll wait for a
> > feedback on libxl patches first.
> 
> That doesn't work, as map_domain_pirq() check irq access earlier.
> Which bring be to a question, what should be rules guarding stubdomain
> access to PHYSDEVOP_map_pirq? With this patch, stubdomain will be able
> to create and map multiple irq (DoS possibility?), as only target domain
> is validated in practice. Is that ok? If not, what additional limits
> could be applied here?
> In INTx case the problem doesn't apply, because toolstack grant access
> to particular IRQ and no allocation happen on stubdomain request. But in
> MSI case, it isn't that easy as IRQ number isn't known before (as
> explained in the commit message).
> 



-- 
Best Regards,
Marek Marczykowski-Górecki
Invisible Things Lab
A: Because it messes up the order in which people normally read text.
Q: Why is top-posting such a bad thing?