Re: [PATCH 2/2] tools/light: Revoke permissions when a PCI detach for HVM domain

[Julien Grall <julien@xxxxxxx>]

Hi Jason,

On 10/08/2023 21:02, Jason Andryuk wrote:
> On Wed, Aug 9, 2023 at 6:34 AM Julien Grall <julien@xxxxxxx> wrote:
> > From: Julien Grall <jgrall@xxxxxxxxxx>
> >
> > Currently, libxl will grant IOMEM, I/O port and IRQ permissions when
> > a PCI is attached (see pci_add_dm_done()) for all domain types. However,
> > the permissions are only revoked for non-HVM domain (see do_pci_remove()).
> >
> > This means that HVM domains will be left with extra permissions. While
> > this look bad on the paper, the IRQ permissions should be revoked
> > when the Device Model call xc_physdev_unmap_pirq() and such domain
> > cannot directly mapped I/O port and IOMEM regions. Instead, this has to
> > be done by a Device Model.
> >
> > The Device Model can only run in dom0 or PV stubdomain (upstream libxl
> > doesn't have support for HVM/PVH stubdomain).
> >
> > For PV/PVH stubdomain, the permission are properly revoked, so there is
> > no security concern.
> >
> > This leaves dom0. There are two cases:
> >    1) Privileged: Anyone gaining access to the Device Model would already
> >       have large control on the host.
> >    2) Deprivileged: PCI passthrough require PHYSDEV operations which
> >       are not accessible when the Device Model is restricted.
> >
> > So overall, it is believed that the extra permissions cannot be exploited.
> >
> > Rework the code so the permissions are all removed for HVM domains.
> > This needs to happen after the QEMU has detached the device. So
> > the revocation is now moved in a separate function which is called
> > from pci_remove_detached().
> >
> > Signed-off-by: Julien Grall <jgrall@xxxxxxxxxx>
> Reviewed-by: Jason Andryuk <jandryuk@xxxxxxxxx>
Thanks for the review!

> With one suggestion below
>
> > ---
> >
> > TODO: I am getting a bit confused with the async work in libxl. I am
> > not entirely sure whether pci_remove_detached() is the correct place
> > to revoke.
> I think the location is fine.
>
> > TODO: For HVM, we are now getting the following error on detach:
> > libxl: error: libxl_pci.c:2009:pci_revoke_permissions: Domain 
> > 3:xc_physdev_unmap_pirq irq=23: Invalid argument
> >
> > This is because the IRQ was unmapped by QEMU. It doesn't feel
> > right to skip the call. So maybe we can ignore the error?
> Sounds reasonable.  Would be better if we could clearly differentiate
> between QEMU already unmapped and some other EINVAL error.
The only way I could think is to have a distinct errno in Xen to 
indicate the PIRQ is not mapped. CCing the x86 maintainers to check if 
they would be ok with that.
> > ---
> >   tools/libs/light/libxl_pci.c | 142 ++++++++++++++++++++---------------
> >   1 file changed, 80 insertions(+), 62 deletions(-)
> >
> > diff --git a/tools/libs/light/libxl_pci.c b/tools/libs/light/libxl_pci.c
> > index 7f5f170e6eb0..f5a4b88eb2c0 100644
> > --- a/tools/libs/light/libxl_pci.c
> > +++ b/tools/libs/light/libxl_pci.c
> > @@ -1943,6 +1943,79 @@ static void pci_remove_stubdom_done(libxl__egc *egc,
> >   static void pci_remove_done(libxl__egc *egc,
> >       pci_remove_state *prs, int rc);
> >
> > +static void pci_revoke_permissions(libxl__egc *egc, pci_remove_state *prs)
> > +{
> > +    STATE_AO_GC(prs->aodev->ao);
> > +    libxl_ctx *ctx = libxl__gc_owner(gc);
> > +    const libxl_device_pci *pci = &prs->pci;
> > +    const char *sysfs_path;
> > +    uint32_t domid = prs->domid;
> > +    FILE *f;
> > +    unsigned int start = 0, end = 0, flags = 0, size = 0;
> These variables ...
>
> > +    int irq = 0;
> > +    int i, rc;
> > +
> > +    sysfs_path = GCSPRINTF(SYSFS_PCI_DEV"/"PCI_BDF"/resource",
> > +                           pci->domain, pci->bus, pci->dev, pci->func);
> > +
> > +    f = fopen(sysfs_path, "r");
> > +    if (f == NULL) {
> > +        LOGED(ERROR, domid, "Couldn't open %s", sysfs_path);
> > +        goto skip_bar;
> > +    }
> > +
> > +    for (i = 0; i < PROC_PCI_NUM_RESOURCES; i++) {
> ... could move into the loop here.
You are right. I will respin the patch with this change and whatever we 
decide for the unmap error.
Cheers,

--
Julien Grall