[v6,8/8] iommu/vt-d: Disallow read-only mappings to nest parent domain
Commit Message
From: Lu Baolu <baolu.lu@linux.intel.com>
When remapping hardware is configured by system software in scalable mode
as Nested (PGTT=011b) and with PWSNP field Set in the PASID-table-entry,
it may Set Accessed bit and Dirty bit (and Extended Access bit if enabled)
in first-stage page-table entries even when second-stage mappings indicate
that corresponding first-stage page-table is Read-Only.
As the result, contents of pages designated by VMM as Read-Only can be
modified by IOMMU via PML5E (PML4E for 4-level tables) access as part of
address translation process due to DMAs issued by Guest.
This disallows read-only mappings in the domain that is supposed to be used
as nested parent. Reference from Sapphire Rapids Specification Update [1],
errata details, SPR17. Userspace should know this limitation by checking
the IOMMU_HW_INFO_VTD_ERRATA_772415_SPR17 flag reported in the IOMMU_GET_HW_INFO
ioctl.
[1] https://www.intel.com/content/www/us/en/content-details/772415/content-details.html
Reviewed-by: Kevin Tian <kevin.tian@intel.com>
Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com>
Signed-off-by: Yi Liu <yi.l.liu@intel.com>
---
drivers/iommu/intel/iommu.c | 9 +++++++++
drivers/iommu/intel/iommu.h | 1 +
include/uapi/linux/iommufd.h | 12 +++++++++++-
3 files changed, 21 insertions(+), 1 deletion(-)
Comments
On 10/20/23 5:32 PM, Yi Liu wrote:
> From: Lu Baolu <baolu.lu@linux.intel.com>
>
> When remapping hardware is configured by system software in scalable mode
> as Nested (PGTT=011b) and with PWSNP field Set in the PASID-table-entry,
> it may Set Accessed bit and Dirty bit (and Extended Access bit if enabled)
> in first-stage page-table entries even when second-stage mappings indicate
> that corresponding first-stage page-table is Read-Only.
>
> As the result, contents of pages designated by VMM as Read-Only can be
> modified by IOMMU via PML5E (PML4E for 4-level tables) access as part of
> address translation process due to DMAs issued by Guest.
>
> This disallows read-only mappings in the domain that is supposed to be used
> as nested parent. Reference from Sapphire Rapids Specification Update [1],
> errata details, SPR17. Userspace should know this limitation by checking
> the IOMMU_HW_INFO_VTD_ERRATA_772415_SPR17 flag reported in the IOMMU_GET_HW_INFO
> ioctl.
>
> [1] https://www.intel.com/content/www/us/en/content-details/772415/content-details.html
>
> Reviewed-by: Kevin Tian <kevin.tian@intel.com>
> Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com>
> Signed-off-by: Yi Liu <yi.l.liu@intel.com>
> ---
> drivers/iommu/intel/iommu.c | 9 +++++++++
> drivers/iommu/intel/iommu.h | 1 +
> include/uapi/linux/iommufd.h | 12 +++++++++++-
> 3 files changed, 21 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c
> index c7704e7efd4a..a0341a069fbf 100644
> --- a/drivers/iommu/intel/iommu.c
> +++ b/drivers/iommu/intel/iommu.c
> @@ -2193,6 +2193,11 @@ __domain_mapping(struct dmar_domain *domain, unsigned long iov_pfn,
> if ((prot & (DMA_PTE_READ|DMA_PTE_WRITE)) == 0)
> return -EINVAL;
>
> + if (!(prot & DMA_PTE_WRITE) && domain->is_nested_parent) {
> + pr_err_ratelimited("Read-only mapping is disallowed on the domain which serves as the parent in a nested configuration, due to HW errata (ERRATA_772415_SPR17)\n");
> + return -EINVAL;
> + }
> +
> attr = prot & (DMA_PTE_READ | DMA_PTE_WRITE | DMA_PTE_SNP);
> attr |= DMA_FL_PTE_PRESENT;
> if (domain->use_first_level) {
> @@ -4101,6 +4106,9 @@ intel_iommu_domain_alloc_user(struct device *dev, u32 flags,
> domain = iommu_domain_alloc(dev->bus);
> if (!domain)
> return ERR_PTR(-ENOMEM);
> + container_of(domain,
> + struct dmar_domain,
> + domain)->is_nested_parent = request_nest_parent;
How about
to_dmar_domain(domain)->is_nested_parent = ...;
?
I would also prefer to introduce is_nested_parent_domain to the user
domain allocation patch (patch 7/8). This field should be checked when
allocating a nested user domain.
diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c
index 8f81a5c9fcc0..d3f6bc1f6590 100644
--- a/drivers/iommu/intel/iommu.c
+++ b/drivers/iommu/intel/iommu.c
@@ -4121,6 +4121,8 @@ intel_iommu_domain_alloc_user(struct device *dev,
u32 flags,
return ERR_PTR(-EINVAL);
if (request_nest_parent)
return ERR_PTR(-EINVAL);
+ if (!to_dmar_domain(parent)->is_nested_parent)
+ return ERR_PTR(-EINVAL);
return intel_nested_domain_alloc(parent, user_data);
}
Best regards,
baolu
> From: Baolu Lu <baolu.lu@linux.intel.com>
> Sent: Saturday, October 21, 2023 11:24 AM
>
> On 10/20/23 5:32 PM, Yi Liu wrote:
> > From: Lu Baolu <baolu.lu@linux.intel.com>
> >
> > When remapping hardware is configured by system software in scalable mode
> > as Nested (PGTT=011b) and with PWSNP field Set in the PASID-table-entry,
> > it may Set Accessed bit and Dirty bit (and Extended Access bit if enabled)
> > in first-stage page-table entries even when second-stage mappings indicate
> > that corresponding first-stage page-table is Read-Only.
> >
> > As the result, contents of pages designated by VMM as Read-Only can be
> > modified by IOMMU via PML5E (PML4E for 4-level tables) access as part of
> > address translation process due to DMAs issued by Guest.
> >
> > This disallows read-only mappings in the domain that is supposed to be used
> > as nested parent. Reference from Sapphire Rapids Specification Update [1],
> > errata details, SPR17. Userspace should know this limitation by checking
> > the IOMMU_HW_INFO_VTD_ERRATA_772415_SPR17 flag reported in the
> IOMMU_GET_HW_INFO
> > ioctl.
> >
> > [1] https://www.intel.com/content/www/us/en/content-details/772415/content-
> details.html
> >
> > Reviewed-by: Kevin Tian <kevin.tian@intel.com>
> > Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com>
> > Signed-off-by: Yi Liu <yi.l.liu@intel.com>
> > ---
> > drivers/iommu/intel/iommu.c | 9 +++++++++
> > drivers/iommu/intel/iommu.h | 1 +
> > include/uapi/linux/iommufd.h | 12 +++++++++++-
> > 3 files changed, 21 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c
> > index c7704e7efd4a..a0341a069fbf 100644
> > --- a/drivers/iommu/intel/iommu.c
> > +++ b/drivers/iommu/intel/iommu.c
> > @@ -2193,6 +2193,11 @@ __domain_mapping(struct dmar_domain *domain,
> unsigned long iov_pfn,
> > if ((prot & (DMA_PTE_READ|DMA_PTE_WRITE)) == 0)
> > return -EINVAL;
> >
> > + if (!(prot & DMA_PTE_WRITE) && domain->is_nested_parent) {
> > + pr_err_ratelimited("Read-only mapping is disallowed on the domain
> which serves as the parent in a nested configuration, due to HW errata
> (ERRATA_772415_SPR17)\n");
> > + return -EINVAL;
> > + }
> > +
> > attr = prot & (DMA_PTE_READ | DMA_PTE_WRITE | DMA_PTE_SNP);
> > attr |= DMA_FL_PTE_PRESENT;
> > if (domain->use_first_level) {
> > @@ -4101,6 +4106,9 @@ intel_iommu_domain_alloc_user(struct device *dev, u32
> flags,
> > domain = iommu_domain_alloc(dev->bus);
> > if (!domain)
> > return ERR_PTR(-ENOMEM);
> > + container_of(domain,
> > + struct dmar_domain,
> > + domain)->is_nested_parent = request_nest_parent;
>
> How about
> to_dmar_domain(domain)->is_nested_parent = ...;
> ?
Yes.
>
> I would also prefer to introduce is_nested_parent_domain to the user
> domain allocation patch (patch 7/8). This field should be checked when
> allocating a nested user domain.
A ctually, no need. This should be a common check, so iommufd core already
has the check. So the parent should be a nest parent domain, otherwise already
returned in iommufd.
+ if (!parent->nest_parent)
+ return ERR_PTR(-EINVAL);
https://lore.kernel.org/linux-iommu/20231020091946.12173-8-yi.l.liu@intel.com/
> diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c
> index 8f81a5c9fcc0..d3f6bc1f6590 100644
> --- a/drivers/iommu/intel/iommu.c
> +++ b/drivers/iommu/intel/iommu.c
> @@ -4121,6 +4121,8 @@ intel_iommu_domain_alloc_user(struct device *dev,
> u32 flags,
> return ERR_PTR(-EINVAL);
> if (request_nest_parent)
> return ERR_PTR(-EINVAL);
> + if (!to_dmar_domain(parent)->is_nested_parent)
> + return ERR_PTR(-EINVAL);
>
> return intel_nested_domain_alloc(parent, user_data);
> }
>
> Best regards,
> baolu
On 2023/10/23 19:15, Liu, Yi L wrote:
>> I would also prefer to introduce is_nested_parent_domain to the user
>> domain allocation patch (patch 7/8). This field should be checked when
>> allocating a nested user domain.
> A ctually, no need. This should be a common check, so iommufd core already
> has the check. So the parent should be a nest parent domain, otherwise already
> returned in iommufd.
>
> + if (!parent->nest_parent)
> + return ERR_PTR(-EINVAL);
I know this will not cause errors in the code. But since you are
introducing is_parent property in the vt-d driver. The integrity of the
property should be ensured. In this way, it will make the code more
readable and maintainable.
Best regards,
baolu
> From: Baolu Lu <baolu.lu@linux.intel.com>
> Sent: Monday, October 23, 2023 8:18 PM
>
> On 2023/10/23 19:15, Liu, Yi L wrote:
> >> I would also prefer to introduce is_nested_parent_domain to the user
> >> domain allocation patch (patch 7/8). This field should be checked when
> >> allocating a nested user domain.
> > A ctually, no need. This should be a common check, so iommufd core already
> > has the check. So the parent should be a nest parent domain, otherwise already
> > returned in iommufd.
> >
> > + if (!parent->nest_parent)
> > + return ERR_PTR(-EINVAL);
>
> I know this will not cause errors in the code. But since you are
> introducing is_parent property in the vt-d driver. The integrity of the
> property should be ensured. In this way, it will make the code more
> readable and maintainable.
Ok, if consider it as a property, then it's fine. At first, I just want to
make it as a special flag for this errata. But we cannot predict if there
will be more nested parent special stuffs, then this flag is also needed.
Regards,
Yi Liu
@@ -2193,6 +2193,11 @@ __domain_mapping(struct dmar_domain *domain, unsigned long iov_pfn,
if ((prot & (DMA_PTE_READ|DMA_PTE_WRITE)) == 0)
return -EINVAL;
+ if (!(prot & DMA_PTE_WRITE) && domain->is_nested_parent) {
+ pr_err_ratelimited("Read-only mapping is disallowed on the domain which serves as the parent in a nested configuration, due to HW errata (ERRATA_772415_SPR17)\n");
+ return -EINVAL;
+ }
+
attr = prot & (DMA_PTE_READ | DMA_PTE_WRITE | DMA_PTE_SNP);
attr |= DMA_FL_PTE_PRESENT;
if (domain->use_first_level) {
@@ -4101,6 +4106,9 @@ intel_iommu_domain_alloc_user(struct device *dev, u32 flags,
domain = iommu_domain_alloc(dev->bus);
if (!domain)
return ERR_PTR(-ENOMEM);
+ container_of(domain,
+ struct dmar_domain,
+ domain)->is_nested_parent = request_nest_parent;
return domain;
}
@@ -4839,6 +4847,7 @@ static void *intel_iommu_hw_info(struct device *dev, u32 *length, u32 *type)
if (!vtd)
return ERR_PTR(-ENOMEM);
+ vtd->flags = IOMMU_HW_INFO_VTD_ERRATA_772415_SPR17;
vtd->cap_reg = iommu->cap;
vtd->ecap_reg = iommu->ecap;
*length = sizeof(*vtd);
@@ -593,6 +593,7 @@ struct dmar_domain {
* otherwise, goes through the second
* level.
*/
+ u8 is_nested_parent:1; /* has other domains nested on it */
spinlock_t lock; /* Protect device tracking lists */
struct list_head devices; /* all devices' list */
@@ -439,10 +439,20 @@ struct iommu_hwpt_alloc {
};
#define IOMMU_HWPT_ALLOC _IO(IOMMUFD_TYPE, IOMMUFD_CMD_HWPT_ALLOC)
+/**
+ * enum iommu_hw_info_vtd_flags - Flags for VT-d hw_info
+ * @IOMMU_HW_INFO_VTD_ERRATA_772415_SPR17: If set, disallow nesting on domains
+ * with read-only mapping.
+ * https://www.intel.com/content/www/us/en/content-details/772415/content-details.html
+ */
+enum iommu_hw_info_vtd_flags {
+ IOMMU_HW_INFO_VTD_ERRATA_772415_SPR17 = 1 << 0,
+};
+
/**
* struct iommu_hw_info_vtd - Intel VT-d hardware information
*
- * @flags: Must be 0
+ * @flags: Combination of enum iommu_hw_info_vtd_flags
* @__reserved: Must be 0
*
* @cap_reg: Value of Intel VT-d capability register defined in VT-d spec