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.
Thanks,
Jacob
Changeog:
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 (5):
docs: IOMMU user API
iommu/uapi: Add argsz for user filled data
iommu/uapi: Use named union for user data
iommu/uapi: Handle data and argsz filled by users
iommu/vt-d: Check UAPI data processed by IOMMU core
Documentation/userspace-api/iommu.rst | 338 ++++++++++++++++++++++++++++++++++
drivers/iommu/intel/iommu.c | 27 ++-
drivers/iommu/intel/svm.c | 9 +-
drivers/iommu/iommu.c | 192 ++++++++++++++++++-
include/linux/iommu.h | 20 +-
include/uapi/linux/iommu.h | 16 +-
6 files changed, 568 insertions(+), 34 deletions(-)
create mode 100644 Documentation/userspace-api/iommu.rst
--
2.7.4
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 f3a6ca88cf95..5e80484f0537 100644
--- a/drivers/iommu/intel/iommu.c
+++ b/drivers/iommu/intel/iommu.c
@@ -5383,8 +5383,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
As IOMMU UAPI gets extended, user data size may increase. To support
backward compatibiliy, this patch introduces a size field to each UAPI
data structures. It is *always* the responsibility for the user to fill in
the correct size. Padding fields are adjusted to ensure 8 byte alignment.
Specific scenarios for user data handling are documented in:
Documentation/userspace-api/iommu.rst
Signed-off-by: Liu Yi L <[email protected]>
Signed-off-by: Jacob Pan <[email protected]>
---
include/uapi/linux/iommu.h | 12 +++++++++---
1 file changed, 9 insertions(+), 3 deletions(-)
diff --git a/include/uapi/linux/iommu.h b/include/uapi/linux/iommu.h
index e907b7091a46..d5e9014f690e 100644
--- a/include/uapi/linux/iommu.h
+++ b/include/uapi/linux/iommu.h
@@ -135,6 +135,7 @@ enum iommu_page_response_code {
/**
* struct iommu_page_response - Generic page response information
+ * @argsz: User filled size of this data
* @version: API version of this structure
* @flags: encodes whether the corresponding fields are valid
* (IOMMU_FAULT_PAGE_RESPONSE_* values)
@@ -143,6 +144,7 @@ enum iommu_page_response_code {
* @code: response code from &enum iommu_page_response_code
*/
struct iommu_page_response {
+ __u32 argsz;
#define IOMMU_PAGE_RESP_VERSION_1 1
__u32 version;
#define IOMMU_PAGE_RESP_PASID_VALID (1 << 0)
@@ -218,6 +220,7 @@ struct iommu_inv_pasid_info {
/**
* struct iommu_cache_invalidate_info - First level/stage invalidation
* information
+ * @argsz: User filled size of this data
* @version: API version of this structure
* @cache: bitfield that allows to select which caches to invalidate
* @granularity: defines the lowest granularity used for the invalidation:
@@ -246,6 +249,7 @@ struct iommu_inv_pasid_info {
* must support the used granularity.
*/
struct iommu_cache_invalidate_info {
+ __u32 argsz;
#define IOMMU_CACHE_INVALIDATE_INFO_VERSION_1 1
__u32 version;
/* IOMMU paging structure cache */
@@ -255,7 +259,7 @@ struct iommu_cache_invalidate_info {
#define IOMMU_CACHE_INV_TYPE_NR (3)
__u8 cache;
__u8 granularity;
- __u8 padding[2];
+ __u8 padding[6];
union {
struct iommu_inv_pasid_info pasid_info;
struct iommu_inv_addr_info addr_info;
@@ -292,6 +296,7 @@ struct iommu_gpasid_bind_data_vtd {
/**
* struct iommu_gpasid_bind_data - Information about device and guest PASID binding
+ * @argsz: User filled size of this data
* @version: Version of this data structure
* @format: PASID table entry format
* @flags: Additional information on guest bind request
@@ -309,17 +314,18 @@ struct iommu_gpasid_bind_data_vtd {
* PASID to host PASID based on this bind data.
*/
struct iommu_gpasid_bind_data {
+ __u32 argsz;
#define IOMMU_GPASID_BIND_VERSION_1 1
__u32 version;
#define IOMMU_PASID_FORMAT_INTEL_VTD 1
__u32 format;
+ __u32 addr_width;
#define IOMMU_SVA_GPASID_VAL (1 << 0) /* guest PASID valid */
__u64 flags;
__u64 gpgd;
__u64 hpasid;
__u64 gpasid;
- __u32 addr_width;
- __u8 padding[12];
+ __u8 padding[8];
/* Vendor specific data */
union {
struct iommu_gpasid_bind_data_vtd vtd;
--
2.7.4
IOMMU UAPI data has a user filled argsz field which indicates the data
length comes with the API call. 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, results in possible argsz increase.
Backward compatibility is ensured based on size and flags 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 | 192 ++++++++++++++++++++++++++++++++++++++++++++++++--
include/linux/iommu.h | 20 ++++--
2 files changed, 200 insertions(+), 12 deletions(-)
diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index d43120eb1dc5..fe30a940d19e 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -1950,36 +1950,214 @@ int iommu_attach_device(struct iommu_domain *domain, struct device *dev)
}
EXPORT_SYMBOL_GPL(iommu_attach_device);
+/*
+ * Check flags and other user privided data for valid combinations. We also
+ * make sure no reserved fields or unused flags are not 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;
+
+ 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:
+ 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:
+ /* No flags to check */
+ break;
+ default:
+ return -EINVAL;
+ }
+
+ if (info->padding[0] || info->padding[1])
+ return -EINVAL;
+
+ return 0;
+}
+
int iommu_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, maxsz;
+ int ret = 0;
+
if (unlikely(!domain->ops->cache_invalidate))
return -ENODEV;
- return domain->ops->cache_invalidate(domain, dev, inv_info);
+ /* Current kernel data size is the max to be copied from user */
+ maxsz = sizeof(struct iommu_cache_invalidate_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 requires 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;
+ /*
+ * 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(inv_info.argsz, maxsz) - 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_cache_invalidate);
-int iommu_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 all supported format, for now just VT-d */
+ mask = IOMMU_PASID_FORMAT_INTEL_VTD;
+ if (data->format & ~mask)
+ return -EINVAL;
+
+ /* Check all flags */
+ mask = IOMMU_SVA_GPASID_VAL;
+ if (data->flags & ~mask)
+ return -EINVAL;
+
+ /* Check reserved padding fields */
+ for (i = 0; i < 12; 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, maxsz;
+
+ /* Current kernel data size is the max to be copied from user */
+ maxsz = sizeof(struct iommu_gpasid_bind_data);
+ memset((void *)data, 0, maxsz);
+
+ /*
+ * 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(data->argsz, maxsz) - minsz))
+ return -EFAULT;
+
+ return iommu_check_bind_data(data);
+}
+
+int iommu_sva_bind_gpasid(struct iommu_domain *domain, struct device *dev,
+ void __user *udata)
+{
+
+ struct iommu_gpasid_bind_data data;
+ 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_sva_bind_gpasid);
int iommu_sva_unbind_gpasid(struct iommu_domain *domain, struct device *dev,
- ioasid_t pasid)
+ 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;
+ 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);
+
static void __iommu_detach_device(struct iommu_domain *domain,
struct device *dev)
{
diff --git a/include/linux/iommu.h b/include/linux/iommu.h
index 5f0b7859d2eb..439f34a7b5b2 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_cache_invalidate(struct iommu_domain *domain,
struct device *dev,
- struct iommu_cache_invalidate_info *inv_info);
+ void __user *uinfo);
+
extern int iommu_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, void __user *udata);
extern int iommu_sva_unbind_gpasid(struct iommu_domain *domain,
- struct device *dev, ioasid_t pasid);
+ 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,
@@ -1062,13 +1065,20 @@ iommu_cache_invalidate(struct iommu_domain *domain,
return -ENODEV;
}
static inline int iommu_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_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
On 7/17/20 2:45 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]>
Reviewed-by: Lu Baolu <[email protected]>
Best regards,
baolu
> ---
> 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 f3a6ca88cf95..5e80484f0537 100644
> --- a/drivers/iommu/intel/iommu.c
> +++ b/drivers/iommu/intel/iommu.c
> @@ -5383,8 +5383,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))
>
Hi Jacob,
On 7/16/20 8:45 PM, Jacob Pan wrote:
> As IOMMU UAPI gets extended, user data size may increase. To support
> backward compatibiliy, this patch introduces a size field to each UAPI
> data structures. It is *always* the responsibility for the user to fill in
> the correct size. Padding fields are adjusted to ensure 8 byte alignment.
>
> Specific scenarios for user data handling are documented in:
> Documentation/userspace-api/iommu.rst
>
> Signed-off-by: Liu Yi L <[email protected]>
> Signed-off-by: Jacob Pan <[email protected]>
> ---
> include/uapi/linux/iommu.h | 12 +++++++++---
> 1 file changed, 9 insertions(+), 3 deletions(-)
>
> diff --git a/include/uapi/linux/iommu.h b/include/uapi/linux/iommu.h
> index e907b7091a46..d5e9014f690e 100644
> --- a/include/uapi/linux/iommu.h
> +++ b/include/uapi/linux/iommu.h
> @@ -135,6 +135,7 @@ enum iommu_page_response_code {
>
> /**
> * struct iommu_page_response - Generic page response information
> + * @argsz: User filled size of this data
> * @version: API version of this structure
> * @flags: encodes whether the corresponding fields are valid
> * (IOMMU_FAULT_PAGE_RESPONSE_* values)
> @@ -143,6 +144,7 @@ enum iommu_page_response_code {
> * @code: response code from &enum iommu_page_response_code
> */
> struct iommu_page_response {
> + __u32 argsz;
> #define IOMMU_PAGE_RESP_VERSION_1 1
Don't you need to incr the version for all the modified structs?
> __u32 version;
> #define IOMMU_PAGE_RESP_PASID_VALID (1 << 0)
> @@ -218,6 +220,7 @@ struct iommu_inv_pasid_info {
> /**
> * struct iommu_cache_invalidate_info - First level/stage invalidation
> * information
> + * @argsz: User filled size of this data
> * @version: API version of this structure
> * @cache: bitfield that allows to select which caches to invalidate
> * @granularity: defines the lowest granularity used for the invalidation:
> @@ -246,6 +249,7 @@ struct iommu_inv_pasid_info {
> * must support the used granularity.
> */
> struct iommu_cache_invalidate_info {
> + __u32 argsz;
> #define IOMMU_CACHE_INVALIDATE_INFO_VERSION_1 1
> __u32 version;
so there is no "flags" field in this struct. Is it OK?
> /* IOMMU paging structure cache */
> @@ -255,7 +259,7 @@ struct iommu_cache_invalidate_info {
> #define IOMMU_CACHE_INV_TYPE_NR (3)
> __u8 cache;
> __u8 granularity;
> - __u8 padding[2];
> + __u8 padding[6];
> union {
> struct iommu_inv_pasid_info pasid_info;
> struct iommu_inv_addr_info addr_info;
> @@ -292,6 +296,7 @@ struct iommu_gpasid_bind_data_vtd {
>
> /**
> * struct iommu_gpasid_bind_data - Information about device and guest PASID binding
> + * @argsz: User filled size of this data
> * @version: Version of this data structure
> * @format: PASID table entry format
> * @flags: Additional information on guest bind request
> @@ -309,17 +314,18 @@ struct iommu_gpasid_bind_data_vtd {
> * PASID to host PASID based on this bind data.
> */
> struct iommu_gpasid_bind_data {
> + __u32 argsz;
> #define IOMMU_GPASID_BIND_VERSION_1 1
> __u32 version;
> #define IOMMU_PASID_FORMAT_INTEL_VTD 1
> __u32 format;
> + __u32 addr_width;
> #define IOMMU_SVA_GPASID_VAL (1 << 0) /* guest PASID valid */
> __u64 flags;
> __u64 gpgd;
> __u64 hpasid;
> __u64 gpasid;
> - __u32 addr_width;
> - __u8 padding[12];
> + __u8 padding[8];
> /* Vendor specific data */
> union {
> struct iommu_gpasid_bind_data_vtd vtd;
>
Thanks
Eric
Hi Jacob,
On 7/16/20 8:45 PM, Jacob Pan wrote:
Could you share a branch? I was not able to apply this on either
iommu/next or master?
> IOMMU UAPI data has a user filled argsz field which indicates the data
> length comes with the API call.
s/ comes with the API call/ 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, results in possible argsz increase.
s/results/resulting
> Backward compatibility is ensured based on size and flags checking.
flags is missing in iommu_cache_invalidate_info.
>
> 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 | 192 ++++++++++++++++++++++++++++++++++++++++++++++++--
> include/linux/iommu.h | 20 ++++--
> 2 files changed, 200 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
> index d43120eb1dc5..fe30a940d19e 100644
> --- a/drivers/iommu/iommu.c
> +++ b/drivers/iommu/iommu.c
> @@ -1950,36 +1950,214 @@ int iommu_attach_device(struct iommu_domain *domain, struct device *dev)
> }
> EXPORT_SYMBOL_GPL(iommu_attach_device);
>
> +/*
> + * Check flags and other user privided data for valid combinations. We also
s/privided/provided
> + * make sure no reserved fields or unused flags are not set. This is to ensure
s/not// ?
> + * 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;
> +
> + if (info->version != IOMMU_CACHE_INVALIDATE_INFO_VERSION_1)
increased version number?
> + 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:
> + 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:
> + /* No flags to check */
> + break;
> + default:
> + return -EINVAL;
> + }
> +
> + if (info->padding[0] || info->padding[1])
padding has become "__u8 padding[6];" in 2/5
> + return -EINVAL;
> +
> + return 0;
> +}
> +
> int iommu_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, maxsz;
> + int ret = 0;
> +
> if (unlikely(!domain->ops->cache_invalidate))
> return -ENODEV;
>
> - return domain->ops->cache_invalidate(domain, dev, inv_info);
> + /* Current kernel data size is the max to be copied from user */
> + maxsz = sizeof(struct iommu_cache_invalidate_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 requires additional info beyond minsz */
s/requires/require
> + if (inv_info.argsz == minsz &&
> + ((inv_info.granularity == IOMMU_INV_GRANU_PASID) ||
> + (inv_info.granularity == IOMMU_INV_GRANU_ADDR)))
> + 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(inv_info.argsz, maxsz) - 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_cache_invalidate);
>
> -int iommu_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 all supported format, for now just VT-d */
> + mask = IOMMU_PASID_FORMAT_INTEL_VTD;
> + if (data->format & ~mask)
> + return -EINVAL;
> +
> + /* Check all flags */
> + mask = IOMMU_SVA_GPASID_VAL;
> + if (data->flags & ~mask)
> + return -EINVAL;
> +
> + /* Check reserved padding fields */
> + for (i = 0; i < 12; i++) {
now 8
> + 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, maxsz;
> +
> + /* Current kernel data size is the max to be copied from user */
> + maxsz = sizeof(struct iommu_gpasid_bind_data);
> + memset((void *)data, 0, maxsz);
> +
> + /*
> + * 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(data->argsz, maxsz) - minsz))
> + return -EFAULT;
> +
> + return iommu_check_bind_data(data);
> +}
> +
> +int iommu_sva_bind_gpasid(struct iommu_domain *domain, struct device *dev,
> + void __user *udata)
change of proto not described in the commit msg
> +{
> +
> + struct iommu_gpasid_bind_data data;
> + 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_sva_bind_gpasid);
>
> int iommu_sva_unbind_gpasid(struct iommu_domain *domain, struct device *dev,
> - ioasid_t pasid)
> + struct iommu_gpasid_bind_data *data)
This change and the introduction of iommu_uapi_sva_unbind_gpasid look
unrelated to this patch, at least not described in the commit msg.
> {
> 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;
> + 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);
> +
> static void __iommu_detach_device(struct iommu_domain *domain,
> struct device *dev)
> {
> diff --git a/include/linux/iommu.h b/include/linux/iommu.h
> index 5f0b7859d2eb..439f34a7b5b2 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_cache_invalidate(struct iommu_domain *domain,
> struct device *dev,
> - struct iommu_cache_invalidate_info *inv_info);
> + void __user *uinfo);
> +
> extern int iommu_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, void __user *udata);
not motivated in the commit msg. I think change of proto and addition of
iommu_sva_unbind_gpasid would deserve a separate patch.
> extern int iommu_sva_unbind_gpasid(struct iommu_domain *domain,
> - struct device *dev, ioasid_t pasid);
> + 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,
> @@ -1062,13 +1065,20 @@ iommu_cache_invalidate(struct iommu_domain *domain,
> return -ENODEV;
> }
> static inline int iommu_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_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;
> }
>
Thanks
Eric
Hi Jacob,
On 7/16/20 8:45 PM, 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]>
Reviewed-by: Eric Auger <[email protected]>
Thanks
Eric
> ---
> 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 f3a6ca88cf95..5e80484f0537 100644
> --- a/drivers/iommu/intel/iommu.c
> +++ b/drivers/iommu/intel/iommu.c
> @@ -5383,8 +5383,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))
>
On Thu, 16 Jul 2020 11:45:16 -0700
Jacob Pan <[email protected]> wrote:
> IOMMU UAPI data has a user filled argsz field which indicates the data
> length comes with the API call. 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, results in possible argsz increase.
> Backward compatibility is ensured based on size and flags 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 | 192 ++++++++++++++++++++++++++++++++++++++++++++++++--
> include/linux/iommu.h | 20 ++++--
> 2 files changed, 200 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
> index d43120eb1dc5..fe30a940d19e 100644
> --- a/drivers/iommu/iommu.c
> +++ b/drivers/iommu/iommu.c
> @@ -1950,36 +1950,214 @@ int iommu_attach_device(struct iommu_domain *domain, struct device *dev)
> }
> EXPORT_SYMBOL_GPL(iommu_attach_device);
>
> +/*
> + * Check flags and other user privided data for valid combinations. We also
> + * make sure no reserved fields or unused flags are not 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;
> +
> + 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:
> + 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:
> + /* No flags to check */
> + break;
> + default:
> + return -EINVAL;
> + }
> +
> + if (info->padding[0] || info->padding[1])
> + return -EINVAL;
> +
> + return 0;
> +}
> +
> int iommu_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, maxsz;
> + int ret = 0;
> +
> if (unlikely(!domain->ops->cache_invalidate))
> return -ENODEV;
>
> - return domain->ops->cache_invalidate(domain, dev, inv_info);
> + /* Current kernel data size is the max to be copied from user */
> + maxsz = sizeof(struct iommu_cache_invalidate_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 requires 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;
> + /*
> + * 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(inv_info.argsz, maxsz) - minsz))
> + return -EFAULT;
To further clarify previous comments about maxsz usage:
s/maxsz/sizeof(inv_info)/
> +
> + /* 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_cache_invalidate);
Remainder of comments for above provided in 1/5.
>
> -int iommu_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 all supported format, for now just VT-d */
> + mask = IOMMU_PASID_FORMAT_INTEL_VTD;
> + if (data->format & ~mask)
> + return -EINVAL;
> +
> + /* Check all flags */
> + mask = IOMMU_SVA_GPASID_VAL;
> + if (data->flags & ~mask)
> + return -EINVAL;
> +
> + /* Check reserved padding fields */
> + for (i = 0; i < 12; 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, maxsz;
> +
> + /* Current kernel data size is the max to be copied from user */
> + maxsz = sizeof(struct iommu_gpasid_bind_data);
> + memset((void *)data, 0, maxsz);
Let the caller pass a zero'd structure.
> +
> + /*
> + * 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(data->argsz, maxsz) - minsz))
We could replace maxsz entirely with sizeof(*data)
> + return -EFAULT;
> +
> + return iommu_check_bind_data(data);
> +}
> +
> +int iommu_sva_bind_gpasid(struct iommu_domain *domain, struct device *dev,
> + void __user *udata)
iommu_uapi_sva_bind_gpasid(...
> +{
> +
> + 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_sva_bind_gpasid);
>
> int iommu_sva_unbind_gpasid(struct iommu_domain *domain, struct device *dev,
> - ioasid_t pasid)
> + 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);
> +
> static void __iommu_detach_device(struct iommu_domain *domain,
> struct device *dev)
> {
> diff --git a/include/linux/iommu.h b/include/linux/iommu.h
> index 5f0b7859d2eb..439f34a7b5b2 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_cache_invalidate(struct iommu_domain *domain,
uapi
> struct device *dev,
> - struct iommu_cache_invalidate_info *inv_info);
> + void __user *uinfo);
> +
> extern int iommu_sva_bind_gpasid(struct iommu_domain *domain,
uapi
> - 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, void __user *udata);
> extern int iommu_sva_unbind_gpasid(struct iommu_domain *domain,
> - struct device *dev, ioasid_t pasid);
> + 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,
> @@ -1062,13 +1065,20 @@ iommu_cache_invalidate(struct iommu_domain *domain,
> return -ENODEV;
> }
> static inline int iommu_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_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,
These don't match the previous semantics. Thanks,
Alex
> + struct device *dev,
> + struct iommu_gpasid_bind_data *data)
> {
> return -ENODEV;
> }
On Fri, 17 Jul 2020 15:44:23 +0200
Auger Eric <[email protected]> wrote:
> Hi Jacob,
>
> On 7/16/20 8:45 PM, Jacob Pan wrote:
> > As IOMMU UAPI gets extended, user data size may increase. To support
> > backward compatibiliy, this patch introduces a size field to each
> > UAPI data structures. It is *always* the responsibility for the
> > user to fill in the correct size. Padding fields are adjusted to
> > ensure 8 byte alignment.
> >
> > Specific scenarios for user data handling are documented in:
> > Documentation/userspace-api/iommu.rst
> >
> > Signed-off-by: Liu Yi L <[email protected]>
> > Signed-off-by: Jacob Pan <[email protected]>
> > ---
> > include/uapi/linux/iommu.h | 12 +++++++++---
> > 1 file changed, 9 insertions(+), 3 deletions(-)
> >
> > diff --git a/include/uapi/linux/iommu.h b/include/uapi/linux/iommu.h
> > index e907b7091a46..d5e9014f690e 100644
> > --- a/include/uapi/linux/iommu.h
> > +++ b/include/uapi/linux/iommu.h
> > @@ -135,6 +135,7 @@ enum iommu_page_response_code {
> >
> > /**
> > * struct iommu_page_response - Generic page response information
> > + * @argsz: User filled size of this data
> > * @version: API version of this structure
> > * @flags: encodes whether the corresponding fields are valid
> > * (IOMMU_FAULT_PAGE_RESPONSE_* values)
> > @@ -143,6 +144,7 @@ enum iommu_page_response_code {
> > * @code: response code from &enum iommu_page_response_code
> > */
> > struct iommu_page_response {
> > + __u32 argsz;
> > #define IOMMU_PAGE_RESP_VERSION_1 1
> Don't you need to incr the version for all the modified structs?
not literal "flags" but @cache and @granularity are flags in reality. I
think that is OK. I also updated document to say "flags or equivalent".
> > __u32 version;
> > #define IOMMU_PAGE_RESP_PASID_VALID (1 << 0)
> > @@ -218,6 +220,7 @@ struct iommu_inv_pasid_info {
> > /**
> > * struct iommu_cache_invalidate_info - First level/stage
> > invalidation
> > * information
> > + * @argsz: User filled size of this data
> > * @version: API version of this structure
> > * @cache: bitfield that allows to select which caches to
> > invalidate
> > * @granularity: defines the lowest granularity used for the
> > invalidation: @@ -246,6 +249,7 @@ struct iommu_inv_pasid_info {
> > * must support the used granularity.
> > */
> > struct iommu_cache_invalidate_info {
> > + __u32 argsz;
> > #define IOMMU_CACHE_INVALIDATE_INFO_VERSION_1 1
> > __u32 version;
> so there is no "flags" field in this struct. Is it OK?
> > /* IOMMU paging structure cache */
> > @@ -255,7 +259,7 @@ struct iommu_cache_invalidate_info {
> > #define IOMMU_CACHE_INV_TYPE_NR (3)
> > __u8 cache;
> > __u8 granularity;
> > - __u8 padding[2];
> > + __u8 padding[6];
> > union {
> > struct iommu_inv_pasid_info pasid_info;
> > struct iommu_inv_addr_info addr_info;
> > @@ -292,6 +296,7 @@ struct iommu_gpasid_bind_data_vtd {
> >
> > /**
> > * struct iommu_gpasid_bind_data - Information about device and
> > guest PASID binding
> > + * @argsz: User filled size of this data
> > * @version: Version of this data structure
> > * @format: PASID table entry format
> > * @flags: Additional information on guest bind request
> > @@ -309,17 +314,18 @@ struct iommu_gpasid_bind_data_vtd {
> > * PASID to host PASID based on this bind data.
> > */
> > struct iommu_gpasid_bind_data {
> > + __u32 argsz;
> > #define IOMMU_GPASID_BIND_VERSION_1 1
> > __u32 version;
> > #define IOMMU_PASID_FORMAT_INTEL_VTD 1
> > __u32 format;
> > + __u32 addr_width;
> > #define IOMMU_SVA_GPASID_VAL (1 << 0) /* guest PASID valid
> > */ __u64 flags;
> > __u64 gpgd;
> > __u64 hpasid;
> > __u64 gpasid;
> > - __u32 addr_width;
> > - __u8 padding[12];
> > + __u8 padding[8];
> > /* Vendor specific data */
> > union {
> > struct iommu_gpasid_bind_data_vtd vtd;
> >
> Thanks
>
> Eric
>
[Jacob Pan]
On Fri, 17 Jul 2020 17:58:04 +0200
Auger Eric <[email protected]> wrote:
> Hi Jacob,
> On 7/16/20 8:45 PM, Jacob Pan wrote:
>
> Could you share a branch? I was not able to apply this on either
> iommu/next or master?
>
Will push it to github for the next version. This set is based on
v5.8-rc1 and my fixes for devTLB.
> > IOMMU UAPI data has a user filled argsz field which indicates the
> > data length comes with the API call.
> s/ comes with the API call/ of the structure
> User data is not trusted, argsz must be
Sounds good.
> > validated based on the current kernel data size, mandatory data
> > size, and feature flags.
> >
> > User data may also be extended, results in possible argsz
> > increase.
> s/results/resulting
got it.
> > Backward compatibility is ensured based on size and flags
> > checking.
> flags is missing in iommu_cache_invalidate_info.
Right, I will rephrase as "flags or the functional equivalent fields"
> >
> > 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 | 192
> > ++++++++++++++++++++++++++++++++++++++++++++++++--
> > include/linux/iommu.h | 20 ++++-- 2 files changed, 200
> > insertions(+), 12 deletions(-)
> >
> > diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
> > index d43120eb1dc5..fe30a940d19e 100644
> > --- a/drivers/iommu/iommu.c
> > +++ b/drivers/iommu/iommu.c
> > @@ -1950,36 +1950,214 @@ int iommu_attach_device(struct
> > iommu_domain *domain, struct device *dev) }
> > EXPORT_SYMBOL_GPL(iommu_attach_device);
> >
> > +/*
> > + * Check flags and other user privided data for valid
> > combinations. We also
> s/privided/provided
will fix
> > + * make sure no reserved fields or unused flags are not set. This
> > is to ensure
> s/not// ?
right,
> > + * 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;
> > +
> > + if (info->version !=
> > IOMMU_CACHE_INVALIDATE_INFO_VERSION_1)
> increased version number?
Right now we just support version 1. if we increase the version number
in the future, we need to check the current kernel version is newer
than the user provided version.
Does it sounds right to you?
> > + 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:
> > + 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:
> > + /* No flags to check */
> > + break;
> > + default:
> > + return -EINVAL;
> > + }
> > +
> > + if (info->padding[0] || info->padding[1])
> padding has become "__u8 padding[6];" in 2/5
Good point, will fix
>
> > + return -EINVAL;
> > +
> > + return 0;
> > +}
> > +
> > int iommu_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, maxsz;
> > + int ret = 0;
> > +
> > if (unlikely(!domain->ops->cache_invalidate))
> > return -ENODEV;
> >
> > - return domain->ops->cache_invalidate(domain, dev,
> > inv_info);
> > + /* Current kernel data size is the max to be copied from
> > user */
> > + maxsz = sizeof(struct iommu_cache_invalidate_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 requires additional info beyond
> > minsz */
> s/requires/require
got it.
> > + if (inv_info.argsz == minsz &&
> > + ((inv_info.granularity == IOMMU_INV_GRANU_PASID) ||
> > + (inv_info.granularity ==
> > IOMMU_INV_GRANU_ADDR)))
> > + 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(inv_info.argsz, maxsz) -
> > 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_cache_invalidate);
> >
> > -int iommu_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 all supported format, for now just VT-d */
> > + mask = IOMMU_PASID_FORMAT_INTEL_VTD;
> > + if (data->format & ~mask)
> > + return -EINVAL;
> > +
> > + /* Check all flags */
> > + mask = IOMMU_SVA_GPASID_VAL;
> > + if (data->flags & ~mask)
> > + return -EINVAL;
> > +
> > + /* Check reserved padding fields */
> > + for (i = 0; i < 12; i++) {
> now 8
yes, will fix.
> > + 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, maxsz;
> > +
> > + /* Current kernel data size is the max to be copied from
> > user */
> > + maxsz = sizeof(struct iommu_gpasid_bind_data);
> > + memset((void *)data, 0, maxsz);
> > +
> > + /*
> > + * 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(data->argsz, maxsz) - minsz))
> > + return -EFAULT;
> > +
> > + return iommu_check_bind_data(data);
> > +}
> > +
> > +int iommu_sva_bind_gpasid(struct iommu_domain *domain, struct
> > device *dev,
> > + void __user
> > *udata)
> change of proto not described in the commit msg
Good point, will add that to the commit msg.
> > +{
> > +
> > + struct iommu_gpasid_bind_data data;
> > + 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_sva_bind_gpasid);
> >
> > int iommu_sva_unbind_gpasid(struct iommu_domain *domain, struct
> > device *dev,
> > - ioasid_t pasid)
> > + struct iommu_gpasid_bind_data *data)
> This change and the introduction of iommu_uapi_sva_unbind_gpasid look
> unrelated to this patch, at least not described in the commit msg.
I will move the rename into a separate patch, adding iommu_uapi_
prefix to all UAPI functions.
> > {
> > 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;
> > + 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);
> > +
> > static void __iommu_detach_device(struct iommu_domain *domain,
> > struct device *dev)
> > {
> > diff --git a/include/linux/iommu.h b/include/linux/iommu.h
> > index 5f0b7859d2eb..439f34a7b5b2 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_cache_invalidate(struct iommu_domain *domain,
> > struct device *dev,
> > - struct
> > iommu_cache_invalidate_info *inv_info);
> > + void __user *uinfo);
> > +
> > extern int iommu_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, void
> > __user *udata);
> not motivated in the commit msg. I think change of proto and addition
> of iommu_sva_unbind_gpasid would deserve a separate patch.
Will do.
> > extern int iommu_sva_unbind_gpasid(struct iommu_domain *domain,
> > - struct device *dev, ioasid_t
> > pasid);
> > + 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, @@
> > -1062,13 +1065,20 @@ iommu_cache_invalidate(struct iommu_domain
> > *domain, return -ENODEV; }
> > static inline int iommu_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_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;
> > }
> >
>
> Thanks
>
> Eric
>
[Jacob Pan]
On Fri, 17 Jul 2020 13:59:54 -0600
Alex Williamson <[email protected]> wrote:
> On Thu, 16 Jul 2020 11:45:16 -0700
> Jacob Pan <[email protected]> wrote:
>
> > IOMMU UAPI data has a user filled argsz field which indicates the
> > data length comes with the API call. 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, results in possible argsz increase.
> > Backward compatibility is ensured based on size and flags 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 | 192
> > ++++++++++++++++++++++++++++++++++++++++++++++++--
> > include/linux/iommu.h | 20 ++++-- 2 files changed, 200
> > insertions(+), 12 deletions(-)
> >
> > diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
> > index d43120eb1dc5..fe30a940d19e 100644
> > --- a/drivers/iommu/iommu.c
> > +++ b/drivers/iommu/iommu.c
> > @@ -1950,36 +1950,214 @@ int iommu_attach_device(struct
> > iommu_domain *domain, struct device *dev) }
> > EXPORT_SYMBOL_GPL(iommu_attach_device);
> >
> > +/*
> > + * Check flags and other user privided data for valid
> > combinations. We also
> > + * make sure no reserved fields or unused flags are not 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;
> > +
> > + 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:
> > + 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:
> > + /* No flags to check */
> > + break;
> > + default:
> > + return -EINVAL;
> > + }
> > +
> > + if (info->padding[0] || info->padding[1])
> > + return -EINVAL;
> > +
> > + return 0;
> > +}
> > +
> > int iommu_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, maxsz;
> > + int ret = 0;
> > +
> > if (unlikely(!domain->ops->cache_invalidate))
> > return -ENODEV;
> >
> > - return domain->ops->cache_invalidate(domain, dev,
> > inv_info);
> > + /* Current kernel data size is the max to be copied from
> > user */
> > + maxsz = sizeof(struct iommu_cache_invalidate_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 requires 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;
> > + /*
> > + * 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(inv_info.argsz, maxsz) -
> > minsz))
> > + return -EFAULT;
>
> To further clarify previous comments about maxsz usage:
>
> s/maxsz/sizeof(inv_info)/
>
Yes, will remove maxsz.
> > +
> > + /* 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_cache_invalidate);
>
>
> Remainder of comments for above provided in 1/5.
>
Got it.
> >
> > -int iommu_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 all supported format, for now just VT-d */
> > + mask = IOMMU_PASID_FORMAT_INTEL_VTD;
> > + if (data->format & ~mask)
> > + return -EINVAL;
> > +
> > + /* Check all flags */
> > + mask = IOMMU_SVA_GPASID_VAL;
> > + if (data->flags & ~mask)
> > + return -EINVAL;
> > +
> > + /* Check reserved padding fields */
> > + for (i = 0; i < 12; 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, maxsz;
> > +
> > + /* Current kernel data size is the max to be copied from
> > user */
> > + maxsz = sizeof(struct iommu_gpasid_bind_data);
> > + memset((void *)data, 0, maxsz);
>
> Let the caller pass a zero'd structure.
>
Sounds good.
> > +
> > + /*
> > + * 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(data->argsz, maxsz) - minsz))
>
>
> We could replace maxsz entirely with sizeof(*data)
will do.
>
> > + return -EFAULT;
> > +
> > + return iommu_check_bind_data(data);
> > +}
> > +
> > +int iommu_sva_bind_gpasid(struct iommu_domain *domain, struct
> > device *dev,
> > + void __user
> > *udata)
>
> iommu_uapi_sva_bind_gpasid(...
>
will do. make naming consistent for all UAPIs.
> > +{
> > +
> > + struct iommu_gpasid_bind_data data;
>
> = { 0 }
>
yes, callee does not need to zero out the data.
> > + 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_sva_bind_gpasid);
> >
> > int iommu_sva_unbind_gpasid(struct iommu_domain *domain, struct
> > device *dev,
> > - ioasid_t pasid)
> > + 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 };
Ditto
>
> > + 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);
> > +
> > static void __iommu_detach_device(struct iommu_domain *domain,
> > struct device *dev)
> > {
> > diff --git a/include/linux/iommu.h b/include/linux/iommu.h
> > index 5f0b7859d2eb..439f34a7b5b2 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_cache_invalidate(struct iommu_domain *domain,
>
> uapi
got it.
>
> > struct device *dev,
> > - struct
> > iommu_cache_invalidate_info *inv_info);
> > + void __user *uinfo);
> > +
> > extern int iommu_sva_bind_gpasid(struct iommu_domain *domain,
>
> uapi
>
got it.
> > - 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, void
> > __user *udata); extern int iommu_sva_unbind_gpasid(struct
> > iommu_domain *domain,
> > - struct device *dev, ioasid_t
> > pasid);
> > + 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, @@
> > -1062,13 +1065,20 @@ iommu_cache_invalidate(struct iommu_domain
> > *domain, return -ENODEV; }
> > static inline int iommu_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_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,
>
>
> These don't match the previous semantics. Thanks,
>
My mistake, will fix.
Thanks
> Alex
>
> > + struct device *dev,
> > + struct
> > iommu_gpasid_bind_data *data) {
> > return -ENODEV;
> > }
>
[Jacob Pan]