IOMMU user API header was introduced to support nested DMA translation and
related fault handling. The current UAPI data structures consist of three
areas that cover the interactions between host kernel and guest:
- fault handling
- cache invalidation
- bind guest page tables, i.e. guest PASID
Future extensions are likely to support more architectures and vIOMMU features.
In the previous discussion, using user-filled data size and feature flags is
made a preferred approach over a unified version number.
https://lkml.org/lkml/2020/1/29/45
In addition to introduce argsz field to data structures, this patchset is also
trying to document the UAPI design, usage, and extension rules. VT-d driver
changes to utilize the new argsz field is included, VFIO usage is to follow.
This set is available at:
https://github.com/jacobpan/linux.git vsva_v5.8_uapi_v7
Thanks,
Jacob
Changeog:
v7
- Added PASID data format enum for range checking
- Tidy up based on reviews from Alex W.
- Removed doc section for vIOMMU fault handling
v6
- Renamed all UAPI functions with iommu_uapi_ prefix
- Replaced argsz maxsz checking with flag specific size checks
- Documentation improvements based on suggestions by Eric Auger
Replaced example code with a pointer to the actual code
- Added more checks for illegal flags combinations
- Added doc file to MAINTAINERS
v5
- Addjusted paddings in UAPI data to be 8 byte aligned
- Do not clobber argsz in IOMMU core before passing on to vendor driver
- Removed pr_warn_ for invalid UAPI data check, just return -EINVAL
- Clarified VFIO responsibility in UAPI data handling
- Use iommu_uapi prefix to differentiate APIs has in-kernel caller
- Added comment for unchecked flags of invalidation granularity
- Added example in doc to show vendor data checking
v4
- Added checks of UAPI data for reserved fields, version, and flags.
- Removed version check from vendor driver (vt-d)
- Relaxed argsz check to match the UAPI struct size instead of variable
union size
- Updated documentation
v3:
- Rewrote backward compatibility rule to support existing code
re-compiled with newer kernel UAPI header that runs on older
kernel. Based on review comment from Alex W.
https://lore.kernel.org/linux-iommu/[email protected]/
- Take user pointer directly in UAPI functions. Perform argsz check
and copy_from_user() in IOMMU driver. Eliminate the need for
VFIO or other upper layer to parse IOMMU data.
- Create wrapper function for in-kernel users of UAPI functions
v2:
- Removed unified API version and helper
- Introduced argsz for each UAPI data
- Introduced UAPI doc
Jacob Pan (7):
docs: IOMMU user API
iommu/uapi: Add argsz for user filled data
iommu/uapi: Introduce enum type for PASID data format
iommu/uapi: Use named union for user data
iommu/uapi: Rename uapi functions
iommu/uapi: Handle data and argsz filled by users
iommu/vt-d: Check UAPI data processed by IOMMU core
Documentation/userspace-api/iommu.rst | 212 ++++++++++++++++++++++++++++++++++
MAINTAINERS | 1 +
drivers/iommu/intel/iommu.c | 25 ++--
drivers/iommu/intel/svm.c | 9 +-
drivers/iommu/iommu.c | 205 ++++++++++++++++++++++++++++++--
include/linux/iommu.h | 35 ++++--
include/uapi/linux/iommu.h | 24 ++--
7 files changed, 466 insertions(+), 45 deletions(-)
create mode 100644 Documentation/userspace-api/iommu.rst
--
2.7.4
IOMMU user APIs are responsible for processing user data. This patch
changes the interface such that user pointers can be passed into IOMMU
code directly. Separate kernel APIs without user pointers are introduced
for in-kernel users of the UAPI functionality.
IOMMU UAPI data has a user filled argsz field which indicates the data
length of the structure. User data is not trusted, argsz must be
validated based on the current kernel data size, mandatory data size,
and feature flags.
User data may also be extended, resulting in possible argsz increase.
Backward compatibility is ensured based on size and flags (or
the functional equivalent fields) checking.
This patch adds sanity checks in the IOMMU layer. In addition to argsz,
reserved/unused fields in padding, flags, and version are also checked.
Details are documented in Documentation/userspace-api/iommu.rst
Signed-off-by: Liu Yi L <[email protected]>
Signed-off-by: Jacob Pan <[email protected]>
---
drivers/iommu/iommu.c | 201 ++++++++++++++++++++++++++++++++++++++++++++++++--
include/linux/iommu.h | 28 ++++---
2 files changed, 212 insertions(+), 17 deletions(-)
diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index 3a913ce94a3d..1ee55c4b3a3a 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -1950,33 +1950,218 @@ int iommu_attach_device(struct iommu_domain *domain, struct device *dev)
}
EXPORT_SYMBOL_GPL(iommu_attach_device);
+/*
+ * Check flags and other user provided data for valid combinations. We also
+ * make sure no reserved fields or unused flags are set. This is to ensure
+ * not breaking userspace in the future when these fields or flags are used.
+ */
+static int iommu_check_cache_invl_data(struct iommu_cache_invalidate_info *info)
+{
+ u32 mask;
+ int i;
+
+ if (info->version != IOMMU_CACHE_INVALIDATE_INFO_VERSION_1)
+ return -EINVAL;
+
+ mask = (1 << IOMMU_CACHE_INV_TYPE_NR) - 1;
+ if (info->cache & ~mask)
+ return -EINVAL;
+
+ if (info->granularity >= IOMMU_INV_GRANU_NR)
+ return -EINVAL;
+
+ switch (info->granularity) {
+ case IOMMU_INV_GRANU_ADDR:
+ if (info->cache & IOMMU_CACHE_INV_TYPE_PASID)
+ return -EINVAL;
+
+ mask = IOMMU_INV_ADDR_FLAGS_PASID |
+ IOMMU_INV_ADDR_FLAGS_ARCHID |
+ IOMMU_INV_ADDR_FLAGS_LEAF;
+
+ if (info->granu.addr_info.flags & ~mask)
+ return -EINVAL;
+ break;
+ case IOMMU_INV_GRANU_PASID:
+ mask = IOMMU_INV_PASID_FLAGS_PASID |
+ IOMMU_INV_PASID_FLAGS_ARCHID;
+ if (info->granu.pasid_info.flags & ~mask)
+ return -EINVAL;
+
+ break;
+ case IOMMU_INV_GRANU_DOMAIN:
+ if (info->cache & IOMMU_CACHE_INV_TYPE_DEV_IOTLB)
+ return -EINVAL;
+ break;
+ default:
+ return -EINVAL;
+ }
+
+ /* Check reserved padding fields */
+ for (i = 0; i < sizeof(info->padding); i++) {
+ if (info->padding[i])
+ return -EINVAL;
+ }
+
+ return 0;
+}
+
int iommu_uapi_cache_invalidate(struct iommu_domain *domain, struct device *dev,
- struct iommu_cache_invalidate_info *inv_info)
+ void __user *uinfo)
{
+ struct iommu_cache_invalidate_info inv_info = { 0 };
+ u32 minsz;
+ int ret = 0;
+
if (unlikely(!domain->ops->cache_invalidate))
return -ENODEV;
- return domain->ops->cache_invalidate(domain, dev, inv_info);
+ /*
+ * No new spaces can be added before the variable sized union, the
+ * minimum size is the offset to the union.
+ */
+ minsz = offsetof(struct iommu_cache_invalidate_info, granu);
+
+ /* Copy minsz from user to get flags and argsz */
+ if (copy_from_user(&inv_info, uinfo, minsz))
+ return -EFAULT;
+
+ /* Fields before variable size union is mandatory */
+ if (inv_info.argsz < minsz)
+ return -EINVAL;
+
+ /* PASID and address granu require additional info beyond minsz */
+ if (inv_info.argsz == minsz &&
+ ((inv_info.granularity == IOMMU_INV_GRANU_PASID) ||
+ (inv_info.granularity == IOMMU_INV_GRANU_ADDR)))
+ return -EINVAL;
+
+ if (inv_info.granularity == IOMMU_INV_GRANU_PASID &&
+ inv_info.argsz < offsetofend(struct iommu_cache_invalidate_info, granu.pasid_info))
+ return -EINVAL;
+
+ if (inv_info.granularity == IOMMU_INV_GRANU_ADDR &&
+ inv_info.argsz < offsetofend(struct iommu_cache_invalidate_info, granu.addr_info))
+ return -EINVAL;
+
+ /*
+ * User might be using a newer UAPI header which has a larger data
+ * size, we shall support the existing flags within the current
+ * size. Copy the remaining user data _after_ minsz but not more
+ * than the current kernel supported size.
+ */
+ if (copy_from_user((void *)&inv_info + minsz, uinfo + minsz,
+ min_t(u32, inv_info.argsz, sizeof(inv_info)) - minsz))
+ return -EFAULT;
+
+ /* Now the argsz is validated, check the content */
+ ret = iommu_check_cache_invl_data(&inv_info);
+ if (ret)
+ return ret;
+
+ return domain->ops->cache_invalidate(domain, dev, &inv_info);
}
EXPORT_SYMBOL_GPL(iommu_uapi_cache_invalidate);
-int iommu_uapi_sva_bind_gpasid(struct iommu_domain *domain,
- struct device *dev, struct iommu_gpasid_bind_data *data)
+static int iommu_check_bind_data(struct iommu_gpasid_bind_data *data)
+{
+ u32 mask;
+ int i;
+
+ if (data->version != IOMMU_GPASID_BIND_VERSION_1)
+ return -EINVAL;
+
+ /* Check the range of supported formats */
+ if (data->format >= IOMMU_PASID_FORMAT_LAST)
+ return -EINVAL;
+
+ /* Check all flags */
+ mask = IOMMU_SVA_GPASID_VAL;
+ if (data->flags & ~mask)
+ return -EINVAL;
+
+ /* Check reserved padding fields */
+ for (i = 0; i < sizeof(data->padding); i++) {
+ if (data->padding[i])
+ return -EINVAL;
+ }
+
+ return 0;
+}
+
+static int iommu_sva_prepare_bind_data(void __user *udata,
+ struct iommu_gpasid_bind_data *data)
{
+ u32 minsz;
+
+ /*
+ * No new spaces can be added before the variable sized union, the
+ * minimum size is the offset to the union.
+ */
+ minsz = offsetof(struct iommu_gpasid_bind_data, vendor);
+
+ /* Copy minsz from user to get flags and argsz */
+ if (copy_from_user(data, udata, minsz))
+ return -EFAULT;
+
+ /* Fields before variable size union is mandatory */
+ if (data->argsz < minsz)
+ return -EINVAL;
+ /*
+ * User might be using a newer UAPI header, we shall let IOMMU vendor
+ * driver decide on what size it needs. Since the guest PASID bind data
+ * can be vendor specific, larger argsz could be the result of extension
+ * for one vendor but it should not affect another vendor.
+ * Copy the remaining user data _after_ minsz
+ */
+ if (copy_from_user((void *)data + minsz, udata + minsz,
+ min_t(u32, data->argsz, sizeof(*data)) - minsz))
+ return -EFAULT;
+
+ return iommu_check_bind_data(data);
+}
+
+int iommu_uapi_sva_bind_gpasid(struct iommu_domain *domain, struct device *dev,
+ void __user *udata)
+{
+ struct iommu_gpasid_bind_data data = { 0 };
+ int ret;
+
if (unlikely(!domain->ops->sva_bind_gpasid))
return -ENODEV;
- return domain->ops->sva_bind_gpasid(domain, dev, data);
+ ret = iommu_sva_prepare_bind_data(udata, &data);
+ if (ret)
+ return ret;
+
+ return domain->ops->sva_bind_gpasid(domain, dev, &data);
}
EXPORT_SYMBOL_GPL(iommu_uapi_sva_bind_gpasid);
-int iommu_uapi_sva_unbind_gpasid(struct iommu_domain *domain, struct device *dev,
- ioasid_t pasid)
+int iommu_sva_unbind_gpasid(struct iommu_domain *domain, struct device *dev,
+ struct iommu_gpasid_bind_data *data)
{
if (unlikely(!domain->ops->sva_unbind_gpasid))
return -ENODEV;
- return domain->ops->sva_unbind_gpasid(dev, pasid);
+ return domain->ops->sva_unbind_gpasid(dev, data->hpasid);
+}
+EXPORT_SYMBOL_GPL(iommu_sva_unbind_gpasid);
+
+int iommu_uapi_sva_unbind_gpasid(struct iommu_domain *domain, struct device *dev,
+ void __user *udata)
+{
+ struct iommu_gpasid_bind_data data = { 0 };
+ int ret;
+
+ if (unlikely(!domain->ops->sva_bind_gpasid))
+ return -ENODEV;
+
+ ret = iommu_sva_prepare_bind_data(udata, &data);
+ if (ret)
+ return ret;
+
+ return iommu_sva_unbind_gpasid(domain, dev, &data);
}
EXPORT_SYMBOL_GPL(iommu_uapi_sva_unbind_gpasid);
diff --git a/include/linux/iommu.h b/include/linux/iommu.h
index 2dcc1a33f6dc..4a02c9e09048 100644
--- a/include/linux/iommu.h
+++ b/include/linux/iommu.h
@@ -432,11 +432,14 @@ extern void iommu_detach_device(struct iommu_domain *domain,
struct device *dev);
extern int iommu_uapi_cache_invalidate(struct iommu_domain *domain,
struct device *dev,
- struct iommu_cache_invalidate_info *inv_info);
+ void __user *uinfo);
+
extern int iommu_uapi_sva_bind_gpasid(struct iommu_domain *domain,
- struct device *dev, struct iommu_gpasid_bind_data *data);
+ struct device *dev, void __user *udata);
extern int iommu_uapi_sva_unbind_gpasid(struct iommu_domain *domain,
- struct device *dev, ioasid_t pasid);
+ struct device *dev, void __user *udata);
+extern int iommu_sva_unbind_gpasid(struct iommu_domain *domain,
+ struct device *dev, struct iommu_gpasid_bind_data *data);
extern struct iommu_domain *iommu_get_domain_for_dev(struct device *dev);
extern struct iommu_domain *iommu_get_dma_domain(struct device *dev);
extern int iommu_map(struct iommu_domain *domain, unsigned long iova,
@@ -1054,22 +1057,29 @@ static inline int iommu_sva_get_pasid(struct iommu_sva *handle)
return IOMMU_PASID_INVALID;
}
-static inline int iommu_uapi_cache_invalidate(struct iommu_domain *domain,
- struct device *dev,
- struct iommu_cache_invalidate_info *inv_info)
+static inline int
+iommu_uapi_cache_invalidate(struct iommu_domain *domain,
+ struct device *dev,
+ struct iommu_cache_invalidate_info *inv_info)
{
return -ENODEV;
}
static inline int iommu_uapi_sva_bind_gpasid(struct iommu_domain *domain,
- struct device *dev,
- struct iommu_gpasid_bind_data *data)
+ struct device *dev, void __user *udata)
{
return -ENODEV;
}
static inline int iommu_uapi_sva_unbind_gpasid(struct iommu_domain *domain,
- struct device *dev, int pasid)
+ struct device *dev, void __user *udata)
+{
+ return -ENODEV;
+}
+
+static inline int iommu_sva_unbind_gpasid(struct iommu_domain *domain,
+ struct device *dev,
+ struct iommu_gpasid_bind_data *data)
{
return -ENODEV;
}
--
2.7.4
Hi Jacob,
On 7/30/20 2:21 AM, Jacob Pan wrote:
> IOMMU user APIs are responsible for processing user data. This patch
> changes the interface such that user pointers can be passed into IOMMU
> code directly. Separate kernel APIs without user pointers are introduced
> for in-kernel users of the UAPI functionality.
This is just done for a single function, ie. iommu_sva_unbind_gpasid.
If I am not wrong there is no user of this latter after applying the
whole series? If correct you may remove it at this stage?
>
> IOMMU UAPI data has a user filled argsz field which indicates the data
> length of the structure. User data is not trusted, argsz must be
> validated based on the current kernel data size, mandatory data size,
> and feature flags.
>
> User data may also be extended, resulting in possible argsz increase.
> Backward compatibility is ensured based on size and flags (or
> the functional equivalent fields) checking.
>
> This patch adds sanity checks in the IOMMU layer. In addition to argsz,
> reserved/unused fields in padding, flags, and version are also checked.
> Details are documented in Documentation/userspace-api/iommu.rst
>
> Signed-off-by: Liu Yi L <[email protected]>
> Signed-off-by: Jacob Pan <[email protected]>
> ---
> drivers/iommu/iommu.c | 201 ++++++++++++++++++++++++++++++++++++++++++++++++--
> include/linux/iommu.h | 28 ++++---
> 2 files changed, 212 insertions(+), 17 deletions(-)
>
> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
> index 3a913ce94a3d..1ee55c4b3a3a 100644
> --- a/drivers/iommu/iommu.c
> +++ b/drivers/iommu/iommu.c
> @@ -1950,33 +1950,218 @@ int iommu_attach_device(struct iommu_domain *domain, struct device *dev)
> }
> EXPORT_SYMBOL_GPL(iommu_attach_device);
>
> +/*
> + * Check flags and other user provided data for valid combinations. We also
> + * make sure no reserved fields or unused flags are set. This is to ensure
> + * not breaking userspace in the future when these fields or flags are used.
> + */
> +static int iommu_check_cache_invl_data(struct iommu_cache_invalidate_info *info)
> +{
> + u32 mask;
> + int i;
> +
> + if (info->version != IOMMU_CACHE_INVALIDATE_INFO_VERSION_1)
> + return -EINVAL;
> +
> + mask = (1 << IOMMU_CACHE_INV_TYPE_NR) - 1;
> + if (info->cache & ~mask)
> + return -EINVAL;
> +
> + if (info->granularity >= IOMMU_INV_GRANU_NR)
> + return -EINVAL;
> +
> + switch (info->granularity) {
> + case IOMMU_INV_GRANU_ADDR:
> + if (info->cache & IOMMU_CACHE_INV_TYPE_PASID)
> + return -EINVAL;
> +
> + mask = IOMMU_INV_ADDR_FLAGS_PASID |
> + IOMMU_INV_ADDR_FLAGS_ARCHID |
> + IOMMU_INV_ADDR_FLAGS_LEAF;
> +
> + if (info->granu.addr_info.flags & ~mask)
> + return -EINVAL;
> + break;
> + case IOMMU_INV_GRANU_PASID:
> + mask = IOMMU_INV_PASID_FLAGS_PASID |
> + IOMMU_INV_PASID_FLAGS_ARCHID;
> + if (info->granu.pasid_info.flags & ~mask)
> + return -EINVAL;
> +
> + break;
> + case IOMMU_INV_GRANU_DOMAIN:
> + if (info->cache & IOMMU_CACHE_INV_TYPE_DEV_IOTLB)
> + return -EINVAL;
> + break;
> + default:
> + return -EINVAL;
> + }
> +
> + /* Check reserved padding fields */
> + for (i = 0; i < sizeof(info->padding); i++) {
> + if (info->padding[i])
> + return -EINVAL;
> + }
> +
> + return 0;
> +}
> +
> int iommu_uapi_cache_invalidate(struct iommu_domain *domain, struct device *dev,
> - struct iommu_cache_invalidate_info *inv_info)
> + void __user *uinfo)
> {
> + struct iommu_cache_invalidate_info inv_info = { 0 };
> + u32 minsz;
> + int ret = 0;
> +
> if (unlikely(!domain->ops->cache_invalidate))
> return -ENODEV;
> > - return domain->ops->cache_invalidate(domain, dev, inv_info);
> + /*
> + * No new spaces can be added before the variable sized union, the
> + * minimum size is the offset to the union.
> + */
> + minsz = offsetof(struct iommu_cache_invalidate_info, granu);
> +
> + /* Copy minsz from user to get flags and argsz */
> + if (copy_from_user(&inv_info, uinfo, minsz))
> + return -EFAULT;
> +
> + /* Fields before variable size union is mandatory */
> + if (inv_info.argsz < minsz)
> + return -EINVAL;
> +
> + /* PASID and address granu require additional info beyond minsz */
> + if (inv_info.argsz == minsz &&
> + ((inv_info.granularity == IOMMU_INV_GRANU_PASID) ||
> + (inv_info.granularity == IOMMU_INV_GRANU_ADDR)))
> + return -EINVAL;
> +
> + if (inv_info.granularity == IOMMU_INV_GRANU_PASID &&
> + inv_info.argsz < offsetofend(struct iommu_cache_invalidate_info, granu.pasid_info))
> + return -EINVAL;
> +
> + if (inv_info.granularity == IOMMU_INV_GRANU_ADDR &&
> + inv_info.argsz < offsetofend(struct iommu_cache_invalidate_info, granu.addr_info))
> + return -EINVAL;
> +
> + /*
> + * User might be using a newer UAPI header which has a larger data
> + * size, we shall support the existing flags within the current
> + * size. Copy the remaining user data _after_ minsz but not more
> + * than the current kernel supported size.
> + */
> + if (copy_from_user((void *)&inv_info + minsz, uinfo + minsz,
> + min_t(u32, inv_info.argsz, sizeof(inv_info)) - minsz))
> + return -EFAULT;
> +
> + /* Now the argsz is validated, check the content */
> + ret = iommu_check_cache_invl_data(&inv_info);
> + if (ret)
> + return ret;
> +
> + return domain->ops->cache_invalidate(domain, dev, &inv_info);
> }
> EXPORT_SYMBOL_GPL(iommu_uapi_cache_invalidate);
>
> -int iommu_uapi_sva_bind_gpasid(struct iommu_domain *domain,
> - struct device *dev, struct iommu_gpasid_bind_data *data)
> +static int iommu_check_bind_data(struct iommu_gpasid_bind_data *data)
> +{
> + u32 mask;
> + int i;
> +
> + if (data->version != IOMMU_GPASID_BIND_VERSION_1)
> + return -EINVAL;
> +
> + /* Check the range of supported formats */
> + if (data->format >= IOMMU_PASID_FORMAT_LAST)
> + return -EINVAL;
> +
> + /* Check all flags */
> + mask = IOMMU_SVA_GPASID_VAL;
> + if (data->flags & ~mask)
> + return -EINVAL;
> +
> + /* Check reserved padding fields */
> + for (i = 0; i < sizeof(data->padding); i++) {
> + if (data->padding[i])
> + return -EINVAL;
> + }
> +
> + return 0;
> +}
> +
> +static int iommu_sva_prepare_bind_data(void __user *udata,
> + struct iommu_gpasid_bind_data *data)
> {
> + u32 minsz;
> +
> + /*
> + * No new spaces can be added before the variable sized union, the
> + * minimum size is the offset to the union.
> + */
> + minsz = offsetof(struct iommu_gpasid_bind_data, vendor);
> +
> + /* Copy minsz from user to get flags and argsz */
> + if (copy_from_user(data, udata, minsz))
> + return -EFAULT;
> +
> + /* Fields before variable size union is mandatory */
> + if (data->argsz < minsz)
> + return -EINVAL;
> + /*
> + * User might be using a newer UAPI header, we shall let IOMMU vendor
> + * driver decide on what size it needs. Since the guest PASID bind data
> + * can be vendor specific, larger argsz could be the result of extension
> + * for one vendor but it should not affect another vendor.
> + * Copy the remaining user data _after_ minsz
> + */
> + if (copy_from_user((void *)data + minsz, udata + minsz,
> + min_t(u32, data->argsz, sizeof(*data)) - minsz))
> + return -EFAULT;
> +
> + return iommu_check_bind_data(data);
> +}
> +
> +int iommu_uapi_sva_bind_gpasid(struct iommu_domain *domain, struct device *dev,
> + void __user *udata)
> +{
> + struct iommu_gpasid_bind_data data = { 0 };
> + int ret;
> +
> if (unlikely(!domain->ops->sva_bind_gpasid))
> return -ENODEV;
>
> - return domain->ops->sva_bind_gpasid(domain, dev, data);
> + ret = iommu_sva_prepare_bind_data(udata, &data);
> + if (ret)
> + return ret;
> +
> + return domain->ops->sva_bind_gpasid(domain, dev, &data);
> }
> EXPORT_SYMBOL_GPL(iommu_uapi_sva_bind_gpasid);
>
> -int iommu_uapi_sva_unbind_gpasid(struct iommu_domain *domain, struct device *dev,
> - ioasid_t pasid)
> +int iommu_sva_unbind_gpasid(struct iommu_domain *domain, struct device *dev,
> + struct iommu_gpasid_bind_data *data)
> {
> if (unlikely(!domain->ops->sva_unbind_gpasid))
> return -ENODEV;
>
> - return domain->ops->sva_unbind_gpasid(dev, pasid);
> + return domain->ops->sva_unbind_gpasid(dev, data->hpasid);
> +}
> +EXPORT_SYMBOL_GPL(iommu_sva_unbind_gpasid);
> +
> +int iommu_uapi_sva_unbind_gpasid(struct iommu_domain *domain, struct device *dev,
> + void __user *udata)
> +{
> + struct iommu_gpasid_bind_data data = { 0 };
> + int ret;
> +
> + if (unlikely(!domain->ops->sva_bind_gpasid))
> + return -ENODEV;
> +
> + ret = iommu_sva_prepare_bind_data(udata, &data);
> + if (ret)
> + return ret;
> +
> + return iommu_sva_unbind_gpasid(domain, dev, &data);
> }
> EXPORT_SYMBOL_GPL(iommu_uapi_sva_unbind_gpasid);
>
> diff --git a/include/linux/iommu.h b/include/linux/iommu.h
> index 2dcc1a33f6dc..4a02c9e09048 100644
> --- a/include/linux/iommu.h
> +++ b/include/linux/iommu.h
> @@ -432,11 +432,14 @@ extern void iommu_detach_device(struct iommu_domain *domain,
> struct device *dev);
> extern int iommu_uapi_cache_invalidate(struct iommu_domain *domain,
> struct device *dev,
> - struct iommu_cache_invalidate_info *inv_info);
> + void __user *uinfo);
> +
> extern int iommu_uapi_sva_bind_gpasid(struct iommu_domain *domain,
> - struct device *dev, struct iommu_gpasid_bind_data *data);
> + struct device *dev, void __user *udata);
> extern int iommu_uapi_sva_unbind_gpasid(struct iommu_domain *domain,
> - struct device *dev, ioasid_t pasid);
> + struct device *dev, void __user *udata);
> +extern int iommu_sva_unbind_gpasid(struct iommu_domain *domain,
> + struct device *dev, struct iommu_gpasid_bind_data *data);
> extern struct iommu_domain *iommu_get_domain_for_dev(struct device *dev);
> extern struct iommu_domain *iommu_get_dma_domain(struct device *dev);
> extern int iommu_map(struct iommu_domain *domain, unsigned long iova,
> @@ -1054,22 +1057,29 @@ static inline int iommu_sva_get_pasid(struct iommu_sva *handle)
> return IOMMU_PASID_INVALID;
> }
>
> -static inline int iommu_uapi_cache_invalidate(struct iommu_domain *domain,
> - struct device *dev,
> - struct iommu_cache_invalidate_info *inv_info)
> +static inline int
> +iommu_uapi_cache_invalidate(struct iommu_domain *domain,
> + struct device *dev,
> + struct iommu_cache_invalidate_info *inv_info)
> {
> return -ENODEV;
> }
>
> static inline int iommu_uapi_sva_bind_gpasid(struct iommu_domain *domain,
> - struct device *dev,
> - struct iommu_gpasid_bind_data *data)
> + struct device *dev, void __user *udata)
> {
> return -ENODEV;
> }
>
> static inline int iommu_uapi_sva_unbind_gpasid(struct iommu_domain *domain,
> - struct device *dev, int pasid)
> + struct device *dev, void __user *udata)
> +{
> + return -ENODEV;
> +}
> +
> +static inline int iommu_sva_unbind_gpasid(struct iommu_domain *domain,
> + struct device *dev,
> + struct iommu_gpasid_bind_data *data)
> {
> return -ENODEV;
> }
>
Otherwise looks good to me
Reviewed-by: Eric Auger <[email protected]>
Thanks
Eric
Hi Eric,
> From: Auger Eric <[email protected]>
> Sent: Thursday, August 13, 2020 5:12 PM
>
> Hi Jacob,
>
> On 7/30/20 2:21 AM, Jacob Pan wrote:
> > IOMMU user APIs are responsible for processing user data. This patch
> > changes the interface such that user pointers can be passed into IOMMU
> > code directly. Separate kernel APIs without user pointers are introduced
> > for in-kernel users of the UAPI functionality.
> This is just done for a single function, ie. iommu_sva_unbind_gpasid.
>
> If I am not wrong there is no user of this latter after applying the
> whole series? If correct you may remove it at this stage?
the user of this function is in vfio. And it is the same with
iommu_uapi_sva_bind/unbind_gpasid() and iommu_uapi_cache_invalidate().
https://lore.kernel.org/kvm/[email protected]/
https://lore.kernel.org/kvm/[email protected]/
Regards,
Yi Liu
> >
> > IOMMU UAPI data has a user filled argsz field which indicates the data
> > length of the structure. User data is not trusted, argsz must be
> > validated based on the current kernel data size, mandatory data size,
> > and feature flags.
> >
> > User data may also be extended, resulting in possible argsz increase.
> > Backward compatibility is ensured based on size and flags (or
> > the functional equivalent fields) checking.
> >
> > This patch adds sanity checks in the IOMMU layer. In addition to argsz,
> > reserved/unused fields in padding, flags, and version are also checked.
> > Details are documented in Documentation/userspace-api/iommu.rst
> >
> > Signed-off-by: Liu Yi L <[email protected]>
> > Signed-off-by: Jacob Pan <[email protected]>
> > ---
> > drivers/iommu/iommu.c | 201
> ++++++++++++++++++++++++++++++++++++++++++++++++--
> > include/linux/iommu.h | 28 ++++---
> > 2 files changed, 212 insertions(+), 17 deletions(-)
> >
> > diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
> > index 3a913ce94a3d..1ee55c4b3a3a 100644
> > --- a/drivers/iommu/iommu.c
> > +++ b/drivers/iommu/iommu.c
> > @@ -1950,33 +1950,218 @@ int iommu_attach_device(struct iommu_domain
> *domain, struct device *dev)
> > }
> > EXPORT_SYMBOL_GPL(iommu_attach_device);
> >
> > +/*
> > + * Check flags and other user provided data for valid combinations. We also
> > + * make sure no reserved fields or unused flags are set. This is to ensure
> > + * not breaking userspace in the future when these fields or flags are used.
> > + */
> > +static int iommu_check_cache_invl_data(struct iommu_cache_invalidate_info
> *info)
> > +{
> > + u32 mask;
> > + int i;
> > +
> > + if (info->version != IOMMU_CACHE_INVALIDATE_INFO_VERSION_1)
> > + return -EINVAL;
> > +
> > + mask = (1 << IOMMU_CACHE_INV_TYPE_NR) - 1;
> > + if (info->cache & ~mask)
> > + return -EINVAL;
> > +
> > + if (info->granularity >= IOMMU_INV_GRANU_NR)
> > + return -EINVAL;
> > +
> > + switch (info->granularity) {
> > + case IOMMU_INV_GRANU_ADDR:
> > + if (info->cache & IOMMU_CACHE_INV_TYPE_PASID)
> > + return -EINVAL;
> > +
> > + mask = IOMMU_INV_ADDR_FLAGS_PASID |
> > + IOMMU_INV_ADDR_FLAGS_ARCHID |
> > + IOMMU_INV_ADDR_FLAGS_LEAF;
> > +
> > + if (info->granu.addr_info.flags & ~mask)
> > + return -EINVAL;
> > + break;
> > + case IOMMU_INV_GRANU_PASID:
> > + mask = IOMMU_INV_PASID_FLAGS_PASID |
> > + IOMMU_INV_PASID_FLAGS_ARCHID;
> > + if (info->granu.pasid_info.flags & ~mask)
> > + return -EINVAL;
> > +
> > + break;
> > + case IOMMU_INV_GRANU_DOMAIN:
> > + if (info->cache & IOMMU_CACHE_INV_TYPE_DEV_IOTLB)
> > + return -EINVAL;
> > + break;
> > + default:
> > + return -EINVAL;
> > + }
> > +
> > + /* Check reserved padding fields */
> > + for (i = 0; i < sizeof(info->padding); i++) {
> > + if (info->padding[i])
> > + return -EINVAL;
> > + }
> > +
> > + return 0;
> > +}
> > +
> > int iommu_uapi_cache_invalidate(struct iommu_domain *domain, struct device
> *dev,
> > - struct iommu_cache_invalidate_info *inv_info)
> > + void __user *uinfo)
> > {
> > + struct iommu_cache_invalidate_info inv_info = { 0 };
> > + u32 minsz;
> > + int ret = 0;
> > +
> > if (unlikely(!domain->ops->cache_invalidate))
> > return -ENODEV;
> > > - return domain->ops->cache_invalidate(domain, dev, inv_info);
> > + /*
> > + * No new spaces can be added before the variable sized union, the
> > + * minimum size is the offset to the union.
> > + */
> > + minsz = offsetof(struct iommu_cache_invalidate_info, granu);
> > +
> > + /* Copy minsz from user to get flags and argsz */
> > + if (copy_from_user(&inv_info, uinfo, minsz))
> > + return -EFAULT;
> > +
> > + /* Fields before variable size union is mandatory */
> > + if (inv_info.argsz < minsz)
> > + return -EINVAL;
> > +
> > + /* PASID and address granu require additional info beyond minsz */
> > + if (inv_info.argsz == minsz &&
> > + ((inv_info.granularity == IOMMU_INV_GRANU_PASID) ||
> > + (inv_info.granularity == IOMMU_INV_GRANU_ADDR)))
> > + return -EINVAL;
> > +
> > + if (inv_info.granularity == IOMMU_INV_GRANU_PASID &&
> > + inv_info.argsz < offsetofend(struct iommu_cache_invalidate_info,
> granu.pasid_info))
> > + return -EINVAL;
> > +
> > + if (inv_info.granularity == IOMMU_INV_GRANU_ADDR &&
> > + inv_info.argsz < offsetofend(struct iommu_cache_invalidate_info,
> granu.addr_info))
> > + return -EINVAL;
> > +
> > + /*
> > + * User might be using a newer UAPI header which has a larger data
> > + * size, we shall support the existing flags within the current
> > + * size. Copy the remaining user data _after_ minsz but not more
> > + * than the current kernel supported size.
> > + */
> > + if (copy_from_user((void *)&inv_info + minsz, uinfo + minsz,
> > + min_t(u32, inv_info.argsz, sizeof(inv_info)) - minsz))
> > + return -EFAULT;
> > +
> > + /* Now the argsz is validated, check the content */
> > + ret = iommu_check_cache_invl_data(&inv_info);
> > + if (ret)
> > + return ret;
> > +
> > + return domain->ops->cache_invalidate(domain, dev, &inv_info);
> > }
> > EXPORT_SYMBOL_GPL(iommu_uapi_cache_invalidate);
> >
> > -int iommu_uapi_sva_bind_gpasid(struct iommu_domain *domain,
> > - struct device *dev, struct iommu_gpasid_bind_data
> *data)
> > +static int iommu_check_bind_data(struct iommu_gpasid_bind_data *data)
> > +{
> > + u32 mask;
> > + int i;
> > +
> > + if (data->version != IOMMU_GPASID_BIND_VERSION_1)
> > + return -EINVAL;
> > +
> > + /* Check the range of supported formats */
> > + if (data->format >= IOMMU_PASID_FORMAT_LAST)
> > + return -EINVAL;
> > +
> > + /* Check all flags */
> > + mask = IOMMU_SVA_GPASID_VAL;
> > + if (data->flags & ~mask)
> > + return -EINVAL;
> > +
> > + /* Check reserved padding fields */
> > + for (i = 0; i < sizeof(data->padding); i++) {
> > + if (data->padding[i])
> > + return -EINVAL;
> > + }
> > +
> > + return 0;
> > +}
> > +
> > +static int iommu_sva_prepare_bind_data(void __user *udata,
> > + struct iommu_gpasid_bind_data *data)
> > {
> > + u32 minsz;
> > +
> > + /*
> > + * No new spaces can be added before the variable sized union, the
> > + * minimum size is the offset to the union.
> > + */
> > + minsz = offsetof(struct iommu_gpasid_bind_data, vendor);
> > +
> > + /* Copy minsz from user to get flags and argsz */
> > + if (copy_from_user(data, udata, minsz))
> > + return -EFAULT;
> > +
> > + /* Fields before variable size union is mandatory */
> > + if (data->argsz < minsz)
> > + return -EINVAL;
> > + /*
> > + * User might be using a newer UAPI header, we shall let IOMMU vendor
> > + * driver decide on what size it needs. Since the guest PASID bind data
> > + * can be vendor specific, larger argsz could be the result of extension
> > + * for one vendor but it should not affect another vendor.
> > + * Copy the remaining user data _after_ minsz
> > + */
> > + if (copy_from_user((void *)data + minsz, udata + minsz,
> > + min_t(u32, data->argsz, sizeof(*data)) - minsz))
> > + return -EFAULT;
> > +
> > + return iommu_check_bind_data(data);
> > +}
> > +
> > +int iommu_uapi_sva_bind_gpasid(struct iommu_domain *domain, struct device
> *dev,
> > + void __user *udata)
> > +{
> > + struct iommu_gpasid_bind_data data = { 0 };
> > + int ret;
> > +
> > if (unlikely(!domain->ops->sva_bind_gpasid))
> > return -ENODEV;
> >
> > - return domain->ops->sva_bind_gpasid(domain, dev, data);
> > + ret = iommu_sva_prepare_bind_data(udata, &data);
> > + if (ret)
> > + return ret;
> > +
> > + return domain->ops->sva_bind_gpasid(domain, dev, &data);
> > }
> > EXPORT_SYMBOL_GPL(iommu_uapi_sva_bind_gpasid);
> >
> > -int iommu_uapi_sva_unbind_gpasid(struct iommu_domain *domain, struct device
> *dev,
> > - ioasid_t pasid)
> > +int iommu_sva_unbind_gpasid(struct iommu_domain *domain, struct device *dev,
> > + struct iommu_gpasid_bind_data *data)
> > {
> > if (unlikely(!domain->ops->sva_unbind_gpasid))
> > return -ENODEV;
> >
> > - return domain->ops->sva_unbind_gpasid(dev, pasid);
> > + return domain->ops->sva_unbind_gpasid(dev, data->hpasid);
> > +}
> > +EXPORT_SYMBOL_GPL(iommu_sva_unbind_gpasid);
> > +
> > +int iommu_uapi_sva_unbind_gpasid(struct iommu_domain *domain, struct device
> *dev,
> > + void __user *udata)
> > +{
> > + struct iommu_gpasid_bind_data data = { 0 };
> > + int ret;
> > +
> > + if (unlikely(!domain->ops->sva_bind_gpasid))
> > + return -ENODEV;
> > +
> > + ret = iommu_sva_prepare_bind_data(udata, &data);
> > + if (ret)
> > + return ret;
> > +
> > + return iommu_sva_unbind_gpasid(domain, dev, &data);
> > }
> > EXPORT_SYMBOL_GPL(iommu_uapi_sva_unbind_gpasid);
> >
> > diff --git a/include/linux/iommu.h b/include/linux/iommu.h
> > index 2dcc1a33f6dc..4a02c9e09048 100644
> > --- a/include/linux/iommu.h
> > +++ b/include/linux/iommu.h
> > @@ -432,11 +432,14 @@ extern void iommu_detach_device(struct
> iommu_domain *domain,
> > struct device *dev);
> > extern int iommu_uapi_cache_invalidate(struct iommu_domain *domain,
> > struct device *dev,
> > - struct iommu_cache_invalidate_info *inv_info);
> > + void __user *uinfo);
> > +
> > extern int iommu_uapi_sva_bind_gpasid(struct iommu_domain *domain,
> > - struct device *dev, struct
> iommu_gpasid_bind_data *data);
> > + struct device *dev, void __user *udata);
> > extern int iommu_uapi_sva_unbind_gpasid(struct iommu_domain *domain,
> > - struct device *dev, ioasid_t pasid);
> > + struct device *dev, void __user *udata);
> > +extern int iommu_sva_unbind_gpasid(struct iommu_domain *domain,
> > + struct device *dev, struct
> iommu_gpasid_bind_data *data);
> > extern struct iommu_domain *iommu_get_domain_for_dev(struct device *dev);
> > extern struct iommu_domain *iommu_get_dma_domain(struct device *dev);
> > extern int iommu_map(struct iommu_domain *domain, unsigned long iova,
> > @@ -1054,22 +1057,29 @@ static inline int iommu_sva_get_pasid(struct
> iommu_sva *handle)
> > return IOMMU_PASID_INVALID;
> > }
> >
> > -static inline int iommu_uapi_cache_invalidate(struct iommu_domain *domain,
> > - struct device *dev,
> > - struct iommu_cache_invalidate_info
> *inv_info)
> > +static inline int
> > +iommu_uapi_cache_invalidate(struct iommu_domain *domain,
> > + struct device *dev,
> > + struct iommu_cache_invalidate_info *inv_info)
> > {
> > return -ENODEV;
> > }
> >
> > static inline int iommu_uapi_sva_bind_gpasid(struct iommu_domain *domain,
> > - struct device *dev,
> > - struct iommu_gpasid_bind_data *data)
> > + struct device *dev, void __user *udata)
> > {
> > return -ENODEV;
> > }
> >
> > static inline int iommu_uapi_sva_unbind_gpasid(struct iommu_domain *domain,
> > - struct device *dev, int pasid)
> > + struct device *dev, void __user *udata)
> > +{
> > + return -ENODEV;
> > +}
> > +
> > +static inline int iommu_sva_unbind_gpasid(struct iommu_domain *domain,
> > + struct device *dev,
> > + struct iommu_gpasid_bind_data *data)
> > {
> > return -ENODEV;
> > }
> >
> Otherwise looks good to me
> Reviewed-by: Eric Auger <[email protected]>
>
> Thanks
>
> Eric
Hi Yi,
On 8/13/20 11:25 AM, Liu, Yi L wrote:
> Hi Eric,
>
>
>> From: Auger Eric <[email protected]>
>> Sent: Thursday, August 13, 2020 5:12 PM
>>
>> Hi Jacob,
>>
>> On 7/30/20 2:21 AM, Jacob Pan wrote:
>>> IOMMU user APIs are responsible for processing user data. This patch
>>> changes the interface such that user pointers can be passed into IOMMU
>>> code directly. Separate kernel APIs without user pointers are introduced
>>> for in-kernel users of the UAPI functionality.
>> This is just done for a single function, ie. iommu_sva_unbind_gpasid.
>>
>> If I am not wrong there is no user of this latter after applying the
>> whole series? If correct you may remove it at this stage?
>
> the user of this function is in vfio. And it is the same with
> iommu_uapi_sva_bind/unbind_gpasid() and iommu_uapi_cache_invalidate().
>
> https://lore.kernel.org/kvm/[email protected]/
> https://lore.kernel.org/kvm/[email protected]/
Yep I know ;-) But this series mostly deals with iommu uapi rework.
That's not a big deal though.
Thanks
Eric
>
> Regards,
> Yi Liu
>
>>>
>>> IOMMU UAPI data has a user filled argsz field which indicates the data
>>> length of the structure. User data is not trusted, argsz must be
>>> validated based on the current kernel data size, mandatory data size,
>>> and feature flags.
>>>
>>> User data may also be extended, resulting in possible argsz increase.
>>> Backward compatibility is ensured based on size and flags (or
>>> the functional equivalent fields) checking.
>>>
>>> This patch adds sanity checks in the IOMMU layer. In addition to argsz,
>>> reserved/unused fields in padding, flags, and version are also checked.
>>> Details are documented in Documentation/userspace-api/iommu.rst
>>>
>>> Signed-off-by: Liu Yi L <[email protected]>
>>> Signed-off-by: Jacob Pan <[email protected]>
>>> ---
>>> drivers/iommu/iommu.c | 201
>> ++++++++++++++++++++++++++++++++++++++++++++++++--
>>> include/linux/iommu.h | 28 ++++---
>>> 2 files changed, 212 insertions(+), 17 deletions(-)
>>>
>>> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
>>> index 3a913ce94a3d..1ee55c4b3a3a 100644
>>> --- a/drivers/iommu/iommu.c
>>> +++ b/drivers/iommu/iommu.c
>>> @@ -1950,33 +1950,218 @@ int iommu_attach_device(struct iommu_domain
>> *domain, struct device *dev)
>>> }
>>> EXPORT_SYMBOL_GPL(iommu_attach_device);
>>>
>>> +/*
>>> + * Check flags and other user provided data for valid combinations. We also
>>> + * make sure no reserved fields or unused flags are set. This is to ensure
>>> + * not breaking userspace in the future when these fields or flags are used.
>>> + */
>>> +static int iommu_check_cache_invl_data(struct iommu_cache_invalidate_info
>> *info)
>>> +{
>>> + u32 mask;
>>> + int i;
>>> +
>>> + if (info->version != IOMMU_CACHE_INVALIDATE_INFO_VERSION_1)
>>> + return -EINVAL;
>>> +
>>> + mask = (1 << IOMMU_CACHE_INV_TYPE_NR) - 1;
>>> + if (info->cache & ~mask)
>>> + return -EINVAL;
>>> +
>>> + if (info->granularity >= IOMMU_INV_GRANU_NR)
>>> + return -EINVAL;
>>> +
>>> + switch (info->granularity) {
>>> + case IOMMU_INV_GRANU_ADDR:
>>> + if (info->cache & IOMMU_CACHE_INV_TYPE_PASID)
>>> + return -EINVAL;
>>> +
>>> + mask = IOMMU_INV_ADDR_FLAGS_PASID |
>>> + IOMMU_INV_ADDR_FLAGS_ARCHID |
>>> + IOMMU_INV_ADDR_FLAGS_LEAF;
>>> +
>>> + if (info->granu.addr_info.flags & ~mask)
>>> + return -EINVAL;
>>> + break;
>>> + case IOMMU_INV_GRANU_PASID:
>>> + mask = IOMMU_INV_PASID_FLAGS_PASID |
>>> + IOMMU_INV_PASID_FLAGS_ARCHID;
>>> + if (info->granu.pasid_info.flags & ~mask)
>>> + return -EINVAL;
>>> +
>>> + break;
>>> + case IOMMU_INV_GRANU_DOMAIN:
>>> + if (info->cache & IOMMU_CACHE_INV_TYPE_DEV_IOTLB)
>>> + return -EINVAL;
>>> + break;
>>> + default:
>>> + return -EINVAL;
>>> + }
>>> +
>>> + /* Check reserved padding fields */
>>> + for (i = 0; i < sizeof(info->padding); i++) {
>>> + if (info->padding[i])
>>> + return -EINVAL;
>>> + }
>>> +
>>> + return 0;
>>> +}
>>> +
>>> int iommu_uapi_cache_invalidate(struct iommu_domain *domain, struct device
>> *dev,
>>> - struct iommu_cache_invalidate_info *inv_info)
>>> + void __user *uinfo)
>>> {
>>> + struct iommu_cache_invalidate_info inv_info = { 0 };
>>> + u32 minsz;
>>> + int ret = 0;
>>> +
>>> if (unlikely(!domain->ops->cache_invalidate))
>>> return -ENODEV;
>>> > - return domain->ops->cache_invalidate(domain, dev, inv_info);
>>> + /*
>>> + * No new spaces can be added before the variable sized union, the
>>> + * minimum size is the offset to the union.
>>> + */
>>> + minsz = offsetof(struct iommu_cache_invalidate_info, granu);
>>> +
>>> + /* Copy minsz from user to get flags and argsz */
>>> + if (copy_from_user(&inv_info, uinfo, minsz))
>>> + return -EFAULT;
>>> +
>>> + /* Fields before variable size union is mandatory */
>>> + if (inv_info.argsz < minsz)
>>> + return -EINVAL;
>>> +
>>> + /* PASID and address granu require additional info beyond minsz */
>>> + if (inv_info.argsz == minsz &&
>>> + ((inv_info.granularity == IOMMU_INV_GRANU_PASID) ||
>>> + (inv_info.granularity == IOMMU_INV_GRANU_ADDR)))
>>> + return -EINVAL;
>>> +
>>> + if (inv_info.granularity == IOMMU_INV_GRANU_PASID &&
>>> + inv_info.argsz < offsetofend(struct iommu_cache_invalidate_info,
>> granu.pasid_info))
>>> + return -EINVAL;
>>> +
>>> + if (inv_info.granularity == IOMMU_INV_GRANU_ADDR &&
>>> + inv_info.argsz < offsetofend(struct iommu_cache_invalidate_info,
>> granu.addr_info))
>>> + return -EINVAL;
>>> +
>>> + /*
>>> + * User might be using a newer UAPI header which has a larger data
>>> + * size, we shall support the existing flags within the current
>>> + * size. Copy the remaining user data _after_ minsz but not more
>>> + * than the current kernel supported size.
>>> + */
>>> + if (copy_from_user((void *)&inv_info + minsz, uinfo + minsz,
>>> + min_t(u32, inv_info.argsz, sizeof(inv_info)) - minsz))
>>> + return -EFAULT;
>>> +
>>> + /* Now the argsz is validated, check the content */
>>> + ret = iommu_check_cache_invl_data(&inv_info);
>>> + if (ret)
>>> + return ret;
>>> +
>>> + return domain->ops->cache_invalidate(domain, dev, &inv_info);
>>> }
>>> EXPORT_SYMBOL_GPL(iommu_uapi_cache_invalidate);
>>>
>>> -int iommu_uapi_sva_bind_gpasid(struct iommu_domain *domain,
>>> - struct device *dev, struct iommu_gpasid_bind_data
>> *data)
>>> +static int iommu_check_bind_data(struct iommu_gpasid_bind_data *data)
>>> +{
>>> + u32 mask;
>>> + int i;
>>> +
>>> + if (data->version != IOMMU_GPASID_BIND_VERSION_1)
>>> + return -EINVAL;
>>> +
>>> + /* Check the range of supported formats */
>>> + if (data->format >= IOMMU_PASID_FORMAT_LAST)
>>> + return -EINVAL;
>>> +
>>> + /* Check all flags */
>>> + mask = IOMMU_SVA_GPASID_VAL;
>>> + if (data->flags & ~mask)
>>> + return -EINVAL;
>>> +
>>> + /* Check reserved padding fields */
>>> + for (i = 0; i < sizeof(data->padding); i++) {
>>> + if (data->padding[i])
>>> + return -EINVAL;
>>> + }
>>> +
>>> + return 0;
>>> +}
>>> +
>>> +static int iommu_sva_prepare_bind_data(void __user *udata,
>>> + struct iommu_gpasid_bind_data *data)
>>> {
>>> + u32 minsz;
>>> +
>>> + /*
>>> + * No new spaces can be added before the variable sized union, the
>>> + * minimum size is the offset to the union.
>>> + */
>>> + minsz = offsetof(struct iommu_gpasid_bind_data, vendor);
>>> +
>>> + /* Copy minsz from user to get flags and argsz */
>>> + if (copy_from_user(data, udata, minsz))
>>> + return -EFAULT;
>>> +
>>> + /* Fields before variable size union is mandatory */
>>> + if (data->argsz < minsz)
>>> + return -EINVAL;
>>> + /*
>>> + * User might be using a newer UAPI header, we shall let IOMMU vendor
>>> + * driver decide on what size it needs. Since the guest PASID bind data
>>> + * can be vendor specific, larger argsz could be the result of extension
>>> + * for one vendor but it should not affect another vendor.
>>> + * Copy the remaining user data _after_ minsz
>>> + */
>>> + if (copy_from_user((void *)data + minsz, udata + minsz,
>>> + min_t(u32, data->argsz, sizeof(*data)) - minsz))
>>> + return -EFAULT;
>>> +
>>> + return iommu_check_bind_data(data);
>>> +}
>>> +
>>> +int iommu_uapi_sva_bind_gpasid(struct iommu_domain *domain, struct device
>> *dev,
>>> + void __user *udata)
>>> +{
>>> + struct iommu_gpasid_bind_data data = { 0 };
>>> + int ret;
>>> +
>>> if (unlikely(!domain->ops->sva_bind_gpasid))
>>> return -ENODEV;
>>>
>>> - return domain->ops->sva_bind_gpasid(domain, dev, data);
>>> + ret = iommu_sva_prepare_bind_data(udata, &data);
>>> + if (ret)
>>> + return ret;
>>> +
>>> + return domain->ops->sva_bind_gpasid(domain, dev, &data);
>>> }
>>> EXPORT_SYMBOL_GPL(iommu_uapi_sva_bind_gpasid);
>>>
>>> -int iommu_uapi_sva_unbind_gpasid(struct iommu_domain *domain, struct device
>> *dev,
>>> - ioasid_t pasid)
>>> +int iommu_sva_unbind_gpasid(struct iommu_domain *domain, struct device *dev,
>>> + struct iommu_gpasid_bind_data *data)
>>> {
>>> if (unlikely(!domain->ops->sva_unbind_gpasid))
>>> return -ENODEV;
>>>
>>> - return domain->ops->sva_unbind_gpasid(dev, pasid);
>>> + return domain->ops->sva_unbind_gpasid(dev, data->hpasid);
>>> +}
>>> +EXPORT_SYMBOL_GPL(iommu_sva_unbind_gpasid);
>>> +
>>> +int iommu_uapi_sva_unbind_gpasid(struct iommu_domain *domain, struct device
>> *dev,
>>> + void __user *udata)
>>> +{
>>> + struct iommu_gpasid_bind_data data = { 0 };
>>> + int ret;
>>> +
>>> + if (unlikely(!domain->ops->sva_bind_gpasid))
>>> + return -ENODEV;
>>> +
>>> + ret = iommu_sva_prepare_bind_data(udata, &data);
>>> + if (ret)
>>> + return ret;
>>> +
>>> + return iommu_sva_unbind_gpasid(domain, dev, &data);
>>> }
>>> EXPORT_SYMBOL_GPL(iommu_uapi_sva_unbind_gpasid);
>>>
>>> diff --git a/include/linux/iommu.h b/include/linux/iommu.h
>>> index 2dcc1a33f6dc..4a02c9e09048 100644
>>> --- a/include/linux/iommu.h
>>> +++ b/include/linux/iommu.h
>>> @@ -432,11 +432,14 @@ extern void iommu_detach_device(struct
>> iommu_domain *domain,
>>> struct device *dev);
>>> extern int iommu_uapi_cache_invalidate(struct iommu_domain *domain,
>>> struct device *dev,
>>> - struct iommu_cache_invalidate_info *inv_info);
>>> + void __user *uinfo);
>>> +
>>> extern int iommu_uapi_sva_bind_gpasid(struct iommu_domain *domain,
>>> - struct device *dev, struct
>> iommu_gpasid_bind_data *data);
>>> + struct device *dev, void __user *udata);
>>> extern int iommu_uapi_sva_unbind_gpasid(struct iommu_domain *domain,
>>> - struct device *dev, ioasid_t pasid);
>>> + struct device *dev, void __user *udata);
>>> +extern int iommu_sva_unbind_gpasid(struct iommu_domain *domain,
>>> + struct device *dev, struct
>> iommu_gpasid_bind_data *data);
>>> extern struct iommu_domain *iommu_get_domain_for_dev(struct device *dev);
>>> extern struct iommu_domain *iommu_get_dma_domain(struct device *dev);
>>> extern int iommu_map(struct iommu_domain *domain, unsigned long iova,
>>> @@ -1054,22 +1057,29 @@ static inline int iommu_sva_get_pasid(struct
>> iommu_sva *handle)
>>> return IOMMU_PASID_INVALID;
>>> }
>>>
>>> -static inline int iommu_uapi_cache_invalidate(struct iommu_domain *domain,
>>> - struct device *dev,
>>> - struct iommu_cache_invalidate_info
>> *inv_info)
>>> +static inline int
>>> +iommu_uapi_cache_invalidate(struct iommu_domain *domain,
>>> + struct device *dev,
>>> + struct iommu_cache_invalidate_info *inv_info)
>>> {
>>> return -ENODEV;
>>> }
>>>
>>> static inline int iommu_uapi_sva_bind_gpasid(struct iommu_domain *domain,
>>> - struct device *dev,
>>> - struct iommu_gpasid_bind_data *data)
>>> + struct device *dev, void __user *udata)
>>> {
>>> return -ENODEV;
>>> }
>>>
>>> static inline int iommu_uapi_sva_unbind_gpasid(struct iommu_domain *domain,
>>> - struct device *dev, int pasid)
>>> + struct device *dev, void __user *udata)
>>> +{
>>> + return -ENODEV;
>>> +}
>>> +
>>> +static inline int iommu_sva_unbind_gpasid(struct iommu_domain *domain,
>>> + struct device *dev,
>>> + struct iommu_gpasid_bind_data *data)
>>> {
>>> return -ENODEV;
>>> }
>>>
>> Otherwise looks good to me
>> Reviewed-by: Eric Auger <[email protected]>
>>
>> Thanks
>>
>> Eric
>
> From: Auger Eric <[email protected]>
> Sent: Thursday, August 13, 2020 5:31 PM
>
> Hi Yi,
>
> On 8/13/20 11:25 AM, Liu, Yi L wrote:
> > Hi Eric,
> >
> >
> >> From: Auger Eric <[email protected]>
> >> Sent: Thursday, August 13, 2020 5:12 PM
> >>
> >> Hi Jacob,
> >>
> >> On 7/30/20 2:21 AM, Jacob Pan wrote:
> >>> IOMMU user APIs are responsible for processing user data. This patch
> >>> changes the interface such that user pointers can be passed into
> >>> IOMMU code directly. Separate kernel APIs without user pointers are
> >>> introduced for in-kernel users of the UAPI functionality.
> >> This is just done for a single function, ie. iommu_sva_unbind_gpasid.
> >>
> >> If I am not wrong there is no user of this latter after applying the
> >> whole series? If correct you may remove it at this stage?
> >
> > the user of this function is in vfio. And it is the same with
> > iommu_uapi_sva_bind/unbind_gpasid() and iommu_uapi_cache_invalidate().
> >
> > https://lore.kernel.org/kvm/1595917664-33276-11-git-send-email-yi.l.li
> > [email protected]/
> > https://lore.kernel.org/kvm/1595917664-33276-12-git-send-email-yi.l.li
> > [email protected]/
> Yep I know ;-) But this series mostly deals with iommu uapi rework.
> That's not a big deal though.
I see. btw. it's great if you can take a look on vfio v6 to see if your comments
are well addressed. :-)
Regards,
Yi Liu
> Thanks
>
> Eric
> >
> > Regards,
> > Yi Liu
> >
> >>>
> >>> IOMMU UAPI data has a user filled argsz field which indicates the
> >>> data length of the structure. User data is not trusted, argsz must
> >>> be validated based on the current kernel data size, mandatory data
> >>> size, and feature flags.
> >>>
> >>> User data may also be extended, resulting in possible argsz increase.
> >>> Backward compatibility is ensured based on size and flags (or the
> >>> functional equivalent fields) checking.
> >>>
> >>> This patch adds sanity checks in the IOMMU layer. In addition to
> >>> argsz, reserved/unused fields in padding, flags, and version are also checked.
> >>> Details are documented in Documentation/userspace-api/iommu.rst
> >>>
> >>> Signed-off-by: Liu Yi L <[email protected]>
> >>> Signed-off-by: Jacob Pan <[email protected]>
> >>> ---
> >>> drivers/iommu/iommu.c | 201
> >> ++++++++++++++++++++++++++++++++++++++++++++++++--
> >>> include/linux/iommu.h | 28 ++++---
> >>> 2 files changed, 212 insertions(+), 17 deletions(-)
> >>>
> >>> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c index
> >>> 3a913ce94a3d..1ee55c4b3a3a 100644
> >>> --- a/drivers/iommu/iommu.c
> >>> +++ b/drivers/iommu/iommu.c
> >>> @@ -1950,33 +1950,218 @@ int iommu_attach_device(struct iommu_domain
> >> *domain, struct device *dev)
> >>> }
> >>> EXPORT_SYMBOL_GPL(iommu_attach_device);
> >>>
> >>> +/*
> >>> + * Check flags and other user provided data for valid combinations.
> >>> +We also
> >>> + * make sure no reserved fields or unused flags are set. This is to
> >>> +ensure
> >>> + * not breaking userspace in the future when these fields or flags are used.
> >>> + */
> >>> +static int iommu_check_cache_invl_data(struct
> >>> +iommu_cache_invalidate_info
> >> *info)
> >>> +{
> >>> + u32 mask;
> >>> + int i;
> >>> +
> >>> + if (info->version != IOMMU_CACHE_INVALIDATE_INFO_VERSION_1)
> >>> + return -EINVAL;
> >>> +
> >>> + mask = (1 << IOMMU_CACHE_INV_TYPE_NR) - 1;
> >>> + if (info->cache & ~mask)
> >>> + return -EINVAL;
> >>> +
> >>> + if (info->granularity >= IOMMU_INV_GRANU_NR)
> >>> + return -EINVAL;
> >>> +
> >>> + switch (info->granularity) {
> >>> + case IOMMU_INV_GRANU_ADDR:
> >>> + if (info->cache & IOMMU_CACHE_INV_TYPE_PASID)
> >>> + return -EINVAL;
> >>> +
> >>> + mask = IOMMU_INV_ADDR_FLAGS_PASID |
> >>> + IOMMU_INV_ADDR_FLAGS_ARCHID |
> >>> + IOMMU_INV_ADDR_FLAGS_LEAF;
> >>> +
> >>> + if (info->granu.addr_info.flags & ~mask)
> >>> + return -EINVAL;
> >>> + break;
> >>> + case IOMMU_INV_GRANU_PASID:
> >>> + mask = IOMMU_INV_PASID_FLAGS_PASID |
> >>> + IOMMU_INV_PASID_FLAGS_ARCHID;
> >>> + if (info->granu.pasid_info.flags & ~mask)
> >>> + return -EINVAL;
> >>> +
> >>> + break;
> >>> + case IOMMU_INV_GRANU_DOMAIN:
> >>> + if (info->cache & IOMMU_CACHE_INV_TYPE_DEV_IOTLB)
> >>> + return -EINVAL;
> >>> + break;
> >>> + default:
> >>> + return -EINVAL;
> >>> + }
> >>> +
> >>> + /* Check reserved padding fields */
> >>> + for (i = 0; i < sizeof(info->padding); i++) {
> >>> + if (info->padding[i])
> >>> + return -EINVAL;
> >>> + }
> >>> +
> >>> + return 0;
> >>> +}
> >>> +
> >>> int iommu_uapi_cache_invalidate(struct iommu_domain *domain, struct
> >>> device
> >> *dev,
> >>> - struct iommu_cache_invalidate_info *inv_info)
> >>> + void __user *uinfo)
> >>> {
> >>> + struct iommu_cache_invalidate_info inv_info = { 0 };
> >>> + u32 minsz;
> >>> + int ret = 0;
> >>> +
> >>> if (unlikely(!domain->ops->cache_invalidate))
> >>> return -ENODEV;
> >>> > - return domain->ops->cache_invalidate(domain, dev, inv_info);
> >>> + /*
> >>> + * No new spaces can be added before the variable sized union, the
> >>> + * minimum size is the offset to the union.
> >>> + */
> >>> + minsz = offsetof(struct iommu_cache_invalidate_info, granu);
> >>> +
> >>> + /* Copy minsz from user to get flags and argsz */
> >>> + if (copy_from_user(&inv_info, uinfo, minsz))
> >>> + return -EFAULT;
> >>> +
> >>> + /* Fields before variable size union is mandatory */
> >>> + if (inv_info.argsz < minsz)
> >>> + return -EINVAL;
> >>> +
> >>> + /* PASID and address granu require additional info beyond minsz */
> >>> + if (inv_info.argsz == minsz &&
> >>> + ((inv_info.granularity == IOMMU_INV_GRANU_PASID) ||
> >>> + (inv_info.granularity == IOMMU_INV_GRANU_ADDR)))
> >>> + return -EINVAL;
> >>> +
> >>> + if (inv_info.granularity == IOMMU_INV_GRANU_PASID &&
> >>> + inv_info.argsz < offsetofend(struct
> >>> +iommu_cache_invalidate_info,
> >> granu.pasid_info))
> >>> + return -EINVAL;
> >>> +
> >>> + if (inv_info.granularity == IOMMU_INV_GRANU_ADDR &&
> >>> + inv_info.argsz < offsetofend(struct
> >>> +iommu_cache_invalidate_info,
> >> granu.addr_info))
> >>> + return -EINVAL;
> >>> +
> >>> + /*
> >>> + * User might be using a newer UAPI header which has a larger data
> >>> + * size, we shall support the existing flags within the current
> >>> + * size. Copy the remaining user data _after_ minsz but not more
> >>> + * than the current kernel supported size.
> >>> + */
> >>> + if (copy_from_user((void *)&inv_info + minsz, uinfo + minsz,
> >>> + min_t(u32, inv_info.argsz, sizeof(inv_info)) - minsz))
> >>> + return -EFAULT;
> >>> +
> >>> + /* Now the argsz is validated, check the content */
> >>> + ret = iommu_check_cache_invl_data(&inv_info);
> >>> + if (ret)
> >>> + return ret;
> >>> +
> >>> + return domain->ops->cache_invalidate(domain, dev, &inv_info);
> >>> }
> >>> EXPORT_SYMBOL_GPL(iommu_uapi_cache_invalidate);
> >>>
> >>> -int iommu_uapi_sva_bind_gpasid(struct iommu_domain *domain,
> >>> - struct device *dev, struct iommu_gpasid_bind_data
> >> *data)
> >>> +static int iommu_check_bind_data(struct iommu_gpasid_bind_data
> >>> +*data) {
> >>> + u32 mask;
> >>> + int i;
> >>> +
> >>> + if (data->version != IOMMU_GPASID_BIND_VERSION_1)
> >>> + return -EINVAL;
> >>> +
> >>> + /* Check the range of supported formats */
> >>> + if (data->format >= IOMMU_PASID_FORMAT_LAST)
> >>> + return -EINVAL;
> >>> +
> >>> + /* Check all flags */
> >>> + mask = IOMMU_SVA_GPASID_VAL;
> >>> + if (data->flags & ~mask)
> >>> + return -EINVAL;
> >>> +
> >>> + /* Check reserved padding fields */
> >>> + for (i = 0; i < sizeof(data->padding); i++) {
> >>> + if (data->padding[i])
> >>> + return -EINVAL;
> >>> + }
> >>> +
> >>> + return 0;
> >>> +}
> >>> +
> >>> +static int iommu_sva_prepare_bind_data(void __user *udata,
> >>> + struct iommu_gpasid_bind_data *data)
> >>> {
> >>> + u32 minsz;
> >>> +
> >>> + /*
> >>> + * No new spaces can be added before the variable sized union, the
> >>> + * minimum size is the offset to the union.
> >>> + */
> >>> + minsz = offsetof(struct iommu_gpasid_bind_data, vendor);
> >>> +
> >>> + /* Copy minsz from user to get flags and argsz */
> >>> + if (copy_from_user(data, udata, minsz))
> >>> + return -EFAULT;
> >>> +
> >>> + /* Fields before variable size union is mandatory */
> >>> + if (data->argsz < minsz)
> >>> + return -EINVAL;
> >>> + /*
> >>> + * User might be using a newer UAPI header, we shall let IOMMU vendor
> >>> + * driver decide on what size it needs. Since the guest PASID bind data
> >>> + * can be vendor specific, larger argsz could be the result of extension
> >>> + * for one vendor but it should not affect another vendor.
> >>> + * Copy the remaining user data _after_ minsz
> >>> + */
> >>> + if (copy_from_user((void *)data + minsz, udata + minsz,
> >>> + min_t(u32, data->argsz, sizeof(*data)) - minsz))
> >>> + return -EFAULT;
> >>> +
> >>> + return iommu_check_bind_data(data); }
> >>> +
> >>> +int iommu_uapi_sva_bind_gpasid(struct iommu_domain *domain, struct
> >>> +device
> >> *dev,
> >>> + void __user *udata)
> >>> +{
> >>> + struct iommu_gpasid_bind_data data = { 0 };
> >>> + int ret;
> >>> +
> >>> if (unlikely(!domain->ops->sva_bind_gpasid))
> >>> return -ENODEV;
> >>>
> >>> - return domain->ops->sva_bind_gpasid(domain, dev, data);
> >>> + ret = iommu_sva_prepare_bind_data(udata, &data);
> >>> + if (ret)
> >>> + return ret;
> >>> +
> >>> + return domain->ops->sva_bind_gpasid(domain, dev, &data);
> >>> }
> >>> EXPORT_SYMBOL_GPL(iommu_uapi_sva_bind_gpasid);
> >>>
> >>> -int iommu_uapi_sva_unbind_gpasid(struct iommu_domain *domain,
> >>> struct device
> >> *dev,
> >>> - ioasid_t pasid)
> >>> +int iommu_sva_unbind_gpasid(struct iommu_domain *domain, struct device
> *dev,
> >>> + struct iommu_gpasid_bind_data *data)
> >>> {
> >>> if (unlikely(!domain->ops->sva_unbind_gpasid))
> >>> return -ENODEV;
> >>>
> >>> - return domain->ops->sva_unbind_gpasid(dev, pasid);
> >>> + return domain->ops->sva_unbind_gpasid(dev, data->hpasid); }
> >>> +EXPORT_SYMBOL_GPL(iommu_sva_unbind_gpasid);
> >>> +
> >>> +int iommu_uapi_sva_unbind_gpasid(struct iommu_domain *domain,
> >>> +struct device
> >> *dev,
> >>> + void __user *udata)
> >>> +{
> >>> + struct iommu_gpasid_bind_data data = { 0 };
> >>> + int ret;
> >>> +
> >>> + if (unlikely(!domain->ops->sva_bind_gpasid))
> >>> + return -ENODEV;
> >>> +
> >>> + ret = iommu_sva_prepare_bind_data(udata, &data);
> >>> + if (ret)
> >>> + return ret;
> >>> +
> >>> + return iommu_sva_unbind_gpasid(domain, dev, &data);
> >>> }
> >>> EXPORT_SYMBOL_GPL(iommu_uapi_sva_unbind_gpasid);
> >>>
> >>> diff --git a/include/linux/iommu.h b/include/linux/iommu.h index
> >>> 2dcc1a33f6dc..4a02c9e09048 100644
> >>> --- a/include/linux/iommu.h
> >>> +++ b/include/linux/iommu.h
> >>> @@ -432,11 +432,14 @@ extern void iommu_detach_device(struct
> >> iommu_domain *domain,
> >>> struct device *dev);
> >>> extern int iommu_uapi_cache_invalidate(struct iommu_domain *domain,
> >>> struct device *dev,
> >>> - struct iommu_cache_invalidate_info *inv_info);
> >>> + void __user *uinfo);
> >>> +
> >>> extern int iommu_uapi_sva_bind_gpasid(struct iommu_domain *domain,
> >>> - struct device *dev, struct
> >> iommu_gpasid_bind_data *data);
> >>> + struct device *dev, void __user *udata);
> >>> extern int iommu_uapi_sva_unbind_gpasid(struct iommu_domain *domain,
> >>> - struct device *dev, ioasid_t pasid);
> >>> + struct device *dev, void __user *udata);
> extern int
> >>> +iommu_sva_unbind_gpasid(struct iommu_domain *domain,
> >>> + struct device *dev, struct
> >> iommu_gpasid_bind_data *data);
> >>> extern struct iommu_domain *iommu_get_domain_for_dev(struct device
> >>> *dev); extern struct iommu_domain *iommu_get_dma_domain(struct
> >>> device *dev); extern int iommu_map(struct iommu_domain *domain,
> >>> unsigned long iova, @@ -1054,22 +1057,29 @@ static inline int
> >>> iommu_sva_get_pasid(struct
> >> iommu_sva *handle)
> >>> return IOMMU_PASID_INVALID;
> >>> }
> >>>
> >>> -static inline int iommu_uapi_cache_invalidate(struct iommu_domain *domain,
> >>> - struct device *dev,
> >>> - struct iommu_cache_invalidate_info
> >> *inv_info)
> >>> +static inline int
> >>> +iommu_uapi_cache_invalidate(struct iommu_domain *domain,
> >>> + struct device *dev,
> >>> + struct iommu_cache_invalidate_info *inv_info)
> >>> {
> >>> return -ENODEV;
> >>> }
> >>>
> >>> static inline int iommu_uapi_sva_bind_gpasid(struct iommu_domain *domain,
> >>> - struct device *dev,
> >>> - struct iommu_gpasid_bind_data *data)
> >>> + struct device *dev, void __user *udata)
> >>> {
> >>> return -ENODEV;
> >>> }
> >>>
> >>> static inline int iommu_uapi_sva_unbind_gpasid(struct iommu_domain *domain,
> >>> - struct device *dev, int pasid)
> >>> + struct device *dev, void __user *udata)
> {
> >>> + return -ENODEV;
> >>> +}
> >>> +
> >>> +static inline int iommu_sva_unbind_gpasid(struct iommu_domain *domain,
> >>> + struct device *dev,
> >>> + struct iommu_gpasid_bind_data *data)
> >>> {
> >>> return -ENODEV;
> >>> }
> >>>
> >> Otherwise looks good to me
> >> Reviewed-by: Eric Auger <[email protected]>
> >>
> >> Thanks
> >>
> >> Eric
> >
Hi Yi,
On 8/13/20 11:38 AM, Liu, Yi L wrote:
>> From: Auger Eric <[email protected]>
>> Sent: Thursday, August 13, 2020 5:31 PM
>>
>> Hi Yi,
>>
>> On 8/13/20 11:25 AM, Liu, Yi L wrote:
>>> Hi Eric,
>>>
>>>
>>>> From: Auger Eric <[email protected]>
>>>> Sent: Thursday, August 13, 2020 5:12 PM
>>>>
>>>> Hi Jacob,
>>>>
>>>> On 7/30/20 2:21 AM, Jacob Pan wrote:
>>>>> IOMMU user APIs are responsible for processing user data. This patch
>>>>> changes the interface such that user pointers can be passed into
>>>>> IOMMU code directly. Separate kernel APIs without user pointers are
>>>>> introduced for in-kernel users of the UAPI functionality.
>>>> This is just done for a single function, ie. iommu_sva_unbind_gpasid.
>>>>
>>>> If I am not wrong there is no user of this latter after applying the
>>>> whole series? If correct you may remove it at this stage?
>>>
>>> the user of this function is in vfio. And it is the same with
>>> iommu_uapi_sva_bind/unbind_gpasid() and iommu_uapi_cache_invalidate().
>>>
>>> https://lore.kernel.org/kvm/1595917664-33276-11-git-send-email-yi.l.li
>>> [email protected]/
>>> https://lore.kernel.org/kvm/1595917664-33276-12-git-send-email-yi.l.li
>>> [email protected]/
>> Yep I know ;-) But this series mostly deals with iommu uapi rework.
>> That's not a big deal though.
>
> I see. btw. it's great if you can take a look on vfio v6 to see if your comments
> are well addressed. :-)
Yep I will do asap
Thanks
Eric
>
> Regards,
> Yi Liu
>
>> Thanks
>>
>> Eric
>>>
>>> Regards,
>>> Yi Liu
>>>
>>>>>
>>>>> IOMMU UAPI data has a user filled argsz field which indicates the
>>>>> data length of the structure. User data is not trusted, argsz must
>>>>> be validated based on the current kernel data size, mandatory data
>>>>> size, and feature flags.
>>>>>
>>>>> User data may also be extended, resulting in possible argsz increase.
>>>>> Backward compatibility is ensured based on size and flags (or the
>>>>> functional equivalent fields) checking.
>>>>>
>>>>> This patch adds sanity checks in the IOMMU layer. In addition to
>>>>> argsz, reserved/unused fields in padding, flags, and version are also checked.
>>>>> Details are documented in Documentation/userspace-api/iommu.rst
>>>>>
>>>>> Signed-off-by: Liu Yi L <[email protected]>
>>>>> Signed-off-by: Jacob Pan <[email protected]>
>>>>> ---
>>>>> drivers/iommu/iommu.c | 201
>>>> ++++++++++++++++++++++++++++++++++++++++++++++++--
>>>>> include/linux/iommu.h | 28 ++++---
>>>>> 2 files changed, 212 insertions(+), 17 deletions(-)
>>>>>
>>>>> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c index
>>>>> 3a913ce94a3d..1ee55c4b3a3a 100644
>>>>> --- a/drivers/iommu/iommu.c
>>>>> +++ b/drivers/iommu/iommu.c
>>>>> @@ -1950,33 +1950,218 @@ int iommu_attach_device(struct iommu_domain
>>>> *domain, struct device *dev)
>>>>> }
>>>>> EXPORT_SYMBOL_GPL(iommu_attach_device);
>>>>>
>>>>> +/*
>>>>> + * Check flags and other user provided data for valid combinations.
>>>>> +We also
>>>>> + * make sure no reserved fields or unused flags are set. This is to
>>>>> +ensure
>>>>> + * not breaking userspace in the future when these fields or flags are used.
>>>>> + */
>>>>> +static int iommu_check_cache_invl_data(struct
>>>>> +iommu_cache_invalidate_info
>>>> *info)
>>>>> +{
>>>>> + u32 mask;
>>>>> + int i;
>>>>> +
>>>>> + if (info->version != IOMMU_CACHE_INVALIDATE_INFO_VERSION_1)
>>>>> + return -EINVAL;
>>>>> +
>>>>> + mask = (1 << IOMMU_CACHE_INV_TYPE_NR) - 1;
>>>>> + if (info->cache & ~mask)
>>>>> + return -EINVAL;
>>>>> +
>>>>> + if (info->granularity >= IOMMU_INV_GRANU_NR)
>>>>> + return -EINVAL;
>>>>> +
>>>>> + switch (info->granularity) {
>>>>> + case IOMMU_INV_GRANU_ADDR:
>>>>> + if (info->cache & IOMMU_CACHE_INV_TYPE_PASID)
>>>>> + return -EINVAL;
>>>>> +
>>>>> + mask = IOMMU_INV_ADDR_FLAGS_PASID |
>>>>> + IOMMU_INV_ADDR_FLAGS_ARCHID |
>>>>> + IOMMU_INV_ADDR_FLAGS_LEAF;
>>>>> +
>>>>> + if (info->granu.addr_info.flags & ~mask)
>>>>> + return -EINVAL;
>>>>> + break;
>>>>> + case IOMMU_INV_GRANU_PASID:
>>>>> + mask = IOMMU_INV_PASID_FLAGS_PASID |
>>>>> + IOMMU_INV_PASID_FLAGS_ARCHID;
>>>>> + if (info->granu.pasid_info.flags & ~mask)
>>>>> + return -EINVAL;
>>>>> +
>>>>> + break;
>>>>> + case IOMMU_INV_GRANU_DOMAIN:
>>>>> + if (info->cache & IOMMU_CACHE_INV_TYPE_DEV_IOTLB)
>>>>> + return -EINVAL;
>>>>> + break;
>>>>> + default:
>>>>> + return -EINVAL;
>>>>> + }
>>>>> +
>>>>> + /* Check reserved padding fields */
>>>>> + for (i = 0; i < sizeof(info->padding); i++) {
>>>>> + if (info->padding[i])
>>>>> + return -EINVAL;
>>>>> + }
>>>>> +
>>>>> + return 0;
>>>>> +}
>>>>> +
>>>>> int iommu_uapi_cache_invalidate(struct iommu_domain *domain, struct
>>>>> device
>>>> *dev,
>>>>> - struct iommu_cache_invalidate_info *inv_info)
>>>>> + void __user *uinfo)
>>>>> {
>>>>> + struct iommu_cache_invalidate_info inv_info = { 0 };
>>>>> + u32 minsz;
>>>>> + int ret = 0;
>>>>> +
>>>>> if (unlikely(!domain->ops->cache_invalidate))
>>>>> return -ENODEV;
>>>>> > - return domain->ops->cache_invalidate(domain, dev, inv_info);
>>>>> + /*
>>>>> + * No new spaces can be added before the variable sized union, the
>>>>> + * minimum size is the offset to the union.
>>>>> + */
>>>>> + minsz = offsetof(struct iommu_cache_invalidate_info, granu);
>>>>> +
>>>>> + /* Copy minsz from user to get flags and argsz */
>>>>> + if (copy_from_user(&inv_info, uinfo, minsz))
>>>>> + return -EFAULT;
>>>>> +
>>>>> + /* Fields before variable size union is mandatory */
>>>>> + if (inv_info.argsz < minsz)
>>>>> + return -EINVAL;
>>>>> +
>>>>> + /* PASID and address granu require additional info beyond minsz */
>>>>> + if (inv_info.argsz == minsz &&
>>>>> + ((inv_info.granularity == IOMMU_INV_GRANU_PASID) ||
>>>>> + (inv_info.granularity == IOMMU_INV_GRANU_ADDR)))
>>>>> + return -EINVAL;
>>>>> +
>>>>> + if (inv_info.granularity == IOMMU_INV_GRANU_PASID &&
>>>>> + inv_info.argsz < offsetofend(struct
>>>>> +iommu_cache_invalidate_info,
>>>> granu.pasid_info))
>>>>> + return -EINVAL;
>>>>> +
>>>>> + if (inv_info.granularity == IOMMU_INV_GRANU_ADDR &&
>>>>> + inv_info.argsz < offsetofend(struct
>>>>> +iommu_cache_invalidate_info,
>>>> granu.addr_info))
>>>>> + return -EINVAL;
>>>>> +
>>>>> + /*
>>>>> + * User might be using a newer UAPI header which has a larger data
>>>>> + * size, we shall support the existing flags within the current
>>>>> + * size. Copy the remaining user data _after_ minsz but not more
>>>>> + * than the current kernel supported size.
>>>>> + */
>>>>> + if (copy_from_user((void *)&inv_info + minsz, uinfo + minsz,
>>>>> + min_t(u32, inv_info.argsz, sizeof(inv_info)) - minsz))
>>>>> + return -EFAULT;
>>>>> +
>>>>> + /* Now the argsz is validated, check the content */
>>>>> + ret = iommu_check_cache_invl_data(&inv_info);
>>>>> + if (ret)
>>>>> + return ret;
>>>>> +
>>>>> + return domain->ops->cache_invalidate(domain, dev, &inv_info);
>>>>> }
>>>>> EXPORT_SYMBOL_GPL(iommu_uapi_cache_invalidate);
>>>>>
>>>>> -int iommu_uapi_sva_bind_gpasid(struct iommu_domain *domain,
>>>>> - struct device *dev, struct iommu_gpasid_bind_data
>>>> *data)
>>>>> +static int iommu_check_bind_data(struct iommu_gpasid_bind_data
>>>>> +*data) {
>>>>> + u32 mask;
>>>>> + int i;
>>>>> +
>>>>> + if (data->version != IOMMU_GPASID_BIND_VERSION_1)
>>>>> + return -EINVAL;
>>>>> +
>>>>> + /* Check the range of supported formats */
>>>>> + if (data->format >= IOMMU_PASID_FORMAT_LAST)
>>>>> + return -EINVAL;
>>>>> +
>>>>> + /* Check all flags */
>>>>> + mask = IOMMU_SVA_GPASID_VAL;
>>>>> + if (data->flags & ~mask)
>>>>> + return -EINVAL;
>>>>> +
>>>>> + /* Check reserved padding fields */
>>>>> + for (i = 0; i < sizeof(data->padding); i++) {
>>>>> + if (data->padding[i])
>>>>> + return -EINVAL;
>>>>> + }
>>>>> +
>>>>> + return 0;
>>>>> +}
>>>>> +
>>>>> +static int iommu_sva_prepare_bind_data(void __user *udata,
>>>>> + struct iommu_gpasid_bind_data *data)
>>>>> {
>>>>> + u32 minsz;
>>>>> +
>>>>> + /*
>>>>> + * No new spaces can be added before the variable sized union, the
>>>>> + * minimum size is the offset to the union.
>>>>> + */
>>>>> + minsz = offsetof(struct iommu_gpasid_bind_data, vendor);
>>>>> +
>>>>> + /* Copy minsz from user to get flags and argsz */
>>>>> + if (copy_from_user(data, udata, minsz))
>>>>> + return -EFAULT;
>>>>> +
>>>>> + /* Fields before variable size union is mandatory */
>>>>> + if (data->argsz < minsz)
>>>>> + return -EINVAL;
>>>>> + /*
>>>>> + * User might be using a newer UAPI header, we shall let IOMMU vendor
>>>>> + * driver decide on what size it needs. Since the guest PASID bind data
>>>>> + * can be vendor specific, larger argsz could be the result of extension
>>>>> + * for one vendor but it should not affect another vendor.
>>>>> + * Copy the remaining user data _after_ minsz
>>>>> + */
>>>>> + if (copy_from_user((void *)data + minsz, udata + minsz,
>>>>> + min_t(u32, data->argsz, sizeof(*data)) - minsz))
>>>>> + return -EFAULT;
>>>>> +
>>>>> + return iommu_check_bind_data(data); }
>>>>> +
>>>>> +int iommu_uapi_sva_bind_gpasid(struct iommu_domain *domain, struct
>>>>> +device
>>>> *dev,
>>>>> + void __user *udata)
>>>>> +{
>>>>> + struct iommu_gpasid_bind_data data = { 0 };
>>>>> + int ret;
>>>>> +
>>>>> if (unlikely(!domain->ops->sva_bind_gpasid))
>>>>> return -ENODEV;
>>>>>
>>>>> - return domain->ops->sva_bind_gpasid(domain, dev, data);
>>>>> + ret = iommu_sva_prepare_bind_data(udata, &data);
>>>>> + if (ret)
>>>>> + return ret;
>>>>> +
>>>>> + return domain->ops->sva_bind_gpasid(domain, dev, &data);
>>>>> }
>>>>> EXPORT_SYMBOL_GPL(iommu_uapi_sva_bind_gpasid);
>>>>>
>>>>> -int iommu_uapi_sva_unbind_gpasid(struct iommu_domain *domain,
>>>>> struct device
>>>> *dev,
>>>>> - ioasid_t pasid)
>>>>> +int iommu_sva_unbind_gpasid(struct iommu_domain *domain, struct device
>> *dev,
>>>>> + struct iommu_gpasid_bind_data *data)
>>>>> {
>>>>> if (unlikely(!domain->ops->sva_unbind_gpasid))
>>>>> return -ENODEV;
>>>>>
>>>>> - return domain->ops->sva_unbind_gpasid(dev, pasid);
>>>>> + return domain->ops->sva_unbind_gpasid(dev, data->hpasid); }
>>>>> +EXPORT_SYMBOL_GPL(iommu_sva_unbind_gpasid);
>>>>> +
>>>>> +int iommu_uapi_sva_unbind_gpasid(struct iommu_domain *domain,
>>>>> +struct device
>>>> *dev,
>>>>> + void __user *udata)
>>>>> +{
>>>>> + struct iommu_gpasid_bind_data data = { 0 };
>>>>> + int ret;
>>>>> +
>>>>> + if (unlikely(!domain->ops->sva_bind_gpasid))
>>>>> + return -ENODEV;
>>>>> +
>>>>> + ret = iommu_sva_prepare_bind_data(udata, &data);
>>>>> + if (ret)
>>>>> + return ret;
>>>>> +
>>>>> + return iommu_sva_unbind_gpasid(domain, dev, &data);
>>>>> }
>>>>> EXPORT_SYMBOL_GPL(iommu_uapi_sva_unbind_gpasid);
>>>>>
>>>>> diff --git a/include/linux/iommu.h b/include/linux/iommu.h index
>>>>> 2dcc1a33f6dc..4a02c9e09048 100644
>>>>> --- a/include/linux/iommu.h
>>>>> +++ b/include/linux/iommu.h
>>>>> @@ -432,11 +432,14 @@ extern void iommu_detach_device(struct
>>>> iommu_domain *domain,
>>>>> struct device *dev);
>>>>> extern int iommu_uapi_cache_invalidate(struct iommu_domain *domain,
>>>>> struct device *dev,
>>>>> - struct iommu_cache_invalidate_info *inv_info);
>>>>> + void __user *uinfo);
>>>>> +
>>>>> extern int iommu_uapi_sva_bind_gpasid(struct iommu_domain *domain,
>>>>> - struct device *dev, struct
>>>> iommu_gpasid_bind_data *data);
>>>>> + struct device *dev, void __user *udata);
>>>>> extern int iommu_uapi_sva_unbind_gpasid(struct iommu_domain *domain,
>>>>> - struct device *dev, ioasid_t pasid);
>>>>> + struct device *dev, void __user *udata);
>> extern int
>>>>> +iommu_sva_unbind_gpasid(struct iommu_domain *domain,
>>>>> + struct device *dev, struct
>>>> iommu_gpasid_bind_data *data);
>>>>> extern struct iommu_domain *iommu_get_domain_for_dev(struct device
>>>>> *dev); extern struct iommu_domain *iommu_get_dma_domain(struct
>>>>> device *dev); extern int iommu_map(struct iommu_domain *domain,
>>>>> unsigned long iova, @@ -1054,22 +1057,29 @@ static inline int
>>>>> iommu_sva_get_pasid(struct
>>>> iommu_sva *handle)
>>>>> return IOMMU_PASID_INVALID;
>>>>> }
>>>>>
>>>>> -static inline int iommu_uapi_cache_invalidate(struct iommu_domain *domain,
>>>>> - struct device *dev,
>>>>> - struct iommu_cache_invalidate_info
>>>> *inv_info)
>>>>> +static inline int
>>>>> +iommu_uapi_cache_invalidate(struct iommu_domain *domain,
>>>>> + struct device *dev,
>>>>> + struct iommu_cache_invalidate_info *inv_info)
>>>>> {
>>>>> return -ENODEV;
>>>>> }
>>>>>
>>>>> static inline int iommu_uapi_sva_bind_gpasid(struct iommu_domain *domain,
>>>>> - struct device *dev,
>>>>> - struct iommu_gpasid_bind_data *data)
>>>>> + struct device *dev, void __user *udata)
>>>>> {
>>>>> return -ENODEV;
>>>>> }
>>>>>
>>>>> static inline int iommu_uapi_sva_unbind_gpasid(struct iommu_domain *domain,
>>>>> - struct device *dev, int pasid)
>>>>> + struct device *dev, void __user *udata)
>> {
>>>>> + return -ENODEV;
>>>>> +}
>>>>> +
>>>>> +static inline int iommu_sva_unbind_gpasid(struct iommu_domain *domain,
>>>>> + struct device *dev,
>>>>> + struct iommu_gpasid_bind_data *data)
>>>>> {
>>>>> return -ENODEV;
>>>>> }
>>>>>
>>>> Otherwise looks good to me
>>>> Reviewed-by: Eric Auger <[email protected]>
>>>>
>>>> Thanks
>>>>
>>>> Eric
>>>
>