> From: Jason Gunthorpe <jgg@xxxxxxxxxx>
> Sent: Saturday, July 29, 2023 2:03 AM
>
> On Mon, Jul 24, 2023 at 04:03:58AM -0700, Yi Liu wrote:
> > In nested translation, the stage-1 page table is user-managed and used by
> > IOMMU hardware, so update of any present page table entry in the stage-1
> > page table should be followed with an IOTLB invalidation.
> >
> > This adds IOMMU_HWPT_INVALIDATE for stage-1 IOTLB invalidation.
> >
> > Co-developed-by: Nicolin Chen <nicolinc@xxxxxxxxxx>
> > Signed-off-by: Nicolin Chen <nicolinc@xxxxxxxxxx>
> > Signed-off-by: Yi Liu <yi.l.liu@xxxxxxxxx>
> > ---
> > drivers/iommu/iommufd/hw_pagetable.c | 45 +++++++++++++++++++++++++
> > drivers/iommu/iommufd/iommufd_private.h | 9 +++++
> > drivers/iommu/iommufd/main.c | 3 ++
> > include/uapi/linux/iommufd.h | 22 ++++++++++++
> > 4 files changed, 79 insertions(+)
> >
> > diff --git a/drivers/iommu/iommufd/hw_pagetable.c
> b/drivers/iommu/iommufd/hw_pagetable.c
> > index 97e4114226de..9064e6d181b4 100644
> > --- a/drivers/iommu/iommufd/hw_pagetable.c
> > +++ b/drivers/iommu/iommufd/hw_pagetable.c
> > @@ -286,3 +286,48 @@ int iommufd_hwpt_alloc(struct iommufd_ucmd *ucmd)
> > iommufd_put_object(&idev->obj);
> > return rc;
> > }
> > +
> > +int iommufd_hwpt_invalidate(struct iommufd_ucmd *ucmd)
> > +{
> > + struct iommu_hwpt_invalidate *cmd = ucmd->cmd;
> > + struct iommufd_hw_pagetable *hwpt;
> > + u32 user_data_len, klen;
> > + u64 user_ptr;
> > + int rc = 0;
> > +
> > + if (!cmd->data_len || cmd->__reserved)
> > + return -EOPNOTSUPP;
> > +
> > + hwpt = iommufd_get_hwpt(ucmd, cmd->hwpt_id);
> > + if (IS_ERR(hwpt))
> > + return PTR_ERR(hwpt);
> > +
> > + /* Do not allow any kernel-managed hw_pagetable */
> > + if (!hwpt->parent) {
>
> I don't think this is needed because:
>
> > + rc = -EINVAL;
> > + goto out_put_hwpt;
> > + }
> > +
> > + klen = hwpt->domain->ops->cache_invalidate_user_data_len;
> > + if (!hwpt->domain->ops->cache_invalidate_user || !klen) {
> > + rc = -EOPNOTSUPP;
>
> We need to get to a place where the drivers are providing proper ops
> for the domains, so this op should never exist for a paging domain.
Yes.
>
> And return EINVAL here instead.
Sure.
>
> > + goto out_put_hwpt;
> > + }
> > +
> > + /*
> > + * Copy the needed fields before reusing the ucmd buffer, this
> > + * avoids memory allocation in this path.
> > + */
> > + user_ptr = cmd->data_uptr;
> > + user_data_len = cmd->data_len;
>
> Uhh, who checks that klen < the temporary stack struct?
Take vtd as an example. The invalidate structure is struct iommu_hwpt_vtd_s1_invalidate[1].
The klen is sizeof(struct iommu_hwpt_vtd_s1_invalidate)[2]. iommu_hwpt_vtd_s1_invalidate
is also placed in the temporary stack struct (actually it is a union)[1]. So the klen should
be <= temporary stack.
[1] https://lore.kernel.org/linux-iommu/20230724111335.107427-8-yi.l.liu@xxxxxxxxx/
[2] https://lore.kernel.org/linux-iommu/20230724111335.107427-10-yi.l.liu@xxxxxxxxx/
It's not so explicit though. Perhaps worth to have a check like below in this patch?
if (unlikely(klen > sizeof(union ucmd_buffer)))
return -EINVAL;
Regards,
Yi Liu
