Re: [PATCH v11 5/6] iommu/uapi: Handle data and argsz filled by users
Hi Jean-Philippe,
On Fri, 25 Sep 2020 11:46:36 +0200, Jean-Philippe Brucker
wrote:
> On Thu, Sep 24, 2020 at 12:24:19PM -0700, 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.
> >
> > 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
> > Signed-off-by: Jacob Pan
>
> Reviewed-by: Jean-Philippe Brucker
>
> Some comments below in case you're resending, but nothing important.
>
Thanks for the review, I will respin.
> > ---
> > drivers/iommu/iommu.c | 199
> > +++--
> > include/linux/iommu.h | 28 +-- include/uapi/linux/iommu.h |
> > 1 + 3 files changed, 212 insertions(+), 16 deletions(-)
> >
> > diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
> > index 4ae02291ccc2..5c1b7ae48aae 100644
> > --- a/drivers/iommu/iommu.c
> > +++ b/drivers/iommu/iommu.c
> > @@ -1961,34 +1961,219 @@ 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;
>
> nit: no need to initialize it
>
got it.
> > +
> > 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);
>
> Why not use offsetofend() to avoid naming the unions?
>
offsetofend() was used in earlier version but the named union would avoid
future code change if we were to re-purpose the padding fields.
minzs is always at the offsetof the union due to our expansion rules.
> > +
> > + /* Copy minsz from user to get flags and argsz */
> > + if (copy_from_user(&inv_info, uinfo, minsz))
> > + return -EFAULT;
> > +
> > + /* Fiel
Re: [PATCH v11 5/6] iommu/uapi: Handle data and argsz filled by users
On Thu, Sep 24, 2020 at 12:24:19PM -0700, 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.
>
> 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
> Signed-off-by: Jacob Pan
Reviewed-by: Jean-Philippe Brucker
Some comments below in case you're resending, but nothing important.
> ---
> drivers/iommu/iommu.c | 199
> +++--
> include/linux/iommu.h | 28 +--
> include/uapi/linux/iommu.h | 1 +
> 3 files changed, 212 insertions(+), 16 deletions(-)
>
> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
> index 4ae02291ccc2..5c1b7ae48aae 100644
> --- a/drivers/iommu/iommu.c
> +++ b/drivers/iommu/iommu.c
> @@ -1961,34 +1961,219 @@ 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;
nit: no need to initialize it
> +
> 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);
Why not use offsetofend() to avoid naming the unions?
> +
> + /* 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;
Made redundant by the two checks below
> +
> + if (inv_info.granularity == IOMMU_INV_GRANU_P
[PATCH v11 5/6] iommu/uapi: Handle data and argsz filled by users
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
Signed-off-by: Jacob Pan
---
drivers/iommu/iommu.c | 199 +++--
include/linux/iommu.h | 28 +--
include/uapi/linux/iommu.h | 1 +
3 files changed, 212 insertions(+), 16 deletions(-)
diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index 4ae02291ccc2..5c1b7ae48aae 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -1961,34 +1961,219 @@ 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
+*
