[PATCH] iommufd: Fix refcounting race during mmap

[PATCH] iommufd: Fix refcounting race during mmap

[Jason Gunthorpe]

The owner object of the imap can be destroyed while the imap remains in
the mtree. So access to the imap pointer without holding locks is racy
with destruction.

The imap is safe to access outside the lock once a users refcount is
obtained, the owner object cannot start destruction until users is 0.

Thus the users refcount should not be obtained at the end of
iommufd_fops_mmap() but instead inside the mtree lock held around the
mtree_load(). Move the refcount there and use refcount_inc_not_zero() as
we can have a 0 refcount inside the mtree during destruction races.

Cc: stable@vger.kernel.org
Fixes: 56e9a0d8e53f ("iommufd: Add mmap interface")
Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
---
 drivers/iommu/iommufd/main.c | 20 ++++++++++++++------
 1 file changed, 14 insertions(+), 6 deletions(-)

diff --git a/drivers/iommu/iommufd/main.c b/drivers/iommu/iommufd/main.c
index 15af7ced0501d6..109de747e8b3ed 100644
--- a/drivers/iommu/iommufd/main.c
+++ b/drivers/iommu/iommufd/main.c
@@ -551,15 +551,22 @@ static int iommufd_fops_mmap(struct file *filp, struct vm_area_struct *vma)
 		return -EPERM;
 
 	/* vma->vm_pgoff carries a page-shifted start position to an immap */
+	mtree_lock(&ictx->mt_mmap);
 	immap = mtree_load(&ictx->mt_mmap, vma->vm_pgoff << PAGE_SHIFT);
-	if (!immap)
+	if (!immap || !refcount_inc_not_zero(&immap->owner->users)) {
+		mtree_unlock(&ictx->mt_mmap);
 		return -ENXIO;
+	}
+	mtree_unlock(&ictx->mt_mmap);
+
 	/*
 	 * mtree_load() returns the immap for any contained mmio_addr, so only
 	 * allow the exact immap thing to be mapped
 	 */
-	if (vma->vm_pgoff != immap->vm_pgoff || length != immap->length)
-		return -ENXIO;
+	if (vma->vm_pgoff != immap->vm_pgoff || length != immap->length) {
+		rc = -ENXIO;
+		goto err_refcount;
+	}
 
 	vma->vm_pgoff = 0;
 	vma->vm_private_data = immap;
@@ -570,10 +577,11 @@ static int iommufd_fops_mmap(struct file *filp, struct vm_area_struct *vma)
 				immap->mmio_addr >> PAGE_SHIFT, length,
 				vma->vm_page_prot);
 	if (rc)
-		return rc;
+		goto err_refcount;
+	return 0;
 
-	/* vm_ops.open won't be called for mmap itself. */
-	refcount_inc(&immap->owner->users);
+err_refcount:
+	refcount_dec(&immap->owner->users);
 	return rc;
 }
 

base-commit: 8f5ae30d69d7543eee0d70083daf4de8fe15d585
-- 
2.43.0

Re: [PATCH] iommufd: Fix refcounting race during mmap

[Nicolin Chen]

On Tue, Sep 16, 2025 at 01:10:07PM -0300, Jason Gunthorpe wrote:
> The owner object of the imap can be destroyed while the imap remains in
> the mtree. So access to the imap pointer without holding locks is racy
> with destruction.
> 
> The imap is safe to access outside the lock once a users refcount is
> obtained, the owner object cannot start destruction until users is 0.
> 
> Thus the users refcount should not be obtained at the end of
> iommufd_fops_mmap() but instead inside the mtree lock held around the
> mtree_load(). Move the refcount there and use refcount_inc_not_zero() as
> we can have a 0 refcount inside the mtree during destruction races.
> 
> Cc: stable@vger.kernel.org
> Fixes: 56e9a0d8e53f ("iommufd: Add mmap interface")
> Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>

Reviewed-by: Nicolin Chen <nicolinc@nvidia.com>

> ---
>  drivers/iommu/iommufd/main.c | 20 ++++++++++++++------
>  1 file changed, 14 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/iommu/iommufd/main.c b/drivers/iommu/iommufd/main.c
> index 15af7ced0501d6..109de747e8b3ed 100644
> --- a/drivers/iommu/iommufd/main.c
> +++ b/drivers/iommu/iommufd/main.c
> @@ -551,15 +551,22 @@ static int iommufd_fops_mmap(struct file *filp, struct vm_area_struct *vma)
>  		return -EPERM;
>  
>  	/* vma->vm_pgoff carries a page-shifted start position to an immap */
> +	mtree_lock(&ictx->mt_mmap);
>  	immap = mtree_load(&ictx->mt_mmap, vma->vm_pgoff << PAGE_SHIFT);

Nit: should we put the comment line closer to mtree_load()?

Re: [PATCH] iommufd: Fix refcounting race during mmap

[Jason Gunthorpe]

On Wed, Sep 17, 2025 at 11:34:15AM -0700, Nicolin Chen wrote:
> > @@ -551,15 +551,22 @@ static int iommufd_fops_mmap(struct file *filp, struct vm_area_struct *vma)
> >  		return -EPERM;
> >  
> >  	/* vma->vm_pgoff carries a page-shifted start position to an immap */
> > +	mtree_lock(&ictx->mt_mmap);
> >  	immap = mtree_load(&ictx->mt_mmap, vma->vm_pgoff << PAGE_SHIFT);
> 
> Nit: should we put the comment line closer to mtree_load()?

Ok

Thanks,
Jason

RE: [PATCH] iommufd: Fix refcounting race during mmap

[Tian, Kevin]

> From: Jason Gunthorpe <jgg@nvidia.com>
> Sent: Wednesday, September 17, 2025 12:10 AM
> 
> The owner object of the imap can be destroyed while the imap remains in
> the mtree. So access to the imap pointer without holding locks is racy
> with destruction.
> 
> The imap is safe to access outside the lock once a users refcount is
> obtained, the owner object cannot start destruction until users is 0.
> 
> Thus the users refcount should not be obtained at the end of
> iommufd_fops_mmap() but instead inside the mtree lock held around the
> mtree_load(). Move the refcount there and use refcount_inc_not_zero() as
> we can have a 0 refcount inside the mtree during destruction races.
> 
> Cc: stable@vger.kernel.org
> Fixes: 56e9a0d8e53f ("iommufd: Add mmap interface")
> Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>

Reviewed-by: Kevin Tian <kevin.tian@intel.com>

Re: [PATCH] iommufd: Fix refcounting race during mmap

[Jason Gunthorpe]

On Tue, Sep 16, 2025 at 01:10:07PM -0300, Jason Gunthorpe wrote:
> The owner object of the imap can be destroyed while the imap remains in
> the mtree. So access to the imap pointer without holding locks is racy
> with destruction.
> 
> The imap is safe to access outside the lock once a users refcount is
> obtained, the owner object cannot start destruction until users is 0.
> 
> Thus the users refcount should not be obtained at the end of
> iommufd_fops_mmap() but instead inside the mtree lock held around the
> mtree_load(). Move the refcount there and use refcount_inc_not_zero() as
> we can have a 0 refcount inside the mtree during destruction races.
> 
> Cc: stable@vger.kernel.org
> Fixes: 56e9a0d8e53f ("iommufd: Add mmap interface")
> Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
> ---
>  drivers/iommu/iommufd/main.c | 20 ++++++++++++++------
>  1 file changed, 14 insertions(+), 6 deletions(-)

Applied to for-rc

Jason