| Message ID | 2-v1-1211e1294c27+4b1-iommu_no_prot_jgg@nvidia.com |
|---|---|
| State | Accepted |
| Commit | 6093cd582f8e027117a8d4ad5d129a1aacdc53d2 |
| Headers | show |
| Series | Fix maps created without READ or WRITE | expand |
On Thu, Aug 22, 2024 at 11:45:55AM -0300, Jason Gunthorpe wrote: > These three implementations of map_pages() all succeed if a mapping is > requested with no read or write. Since they return back to __iommu_map() > leaving the mapped output as 0 it triggers an infinite loop. Therefore > nothing is using no-access protection bits. > > Further, VFIO and iommufd rely on iommu_iova_to_phys() to get back PFNs > stored by map, if iommu_map() succeeds but iommu_iova_to_phys() fails that > will create serious bugs. > > Thus remove this never used "nothing to do" concept and just fail map > immediately. > > Fixes: e5fc9753b1a8 ("iommu/io-pgtable: Add ARMv7 short descriptor support") > Fixes: e1d3c0fd701d ("iommu: add ARM LPAE page table allocator") > Fixes: 745ef1092bcf ("iommu/io-pgtable: Move Apple DART support to its own file") > Signed-off-by: Jason Gunthorpe <jgg@nvidia.com> > --- > drivers/iommu/io-pgtable-arm-v7s.c | 3 +-- > drivers/iommu/io-pgtable-arm.c | 3 +-- > drivers/iommu/io-pgtable-dart.c | 3 +-- > 3 files changed, 3 insertions(+), 6 deletions(-) > > diff --git a/drivers/iommu/io-pgtable-arm-v7s.c b/drivers/iommu/io-pgtable-arm-v7s.c > index 75f244a3e12df6..06ffc683b28fee 100644 > --- a/drivers/iommu/io-pgtable-arm-v7s.c > +++ b/drivers/iommu/io-pgtable-arm-v7s.c > @@ -552,9 +552,8 @@ static int arm_v7s_map_pages(struct io_pgtable_ops *ops, unsigned long iova, > paddr >= (1ULL << data->iop.cfg.oas))) > return -ERANGE; > > - /* If no access, then nothing to do */ > if (!(prot & (IOMMU_READ | IOMMU_WRITE))) > - return 0; > + return -EINVAL; > > while (pgcount--) { > ret = __arm_v7s_map(data, iova, paddr, pgsize, prot, 1, data->pgd, > diff --git a/drivers/iommu/io-pgtable-arm.c b/drivers/iommu/io-pgtable-arm.c > index f5d9fd1f45bf49..ff4149ae1751d4 100644 > --- a/drivers/iommu/io-pgtable-arm.c > +++ b/drivers/iommu/io-pgtable-arm.c > @@ -515,9 +515,8 @@ static int arm_lpae_map_pages(struct io_pgtable_ops *ops, unsigned long iova, > if (WARN_ON(iaext || paddr >> cfg->oas)) > return -ERANGE; > > - /* If no access, then nothing to do */ > if (!(iommu_prot & (IOMMU_READ | IOMMU_WRITE))) > - return 0; > + return -EINVAL; I think just removing this hunk altogether would get us the right semantics for stage-2 mappings, but it's esoteric and not used so -EINVAL is probably better: Acked-by: Will Deacon <will@kernel.org> Will
> From: Jason Gunthorpe <jgg@nvidia.com> > Sent: Thursday, August 22, 2024 10:46 PM > > These three implementations of map_pages() all succeed if a mapping is > requested with no read or write. Since they return back to __iommu_map() > leaving the mapped output as 0 it triggers an infinite loop. Therefore > nothing is using no-access protection bits. > > Further, VFIO and iommufd rely on iommu_iova_to_phys() to get back PFNs > stored by map, if iommu_map() succeeds but iommu_iova_to_phys() fails > that > will create serious bugs. > > Thus remove this never used "nothing to do" concept and just fail map > immediately. > > Fixes: e5fc9753b1a8 ("iommu/io-pgtable: Add ARMv7 short descriptor > support") > Fixes: e1d3c0fd701d ("iommu: add ARM LPAE page table allocator") > Fixes: 745ef1092bcf ("iommu/io-pgtable: Move Apple DART support to its > own file") > Signed-off-by: Jason Gunthorpe <jgg@nvidia.com> Reviewed-by: Kevin Tian <kevin.tian@intel.com>
diff --git a/drivers/iommu/io-pgtable-arm-v7s.c b/drivers/iommu/io-pgtable-arm-v7s.c index 75f244a3e12df6..06ffc683b28fee 100644 --- a/drivers/iommu/io-pgtable-arm-v7s.c +++ b/drivers/iommu/io-pgtable-arm-v7s.c @@ -552,9 +552,8 @@ static int arm_v7s_map_pages(struct io_pgtable_ops *ops, unsigned long iova, paddr >= (1ULL << data->iop.cfg.oas))) return -ERANGE; - /* If no access, then nothing to do */ if (!(prot & (IOMMU_READ | IOMMU_WRITE))) - return 0; + return -EINVAL; while (pgcount--) { ret = __arm_v7s_map(data, iova, paddr, pgsize, prot, 1, data->pgd, diff --git a/drivers/iommu/io-pgtable-arm.c b/drivers/iommu/io-pgtable-arm.c index f5d9fd1f45bf49..ff4149ae1751d4 100644 --- a/drivers/iommu/io-pgtable-arm.c +++ b/drivers/iommu/io-pgtable-arm.c @@ -515,9 +515,8 @@ static int arm_lpae_map_pages(struct io_pgtable_ops *ops, unsigned long iova, if (WARN_ON(iaext || paddr >> cfg->oas)) return -ERANGE; - /* If no access, then nothing to do */ if (!(iommu_prot & (IOMMU_READ | IOMMU_WRITE))) - return 0; + return -EINVAL; prot = arm_lpae_prot_to_pte(data, iommu_prot); ret = __arm_lpae_map(data, iova, paddr, pgsize, pgcount, prot, lvl, diff --git a/drivers/iommu/io-pgtable-dart.c b/drivers/iommu/io-pgtable-dart.c index ad28031e1e93d6..c004640640ee50 100644 --- a/drivers/iommu/io-pgtable-dart.c +++ b/drivers/iommu/io-pgtable-dart.c @@ -245,9 +245,8 @@ static int dart_map_pages(struct io_pgtable_ops *ops, unsigned long iova, if (WARN_ON(paddr >> cfg->oas)) return -ERANGE; - /* If no access, then nothing to do */ if (!(iommu_prot & (IOMMU_READ | IOMMU_WRITE))) - return 0; + return -EINVAL; tbl = dart_get_table(data, iova);
These three implementations of map_pages() all succeed if a mapping is requested with no read or write. Since they return back to __iommu_map() leaving the mapped output as 0 it triggers an infinite loop. Therefore nothing is using no-access protection bits. Further, VFIO and iommufd rely on iommu_iova_to_phys() to get back PFNs stored by map, if iommu_map() succeeds but iommu_iova_to_phys() fails that will create serious bugs. Thus remove this never used "nothing to do" concept and just fail map immediately. Fixes: e5fc9753b1a8 ("iommu/io-pgtable: Add ARMv7 short descriptor support") Fixes: e1d3c0fd701d ("iommu: add ARM LPAE page table allocator") Fixes: 745ef1092bcf ("iommu/io-pgtable: Move Apple DART support to its own file") Signed-off-by: Jason Gunthorpe <jgg@nvidia.com> --- drivers/iommu/io-pgtable-arm-v7s.c | 3 +-- drivers/iommu/io-pgtable-arm.c | 3 +-- drivers/iommu/io-pgtable-dart.c | 3 +-- 3 files changed, 3 insertions(+), 6 deletions(-)