[PATCH rc 0/2] Fix maps created without READ or WRITE

[PATCH rc 0/2] Fix maps created without READ or WRITE

[Jason Gunthorpe]

I noticed some bugs here while working on iommupt. Fix them up.

Joerg, can you pick this both for your -rc branch?

Thanks,
Jason

Jason Gunthorpe (2):
  iommufd: Do not allow creating areas without READ or WRITE
  iommu: Do not return 0 from map_pages if it doesn't do anything

 drivers/iommu/io-pgtable-arm-v7s.c      | 3 +--
 drivers/iommu/io-pgtable-arm.c          | 3 +--
 drivers/iommu/io-pgtable-dart.c         | 3 +--
 drivers/iommu/iommufd/ioas.c            | 8 ++++++++
 tools/testing/selftests/iommu/iommufd.c | 6 +++---
 5 files changed, 14 insertions(+), 9 deletions(-)


base-commit: 4be8b00b2b0f669989486e9f2fb9b65edb4ef8c4
-- 
2.46.0

[PATCH rc 1/2] iommufd: Do not allow creating areas without READ or WRITE

[Jason Gunthorpe]

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(-)

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,
-- 
2.46.0

[PATCH rc 2/2] iommu: Do not return 0 from map_pages if it doesn't do anything

[Jason Gunthorpe]

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;
 
 	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);
 
-- 
2.46.0

Re: [PATCH rc 2/2] iommu: Do not return 0 from map_pages if it doesn't do anything

[Will Deacon]

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

Re: [PATCH rc 1/2] iommufd: Do not allow creating areas without READ or WRITE

[Nicolin Chen]

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

RE: [PATCH rc 1/2] iommufd: Do not allow creating areas without READ or WRITE

[Tian, Kevin]

> 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>

RE: [PATCH rc 2/2] iommu: Do not return 0 from map_pages if it doesn't do anything

[Tian, Kevin]

> 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>

Re: [PATCH rc 0/2] Fix maps created without READ or WRITE

[Joerg Roedel]

On Thu, Aug 22, 2024 at 11:45:53AM -0300, Jason Gunthorpe wrote:
> I noticed some bugs here while working on iommupt. Fix them up.
> 
> Joerg, can you pick this both for your -rc branch?

Dones, applied to fixes branch, thanks.