| Message ID | 1-v1-1211e1294c27+4b1-iommu_no_prot_jgg@nvidia.com |
|---|---|
| State | Accepted |
| Commit | 996dc53ac289b81957aa70d62ccadc6986d26a87 |
| Headers | show |
| Series | Fix maps created without READ or WRITE | expand |
On Thu, Aug 22, 2024 at 11:45:54AM -0300, Jason Gunthorpe wrote: > This results in passing 0 or just IOMMU_CACHE to iommu_map(). Most of > the page table formats don't like this: > > amdv1 - -EINVAL > armv7s - returns 0, doesn't update mapped > arm-lpae - returns 0 doesn't update mapped > dart - returns 0, doesn't update mapped > VT-D - returns -EINVAL > > Unfortunately the three formats that return 0 cause serious problems: > > - Returning ret = but not uppdating mapped from domain->map_pages() > causes an infinite loop in __iommu_map() > > - Not writing ioptes means that VFIO/iommufd have no way to recover them > and we will have memory leaks and worse during unmap > > Since almost nothing can support this, and it is a useless thing to do, > block it early in iommufd. > > Cc: stable@kernel.org > Fixes: aad37e71d5c4 ("iommufd: IOCTLs for the io_pagetable") > Signed-off-by: Jason Gunthorpe <jgg@nvidia.com> Reviewed-by: Nicolin Chen <nicolinc@nvidia.com> I also tried both patches with io-pgtable-arm and didn't see any issue, since they tends to fix a corner case I think. Nicolin
> From: Jason Gunthorpe <jgg@nvidia.com> > Sent: Thursday, August 22, 2024 10:46 PM > > This results in passing 0 or just IOMMU_CACHE to iommu_map(). Most of > the page table formats don't like this: > > amdv1 - -EINVAL > armv7s - returns 0, doesn't update mapped > arm-lpae - returns 0 doesn't update mapped > dart - returns 0, doesn't update mapped > VT-D - returns -EINVAL > > Unfortunately the three formats that return 0 cause serious problems: > > - Returning ret = but not uppdating mapped from domain->map_pages() > causes an infinite loop in __iommu_map() > > - Not writing ioptes means that VFIO/iommufd have no way to recover them > and we will have memory leaks and worse during unmap > > Since almost nothing can support this, and it is a useless thing to do, > block it early in iommufd. > > Cc: stable@kernel.org > Fixes: aad37e71d5c4 ("iommufd: IOCTLs for the io_pagetable") > Signed-off-by: Jason Gunthorpe <jgg@nvidia.com> Reviewed-by: Kevin Tian <kevin.tian@intel.com>
diff --git a/drivers/iommu/iommufd/ioas.c b/drivers/iommu/iommufd/ioas.c index 82428e44a837ca..2c4b2bb11e78ce 100644 --- a/drivers/iommu/iommufd/ioas.c +++ b/drivers/iommu/iommufd/ioas.c @@ -213,6 +213,10 @@ int iommufd_ioas_map(struct iommufd_ucmd *ucmd) if (cmd->iova >= ULONG_MAX || cmd->length >= ULONG_MAX) return -EOVERFLOW; + if (!(cmd->flags & + (IOMMU_IOAS_MAP_WRITEABLE | IOMMU_IOAS_MAP_READABLE))) + return -EINVAL; + ioas = iommufd_get_ioas(ucmd->ictx, cmd->ioas_id); if (IS_ERR(ioas)) return PTR_ERR(ioas); @@ -253,6 +257,10 @@ int iommufd_ioas_copy(struct iommufd_ucmd *ucmd) cmd->dst_iova >= ULONG_MAX) return -EOVERFLOW; + if (!(cmd->flags & + (IOMMU_IOAS_MAP_WRITEABLE | IOMMU_IOAS_MAP_READABLE))) + return -EINVAL; + src_ioas = iommufd_get_ioas(ucmd->ictx, cmd->src_ioas_id); if (IS_ERR(src_ioas)) return PTR_ERR(src_ioas); diff --git a/tools/testing/selftests/iommu/iommufd.c b/tools/testing/selftests/iommu/iommufd.c index 6343f4053bd46e..4927b9add5add9 100644 --- a/tools/testing/selftests/iommu/iommufd.c +++ b/tools/testing/selftests/iommu/iommufd.c @@ -825,7 +825,7 @@ TEST_F(iommufd_ioas, copy_area) { struct iommu_ioas_copy copy_cmd = { .size = sizeof(copy_cmd), - .flags = IOMMU_IOAS_MAP_FIXED_IOVA, + .flags = IOMMU_IOAS_MAP_FIXED_IOVA | IOMMU_IOAS_MAP_WRITEABLE, .dst_ioas_id = self->ioas_id, .src_ioas_id = self->ioas_id, .length = PAGE_SIZE, @@ -1318,7 +1318,7 @@ TEST_F(iommufd_ioas, copy_sweep) { struct iommu_ioas_copy copy_cmd = { .size = sizeof(copy_cmd), - .flags = IOMMU_IOAS_MAP_FIXED_IOVA, + .flags = IOMMU_IOAS_MAP_FIXED_IOVA | IOMMU_IOAS_MAP_WRITEABLE, .src_ioas_id = self->ioas_id, .dst_iova = MOCK_APERTURE_START, .length = MOCK_PAGE_SIZE, @@ -1608,7 +1608,7 @@ TEST_F(iommufd_mock_domain, user_copy) }; struct iommu_ioas_copy copy_cmd = { .size = sizeof(copy_cmd), - .flags = IOMMU_IOAS_MAP_FIXED_IOVA, + .flags = IOMMU_IOAS_MAP_FIXED_IOVA | IOMMU_IOAS_MAP_WRITEABLE, .dst_ioas_id = self->ioas_id, .dst_iova = MOCK_APERTURE_START, .length = BUFFER_SIZE,
This results in passing 0 or just IOMMU_CACHE to iommu_map(). Most of the page table formats don't like this: amdv1 - -EINVAL armv7s - returns 0, doesn't update mapped arm-lpae - returns 0 doesn't update mapped dart - returns 0, doesn't update mapped VT-D - returns -EINVAL Unfortunately the three formats that return 0 cause serious problems: - Returning ret = but not uppdating mapped from domain->map_pages() causes an infinite loop in __iommu_map() - Not writing ioptes means that VFIO/iommufd have no way to recover them and we will have memory leaks and worse during unmap Since almost nothing can support this, and it is a useless thing to do, block it early in iommufd. Cc: stable@kernel.org Fixes: aad37e71d5c4 ("iommufd: IOCTLs for the io_pagetable") Signed-off-by: Jason Gunthorpe <jgg@nvidia.com> --- drivers/iommu/iommufd/ioas.c | 8 ++++++++ tools/testing/selftests/iommu/iommufd.c | 6 +++--- 2 files changed, 11 insertions(+), 3 deletions(-)