Re: [PATCH 1/2] VT-d: avoid NULL deref on domain_context_mapping_one() error paths

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

On Wed, Apr 06, 2022 at 02:24:32PM +0200, Jan Beulich wrote:
> First there's a printk() which actually wrongly uses pdev in the first
> place: We want to log the coordinates of the (perhaps fake) device
> acted upon, which may not be pdev.
> 
> Then it was quite pointless for eb19326a328d ("VT-d: prepare for per-
> device quarantine page tables (part I)") to add a domid_t parameter to
> domain_context_unmap_one(): It's only used to pass back here via
> me_wifi_quirk() -> map_me_phantom_function(). Drop the parameter again.
> 
> Finally there's the invocation of domain_context_mapping_one(), which
> needs to be passed the correct domain ID. Avoid taking that path when
> pdev is NULL and the quarantine state is what would need restoring to.
> This means we can't security-support PCI devices with RMRRs (if such
> exist in practice) any longer.
> 
> Fixes: 8f41e481b485 ("VT-d: re-assign devices directly")
> Fixes: 14dd241aad8a ("IOMMU/x86: use per-device page tables for quarantining")
> Coverity ID: 1503784
> Reported-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>
> 
> --- a/SUPPORT.md
> +++ b/SUPPORT.md
> @@ -750,6 +750,10 @@ However, this feature can still confer s
>  when used to remove drivers and backends from domain 0
>  (i.e., Driver Domains).
>  
> +On VT-d (Intel hardware) passing through plain PCI (or PCI-X) devices
> +when they have associated Reserved Memory Regions (RMRRs)
> +is not security supported, if such a combination exists in the first place.

Hm, I think this could be confusing from a user PoV.  It's my
understanding you want to differentiate between DEV_TYPE_PCIe_ENDPOINT
and DEV_TYPE_PCI device types, so maybe we could use:

"On VT-d (Intel hardware) passing through non PCI Express devices with
associated Reserved Memory Regions (RMRRs) is not supported."

AFAICT domain_context_mapping will already prevent this from happening
by returning -EOPNOTSUPP (see the DEV_TYPE_PCI case handling).

>  ### x86/Multiple IOREQ servers
>  
>  An IOREQ server provides emulated devices to HVM and PVH guests.
> --- a/xen/drivers/passthrough/vtd/extern.h
> +++ b/xen/drivers/passthrough/vtd/extern.h
> @@ -85,7 +85,7 @@ int domain_context_mapping_one(struct do
>                                 const struct pci_dev *pdev, domid_t domid,
>                                 paddr_t pgd_maddr, unsigned int mode);
>  int domain_context_unmap_one(struct domain *domain, struct vtd_iommu *iommu,
> -                             uint8_t bus, uint8_t devfn, domid_t domid);
> +                             uint8_t bus, uint8_t devfn);
>  int cf_check intel_iommu_get_reserved_device_memory(
>      iommu_grdm_t *func, void *ctxt);
>  
> --- a/xen/drivers/passthrough/vtd/iommu.c
> +++ b/xen/drivers/passthrough/vtd/iommu.c
> @@ -1533,7 +1533,7 @@ int domain_context_mapping_one(
>                  check_cleanup_domid_map(domain, pdev, iommu);
>              printk(XENLOG_ERR
>                     "%pp: unexpected context entry %016lx_%016lx (expected 
> %016lx_%016lx)\n",
> -                   &PCI_SBDF3(pdev->seg, pdev->bus, devfn),
> +                   &PCI_SBDF3(seg, bus, devfn),
>                     (uint64_t)(res >> 64), (uint64_t)res,
>                     (uint64_t)(old >> 64), (uint64_t)old);
>              rc = -EILSEQ;
> @@ -1601,9 +1601,13 @@ int domain_context_mapping_one(
>  
>      if ( rc )
>      {
> -        if ( !prev_dom )
> -            ret = domain_context_unmap_one(domain, iommu, bus, devfn,
> -                                           DEVICE_DOMID(domain, pdev));
> +        if ( !prev_dom ||
> +             /*
> +              * Unmapping here means PCI devices with RMRRs (if such exist)
> +              * will cause problems if such a region was actually accessed.
> +              */
> +             (prev_dom == dom_io && !pdev) )

Maybe I'm reading this wrong, but plain PCI devices with RMRRs are
only allowed to be assigned to the hardware domain, and won't be able
to be reassigned afterwards.  It would be fine to unmap
unconditionally if !prev_dom or !pdev?  As calls with !pdev only
happening for phantom functions or bridge devices.

Thanks, Roger.