2020-07-30 00:15:54

by Jacob Pan

Subject: [PATCH v7 7/7] iommu/vt-d: Check UAPI data processed by IOMMU core

IOMMU generic layer already does sanity checks UAPI data for version
match and argsz range under generic information.
Remove the redundant version check from VT-d driver and check for vendor
specific data size.

Signed-off-by: Jacob Pan <[email protected]>
---
drivers/iommu/intel/iommu.c | 3 +--
drivers/iommu/intel/svm.c | 7 +++++--
2 files changed, 6 insertions(+), 4 deletions(-)

diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c
index 021f62078f52..7e03cca31a0e 100644
--- a/drivers/iommu/intel/iommu.c
+++ b/drivers/iommu/intel/iommu.c
@@ -5391,8 +5391,7 @@ intel_iommu_sva_invalidate(struct iommu_domain *domain, struct device *dev,
int ret = 0;
u64 size = 0;

- if (!inv_info || !dmar_domain ||
- inv_info->version != IOMMU_CACHE_INVALIDATE_INFO_VERSION_1)
+ if (!inv_info || !dmar_domain)
return -EINVAL;

if (!dev || !dev_is_pci(dev))
diff --git a/drivers/iommu/intel/svm.c b/drivers/iommu/intel/svm.c
index 713b3a218483..55ea11e9c0f5 100644
--- a/drivers/iommu/intel/svm.c
+++ b/drivers/iommu/intel/svm.c
@@ -240,8 +240,11 @@ int intel_svm_bind_gpasid(struct iommu_domain *domain, struct device *dev,
if (WARN_ON(!iommu) || !data)
return -EINVAL;

- if (data->version != IOMMU_GPASID_BIND_VERSION_1 ||
- data->format != IOMMU_PASID_FORMAT_INTEL_VTD)
+ if (data->format != IOMMU_PASID_FORMAT_INTEL_VTD)
+ return -EINVAL;
+
+ /* IOMMU core ensures argsz is more than the start of the union */
+ if (data->argsz < offsetofend(struct iommu_gpasid_bind_data, vendor.vtd))
return -EINVAL;

if (!dev_is_pci(dev))
--
2.7.4

2020-08-13 09:22:11

by Eric Auger

Subject: Re: [PATCH v7 7/7] iommu/vt-d: Check UAPI data processed by IOMMU core

Hi Jacob,

On 7/30/20 2:21 AM, Jacob Pan wrote:
> IOMMU generic layer already does sanity checks UAPI data for version
> match and argsz range under generic information.
> Remove the redundant version check from VT-d driver and check for vendor
> specific data size.
>
> Signed-off-by: Jacob Pan <[email protected]>

> ---
> drivers/iommu/intel/iommu.c | 3 +--
> drivers/iommu/intel/svm.c | 7 +++++--
> 2 files changed, 6 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c
> index 021f62078f52..7e03cca31a0e 100644
> --- a/drivers/iommu/intel/iommu.c
> +++ b/drivers/iommu/intel/iommu.c
> @@ -5391,8 +5391,7 @@ intel_iommu_sva_invalidate(struct iommu_domain *domain, struct device *dev,
> int ret = 0;
> u64 size = 0;
>
> - if (!inv_info || !dmar_domain ||
> - inv_info->version != IOMMU_CACHE_INVALIDATE_INFO_VERSION_1)
> + if (!inv_info || !dmar_domain)
> return -EINVAL;
>
> if (!dev || !dev_is_pci(dev))
> diff --git a/drivers/iommu/intel/svm.c b/drivers/iommu/intel/svm.c
> index 713b3a218483..55ea11e9c0f5 100644
> --- a/drivers/iommu/intel/svm.c
> +++ b/drivers/iommu/intel/svm.c
> @@ -240,8 +240,11 @@ int intel_svm_bind_gpasid(struct iommu_domain *domain, struct device *dev,
> if (WARN_ON(!iommu) || !data)
> return -EINVAL;
>
> - if (data->version != IOMMU_GPASID_BIND_VERSION_1 ||
> - data->format != IOMMU_PASID_FORMAT_INTEL_VTD)
> + if (data->format != IOMMU_PASID_FORMAT_INTEL_VTD)
> + return -EINVAL;
> +
> + /* IOMMU core ensures argsz is more than the start of the union */
> + if (data->argsz < offsetofend(struct iommu_gpasid_bind_data, vendor.vtd))
> return -EINVAL;
Shouldn't you test the vendor flags here? intel_pasid_setup_bind_data()
only checks valid ones but not ~mask.

Thanks

Eric
>
> if (!dev_is_pci(dev))
>

2020-08-31 17:46:28

by Jacob Pan

Subject: Re: [PATCH v7 7/7] iommu/vt-d: Check UAPI data processed by IOMMU core

On Thu, 13 Aug 2020 11:19:46 +0200
Auger Eric <[email protected]> wrote:

> Hi Jacob,
>
> On 7/30/20 2:21 AM, Jacob Pan wrote:
> > IOMMU generic layer already does sanity checks UAPI data for version
> > match and argsz range under generic information.
> > Remove the redundant version check from VT-d driver and check for
> > vendor specific data size.
> >
> > Signed-off-by: Jacob Pan <[email protected]>
>
> > ---
> > drivers/iommu/intel/iommu.c | 3 +--
> > drivers/iommu/intel/svm.c | 7 +++++--
> > 2 files changed, 6 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/iommu/intel/iommu.c
> > b/drivers/iommu/intel/iommu.c index 021f62078f52..7e03cca31a0e
> > 100644 --- a/drivers/iommu/intel/iommu.c
> > +++ b/drivers/iommu/intel/iommu.c
> > @@ -5391,8 +5391,7 @@ intel_iommu_sva_invalidate(struct
> > iommu_domain *domain, struct device *dev, int ret = 0;
> > u64 size = 0;
> >
> > - if (!inv_info || !dmar_domain ||
> > - inv_info->version !=
> > IOMMU_CACHE_INVALIDATE_INFO_VERSION_1)
> > + if (!inv_info || !dmar_domain)
> > return -EINVAL;
> >
> > if (!dev || !dev_is_pci(dev))
> > diff --git a/drivers/iommu/intel/svm.c b/drivers/iommu/intel/svm.c
> > index 713b3a218483..55ea11e9c0f5 100644
> > --- a/drivers/iommu/intel/svm.c
> > +++ b/drivers/iommu/intel/svm.c
> > @@ -240,8 +240,11 @@ int intel_svm_bind_gpasid(struct iommu_domain
> > *domain, struct device *dev, if (WARN_ON(!iommu) || !data)
> > return -EINVAL;
> >
> > - if (data->version != IOMMU_GPASID_BIND_VERSION_1 ||
> > - data->format != IOMMU_PASID_FORMAT_INTEL_VTD)
> > + if (data->format != IOMMU_PASID_FORMAT_INTEL_VTD)
> > + return -EINVAL;
> > +
> > + /* IOMMU core ensures argsz is more than the start of the
> > union */
> > + if (data->argsz < offsetofend(struct
> > iommu_gpasid_bind_data, vendor.vtd)) return -EINVAL;
> Shouldn't you test the vendor flags here?
> intel_pasid_setup_bind_data() only checks valid ones but not ~mask.
>
Yes, vendor flags should be tested to make sure there is no undefined
flags being used.

Thanks!

> Thanks
>
> Eric
> >
> > if (!dev_is_pci(dev))
> >
>

[Jacob Pan]