| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <BN9PR11MB527679E5CE0516B2786F57518CED9@BN9PR11MB5276.namprd11.prod.outlook.com>
Date: Tue, 12 Apr 2022 03:15:47 +0000
From: "Tian, Kevin" <kevin.tian@...el.com>
To: Lu Baolu <baolu.lu@...ux.intel.com>,
Joerg Roedel <joro@...tes.org>,
"Jason Gunthorpe" <jgg@...dia.com>,
Christoph Hellwig <hch@...radead.org>,
"Raj, Ashok" <ashok.raj@...el.com>, Will Deacon <will@...nel.org>,
Robin Murphy <robin.murphy@....com>,
Jean-Philippe Brucker <jean-philippe@...aro.com>
CC: Eric Auger <eric.auger@...hat.com>,
"Liu, Yi L" <yi.l.liu@...el.com>,
"Pan, Jacob jun" <jacob.jun.pan@...el.com>,
"iommu@...ts.linux-foundation.org" <iommu@...ts.linux-foundation.org>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: RE: [PATCH RFC v3 02/12] iommu: Add a flag to indicate immutable
singleton group
> From: Lu Baolu <baolu.lu@...ux.intel.com>
> Sent: Sunday, April 10, 2022 6:25 PM
>
> Some features require that a single device must be immutably isolated,
> even when hot plug is supported.
This reads confusing, as hotplug cannot be allowed in a singleton group.
What you actually meant suppose to be:
"Enabling certain device features require the device in a singleton iommu
group which is immutable in fabric i.e. not affected by hotplug"
> For example, the SVA bind()/unbind()
> interfaces require that the device exists in a singleton group. If we
> have a singleton group that doesn't have ACS (or similar technologies)
> and someone hotplugs in another device on a bridge, then our SVA is
> completely broken and we get data corruption.
this needs the background that PASID doesn't join PCI packet routing
thus the dma address (CPU VA) may hit a p2p range.
>
> This adds a flag in the iommu_group struct to indicate an immutable
> singleton group, and uses standard PCI bus topology, isolation features,
> and DMA alias quirks to set the flag. If the device came from DT, assume
> it is static and then the singleton attribute can know from the device
> count in the group.
where does the assumption come from?
>
> Suggested-by: Jason Gunthorpe <jgg@...dia.com>
> Suggested-by: Kevin Tian <kevin.tian@...el.com>
> Signed-off-by: Lu Baolu <baolu.lu@...ux.intel.com>
> ---
> drivers/iommu/iommu.c | 67 ++++++++++++++++++++++++++++++++++++----
> ---
> 1 file changed, 57 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
> index 0c42ece25854..56ffbf5fdc18 100644
> --- a/drivers/iommu/iommu.c
> +++ b/drivers/iommu/iommu.c
> @@ -48,6 +48,7 @@ struct iommu_group {
> struct list_head entry;
> unsigned int owner_cnt;
> void *owner;
> + bool immutable_singleton;
Just call it 'singleton' with a comment to explain it must be immutable?
> };
>
> struct group_device {
> @@ -74,6 +75,16 @@ static const char * const
> iommu_group_resv_type_string[] = {
> #define IOMMU_CMD_LINE_DMA_API BIT(0)
> #define IOMMU_CMD_LINE_STRICT BIT(1)
>
> +/*
> + * To consider a PCI device isolated, we require ACS to support Source
> + * Validation, Request Redirection, Completer Redirection, and Upstream
> + * Forwarding. This effectively means that devices cannot spoof their
> + * requester ID, requests and completions cannot be redirected, and all
> + * transactions are forwarded upstream, even as it passes through a
> + * bridge where the target device is downstream.
> + */
> +#define REQ_ACS_FLAGS (PCI_ACS_SV | PCI_ACS_RR | PCI_ACS_CR |
> PCI_ACS_UF)
> +
> static int iommu_alloc_default_domain(struct iommu_group *group,
> struct device *dev);
> static struct iommu_domain *__iommu_domain_alloc(struct bus_type *bus,
> @@ -89,6 +100,7 @@ static int
> iommu_create_device_direct_mappings(struct iommu_group *group,
> static struct iommu_group *iommu_group_get_for_dev(struct device *dev);
> static ssize_t iommu_group_store_type(struct iommu_group *group,
> const char *buf, size_t count);
> +static int iommu_group_device_count(struct iommu_group *group);
>
> #define IOMMU_GROUP_ATTR(_name, _mode, _show, _store)
> \
> struct iommu_group_attribute iommu_group_attr_##_name = \
> @@ -844,6 +856,37 @@ static bool iommu_is_attach_deferred(struct device
> *dev)
> return false;
> }
>
> +static int has_pci_alias(struct pci_dev *pdev, u16 alias, void *opaque)
> +{
> + return -EEXIST;
> +}
> +
> +static bool pci_immutably_isolated(struct pci_dev *pdev)
> +{
> + /* Skip the bridges. */
> + if (pci_is_bridge(pdev))
> + return false;
> +
> + /*
> + * The device could be considered to be fully isolated if
> + * all devices on the path from the parent to the host-PCI
> + * bridge are protected from peer-to-peer DMA by ACS.
> + */
> + if (!pci_is_root_bus(pdev->bus) &&
> + !pci_acs_path_enabled(pdev->bus->self, NULL, REQ_ACS_FLAGS))
> + return false;
> +
> + /* Multi-function devices should have ACS enabled. */
> + if (pdev->multifunction && !pci_acs_enabled(pdev, REQ_ACS_FLAGS))
> + return false;
Looks my earlier comment was lost, i.e. you can just use
pci_acs_path_enabled(pdev) to cover above two checks.
> +
> + /* Filter out devices which has any alias device. */
> + if (pci_for_each_dma_alias(pdev, has_pci_alias, NULL))
> + return false;
> +
> + return true;
> +}
> +
> /**
> * iommu_group_add_device - add a device to an iommu group
> * @group: the group into which to add the device (reference should be held)
> @@ -898,6 +941,20 @@ int iommu_group_add_device(struct iommu_group
> *group, struct device *dev)
> list_add_tail(&device->list, &group->devices);
> if (group->domain && !iommu_is_attach_deferred(dev))
> ret = __iommu_attach_device(group->domain, dev);
> +
> + /*
> + * Use standard PCI bus topology, isolation features, and DMA
> + * alias quirks to set the immutable singleton attribute. If
> + * the device came from DT, assume it is static and then
> + * singleton can know from the device count in the group.
> + */
> + if (dev_is_pci(dev))
> + group->immutable_singleton =
> + pci_immutably_isolated(to_pci_dev(dev));
> + else if (is_of_node(dev_fwnode(dev)))
> + group->immutable_singleton =
> + (iommu_group_device_count(group) == 1);
> +
> mutex_unlock(&group->mutex);
> if (ret)
> goto err_put_group;
> @@ -1290,16 +1347,6 @@ EXPORT_SYMBOL_GPL(iommu_group_id);
> static struct iommu_group *get_pci_alias_group(struct pci_dev *pdev,
> unsigned long *devfns);
>
> -/*
> - * To consider a PCI device isolated, we require ACS to support Source
> - * Validation, Request Redirection, Completer Redirection, and Upstream
> - * Forwarding. This effectively means that devices cannot spoof their
> - * requester ID, requests and completions cannot be redirected, and all
> - * transactions are forwarded upstream, even as it passes through a
> - * bridge where the target device is downstream.
> - */
> -#define REQ_ACS_FLAGS (PCI_ACS_SV | PCI_ACS_RR | PCI_ACS_CR |
> PCI_ACS_UF)
> -
> /*
> * For multifunction devices which are not isolated from each other, find
> * all the other non-isolated functions and look for existing groups. For
> --
> 2.25.1
Powered by blists - more mailing lists