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

[Julien Grall <julien@xxxxxxx>]

Hi,

On 16/09/2023 01:11, Henry Wang wrote:
> > On Sep 15, 2023, at 20:52, 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 to pci_remove_detached().
> >
> > Also add a comment on top of the error message when the PIRQ cannot
> > be unbind to explain this could be a spurious error as QEMU may have
> > already done it.
> >
> > Signed-off-by: Julien Grall <jgrall@xxxxxxxxxx>
> As in discussion in v1, it is agreed that this patch should be included in
> 4.18, although technically my release-ack tag should be effective after
> code freeze, I am still providing the tag to avoid possible confusion:
>
> Release-acked-by: Henry Wang <Henry.Wang@xxxxxxx>
Thanks. I have committed the patch with Anthony's reviewed-by tag.

Cheers,

--
Julien Grall