Re: [Xen-devel] [PATCH v2] VT-d: don't pass bridge devices to domain_context_mapping_one()

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

On Mon, Jan 20, 2020 at 04:42:22PM +0100, Jan Beulich wrote:
> When passed a non-NULL pdev, the function does an owner check when it
> finds an already existing context mapping. Bridges, however, don't get
> passed through to guests, and hence their owner is always going to be
> Dom0, leading to the assigment of all but one of the function of multi-
> function PCI devices behind bridges to fail.
> 
> Reported-by: Marek Marczykowski-Górecki <marmarek@xxxxxxxxxxxxxxxxxxxxxx>
> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>
> ---
> v2: Add comments.
> ---
> Note: This was reported as an apparent regression from XSA-302 / -306.
>       So far I haven't been able to figure out how the code would have
>       worked before, i.e. to me it looks like a pre-existing problem.
>       This leaves the risk of the change here papering over another
>       issue.
> 
> --- a/xen/drivers/passthrough/vtd/iommu.c
> +++ b/xen/drivers/passthrough/vtd/iommu.c
> @@ -1493,18 +1493,28 @@ static int domain_context_mapping(struct
>          if ( find_upstream_bridge(seg, &bus, &devfn, &secbus) < 1 )
>              break;
>  
> +        /*
> +         * Mapping a bridge should, if anything, pass the struct pci_dev of
> +         * that bridge. Since bridges don't normally get assigned to guests,
> +         * their owner would be the wrong one. Pass NULL instead.
> +         */
>          ret = domain_context_mapping_one(domain, drhd->iommu, bus, devfn,
> -                                         pci_get_pdev(seg, bus, devfn));
> +                                         NULL);
>  
>          /*
>           * Devices behind PCIe-to-PCI/PCIx bridge may generate different
>           * requester-id. It may originate from devfn=0 on the secondary bus
>           * behind the bridge. Map that id as well if we didn't already.
> +         *
> +         * Somewhat similar as for bridges, we don't want to pass a struct
> +         * pci_dev here - there may not even exist one for this (secbus,0,0)
> +         * tuple. If there is one, without properly working device groups it
> +         * may again not have the correct owner.
>           */
>          if ( !ret && pdev_type(seg, bus, devfn) == DEV_TYPE_PCIe2PCI_BRIDGE 
> &&
>               (secbus != pdev->bus || pdev->devfn != 0) )
>              ret = domain_context_mapping_one(domain, drhd->iommu, secbus, 0,
> -                                             pci_get_pdev(seg, secbus, 0));
> +                                             NULL);

Isn't it dangerous to map this device to the guest, and that multiple
guests might end up with the same device mapped?

Thanks, Roger.

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel