[PATCH rc 0/2] iommu/tegra241-cmdqv: Two bug fixes in fallback routine

[PATCH rc 0/2] iommu/tegra241-cmdqv: Two bug fixes in fallback routine

[Nicolin Chen]

Hi Will,

Here are two patches fixing two kernel warnings and a UAF bug.

Both need to be delivered to stable trees.

Thanks
Nicolin

Nicolin Chen (2):
  iommu/tegra241-cmdqv: Fix warnings due to dmam_free_coherent()
  iommu/tegra241-cmdqv: Fix UAF due to re-entry of
    tegra241_cmdqv_remove()

 drivers/iommu/arm/arm-smmu-v3/tegra241-cmdqv.c | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

-- 
2.43.0

[PATCH rc 1/2] iommu/tegra241-cmdqv: Fix warnings due to dmam_free_coherent()

[Nicolin Chen]

Two WARNINGs are observed when SMMU driver rolls back upon failure:
 arm-smmu-v3.9.auto: Failed to register iommu
 arm-smmu-v3.9.auto: probe with driver arm-smmu-v3 failed with error -22
 ------------[ cut here ]------------
 WARNING: CPU: 5 PID: 1 at kernel/dma/mapping.c:74 dmam_free_coherent+0xc0/0xd8
 Call trace:
  dmam_free_coherent+0xc0/0xd8 (P)
  tegra241_vintf_free_lvcmdq+0x74/0x188
  tegra241_cmdqv_remove_vintf+0x60/0x148
  tegra241_cmdqv_remove+0x48/0xc8
  arm_smmu_impl_remove+0x28/0x60
  devm_action_release+0x1c/0x40
 ------------[ cut here ]------------
 128 pages are still in use!
 WARNING: CPU: 16 PID: 1 at mm/page_alloc.c:6902 free_contig_range+0x18c/0x1c8
 Call trace:
  free_contig_range+0x18c/0x1c8 (P)
  cma_release+0x154/0x2f0
  dma_free_contiguous+0x38/0xa0
  dma_direct_free+0x10c/0x248
  dma_free_attrs+0x100/0x290
  dmam_free_coherent+0x78/0xd8
  tegra241_vintf_free_lvcmdq+0x74/0x160
  tegra241_cmdqv_remove+0x98/0x198
  arm_smmu_impl_remove+0x28/0x60
  devm_action_release+0x1c/0x40

For the first warning: when the main SMMU driver cleans up its resources,
any routine in arm_smmu_impl_remove() should not use any devres function.

For the second warning: since those pages were allocated using smmu->dev
via devres, they should be just freed by devres.

tegra241_vcmdq_free_smmu_cmdq() is called by tegra241_cmdqv_init_vintf()
as well, cleaning up all CMDQV resources but it doesn't removing SMMU as
arm_smmu_impl_remove() does.

Add a removing_smmu boolean to skip tegra241_vcmdq_free_smmu_cmdq() when
SMMU driver itself is being removed.

Fixes: 483e0bd8883a ("iommu/tegra241-cmdqv: Do not allocate vcmdq until dma_set_mask_and_coherent")
Cc: stable@vger.kernel.org
Signed-off-by: Nicolin Chen <nicolinc@nvidia.com>
---
 drivers/iommu/arm/arm-smmu-v3/tegra241-cmdqv.c | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

diff --git a/drivers/iommu/arm/arm-smmu-v3/tegra241-cmdqv.c b/drivers/iommu/arm/arm-smmu-v3/tegra241-cmdqv.c
index d525ab43a4ae..ce7be8eeb43c 100644
--- a/drivers/iommu/arm/arm-smmu-v3/tegra241-cmdqv.c
+++ b/drivers/iommu/arm/arm-smmu-v3/tegra241-cmdqv.c
@@ -555,12 +555,15 @@ static int tegra241_vintf_init_lvcmdq(struct tegra241_vintf *vintf, u16 lidx,
 	return 0;
 }
 
-static void tegra241_vintf_free_lvcmdq(struct tegra241_vintf *vintf, u16 lidx)
+static void tegra241_vintf_free_lvcmdq(struct tegra241_vintf *vintf, u16 lidx,
+				       bool removing_smmu)
 {
 	struct tegra241_vcmdq *vcmdq = vintf->lvcmdqs[lidx];
 	char header[64];
 
-	tegra241_vcmdq_free_smmu_cmdq(vcmdq);
+	/* When removing SMMU, the queue memory space will be freed by devres */
+	if (!removing_smmu)
+		tegra241_vcmdq_free_smmu_cmdq(vcmdq);
 	tegra241_vintf_deinit_lvcmdq(vintf, lidx);
 
 	dev_dbg(vintf->cmdqv->dev,
@@ -641,7 +644,7 @@ static int tegra241_cmdqv_init_vintf(struct tegra241_cmdqv *cmdqv, u16 max_idx,
 static void tegra241_vintf_remove_lvcmdq(struct tegra241_vintf *vintf, u16 lidx)
 {
 	tegra241_vcmdq_hw_deinit(vintf->lvcmdqs[lidx]);
-	tegra241_vintf_free_lvcmdq(vintf, lidx);
+	tegra241_vintf_free_lvcmdq(vintf, lidx, true);
 }
 
 static void tegra241_cmdqv_remove_vintf(struct tegra241_cmdqv *cmdqv, u16 idx)
@@ -792,7 +795,7 @@ static int tegra241_cmdqv_init_structures(struct arm_smmu_device *smmu)
 
 free_lvcmdq:
 	for (lidx--; lidx >= 0; lidx--)
-		tegra241_vintf_free_lvcmdq(vintf, lidx);
+		tegra241_vintf_free_lvcmdq(vintf, lidx, false);
 	tegra241_cmdqv_deinit_vintf(cmdqv, vintf->idx);
 free_vintf:
 	kfree(vintf);
-- 
2.43.0

Re: [PATCH rc 1/2] iommu/tegra241-cmdqv: Fix warnings due to dmam_free_coherent()

[Jason Gunthorpe]

On Mon, Apr 07, 2025 at 01:34:59AM -0700, Nicolin Chen wrote:
> Two WARNINGs are observed when SMMU driver rolls back upon failure:
>  arm-smmu-v3.9.auto: Failed to register iommu
>  arm-smmu-v3.9.auto: probe with driver arm-smmu-v3 failed with error -22
>  ------------[ cut here ]------------
>  WARNING: CPU: 5 PID: 1 at kernel/dma/mapping.c:74 dmam_free_coherent+0xc0/0xd8
>  Call trace:
>   dmam_free_coherent+0xc0/0xd8 (P)
>   tegra241_vintf_free_lvcmdq+0x74/0x188
>   tegra241_cmdqv_remove_vintf+0x60/0x148
>   tegra241_cmdqv_remove+0x48/0xc8
>   arm_smmu_impl_remove+0x28/0x60
>   devm_action_release+0x1c/0x40
>  ------------[ cut here ]------------
>  128 pages are still in use!
>  WARNING: CPU: 16 PID: 1 at mm/page_alloc.c:6902 free_contig_range+0x18c/0x1c8
>  Call trace:
>   free_contig_range+0x18c/0x1c8 (P)
>   cma_release+0x154/0x2f0
>   dma_free_contiguous+0x38/0xa0
>   dma_direct_free+0x10c/0x248
>   dma_free_attrs+0x100/0x290
>   dmam_free_coherent+0x78/0xd8
>   tegra241_vintf_free_lvcmdq+0x74/0x160
>   tegra241_cmdqv_remove+0x98/0x198
>   arm_smmu_impl_remove+0x28/0x60
>   devm_action_release+0x1c/0x40
> 
> For the first warning: when the main SMMU driver cleans up its resources,
> any routine in arm_smmu_impl_remove() should not use any devres function.

Bleck. This is situations where you should not be using devres at all.

It is not that arm_smmu_impl_remove() should not use devres, the
problem is that arm_smmu_impl_probe() has mis-ordered the devres
callbacks if ops->device_remove() is going to be manually freeing
things that probe allocated.

IMHO you should just put the goto unwind back into arm_smmu_device()
probe and not use devm for ops->device_remove(). That will put things
in their proper order and no problem.

Because changing arm_smmu_init_one_queue to avoid devm looks worse..

> -static void tegra241_vintf_free_lvcmdq(struct tegra241_vintf *vintf, u16 lidx)
> +static void tegra241_vintf_free_lvcmdq(struct tegra241_vintf *vintf, u16 lidx,
> +				       bool removing_smmu)
>  {

And this is kind of ugly

Jason

Re: [PATCH rc 1/2] iommu/tegra241-cmdqv: Fix warnings due to dmam_free_coherent()

[Nicolin Chen]

On Mon, Apr 07, 2025 at 02:44:08PM -0300, Jason Gunthorpe wrote:
> On Mon, Apr 07, 2025 at 01:34:59AM -0700, Nicolin Chen wrote:
> > Two WARNINGs are observed when SMMU driver rolls back upon failure:
> >  arm-smmu-v3.9.auto: Failed to register iommu
> >  arm-smmu-v3.9.auto: probe with driver arm-smmu-v3 failed with error -22
> >  ------------[ cut here ]------------
> >  WARNING: CPU: 5 PID: 1 at kernel/dma/mapping.c:74 dmam_free_coherent+0xc0/0xd8
> >  Call trace:
> >   dmam_free_coherent+0xc0/0xd8 (P)
> >   tegra241_vintf_free_lvcmdq+0x74/0x188
> >   tegra241_cmdqv_remove_vintf+0x60/0x148
> >   tegra241_cmdqv_remove+0x48/0xc8
> >   arm_smmu_impl_remove+0x28/0x60
> >   devm_action_release+0x1c/0x40
> >  ------------[ cut here ]------------
> >  128 pages are still in use!
> >  WARNING: CPU: 16 PID: 1 at mm/page_alloc.c:6902 free_contig_range+0x18c/0x1c8
> >  Call trace:
> >   free_contig_range+0x18c/0x1c8 (P)
> >   cma_release+0x154/0x2f0
> >   dma_free_contiguous+0x38/0xa0
> >   dma_direct_free+0x10c/0x248
> >   dma_free_attrs+0x100/0x290
> >   dmam_free_coherent+0x78/0xd8
> >   tegra241_vintf_free_lvcmdq+0x74/0x160
> >   tegra241_cmdqv_remove+0x98/0x198
> >   arm_smmu_impl_remove+0x28/0x60
> >   devm_action_release+0x1c/0x40
> > 
> > For the first warning: when the main SMMU driver cleans up its resources,
> > any routine in arm_smmu_impl_remove() should not use any devres function.
> 
> Bleck. This is situations where you should not be using devres at all.
> 
> It is not that arm_smmu_impl_remove() should not use devres, the
> problem is that arm_smmu_impl_probe() has mis-ordered the devres
> callbacks if ops->device_remove() is going to be manually freeing
> things that probe allocated.
> 
> IMHO you should just put the goto unwind back into arm_smmu_device()
> probe and not use devm for ops->device_remove(). That will put things
> in their proper order and no problem.

I did that in my first attempt but it didn't keep the "fallback
to standard SMMU" part as the driver was..

But giving it a second thought, I think this fallback might not
be necessary at all since a structure allocation failure so the
standard SMMU driver will unlikely be able to continue normally.

I think the correct way is to fail init_structures and that will
ask SMMU driver to unwind with smmu->impl_ops->device_remove, as
you suggested here.

Thanks
Nicolin

[PATCH rc 2/2] iommu/tegra241-cmdqv: Fix UAF due to re-entry of tegra241_cmdqv_remove()

[Nicolin Chen]

When falling back to standard SMMU CMDQ, impl_ops must be set to NULL, so
tegra241_cmdqv_remove() will not be re-entered by arm_smmu_impl_remove(),
which will trigger a UAF.

Fixes: 483e0bd8883a ("iommu/tegra241-cmdqv: Do not allocate vcmdq until dma_set_mask_and_coherent")
Cc: stable@vger.kernel.org
Signed-off-by: Nicolin Chen <nicolinc@nvidia.com>
---
 drivers/iommu/arm/arm-smmu-v3/tegra241-cmdqv.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/iommu/arm/arm-smmu-v3/tegra241-cmdqv.c b/drivers/iommu/arm/arm-smmu-v3/tegra241-cmdqv.c
index ce7be8eeb43c..8249ef087af3 100644
--- a/drivers/iommu/arm/arm-smmu-v3/tegra241-cmdqv.c
+++ b/drivers/iommu/arm/arm-smmu-v3/tegra241-cmdqv.c
@@ -803,6 +803,7 @@ static int tegra241_cmdqv_init_structures(struct arm_smmu_device *smmu)
 	dev_info(smmu->impl_dev, "Falling back to standard SMMU CMDQ\n");
 	smmu->options &= ~ARM_SMMU_OPT_TEGRA241_CMDQV;
 	tegra241_cmdqv_remove(smmu);
+	smmu->impl_ops = NULL;
 	return 0;
 }
 
-- 
2.43.0

Re: [PATCH rc 2/2] iommu/tegra241-cmdqv: Fix UAF due to re-entry of tegra241_cmdqv_remove()

[Jason Gunthorpe]

On Mon, Apr 07, 2025 at 01:35:00AM -0700, Nicolin Chen wrote:
> When falling back to standard SMMU CMDQ, impl_ops must be set to NULL, so
> tegra241_cmdqv_remove() will not be re-entered by arm_smmu_impl_remove(),
> which will trigger a UAF.
> 
> Fixes: 483e0bd8883a ("iommu/tegra241-cmdqv: Do not allocate vcmdq until dma_set_mask_and_coherent")
> Cc: stable@vger.kernel.org
> Signed-off-by: Nicolin Chen <nicolinc@nvidia.com>
> ---
>  drivers/iommu/arm/arm-smmu-v3/tegra241-cmdqv.c | 1 +
>  1 file changed, 1 insertion(+)

Reviewed-by: Jason Gunthorpe <jgg@nvidia.com>

Jason

Re: [PATCH rc 2/2] iommu/tegra241-cmdqv: Fix UAF due to re-entry of tegra241_cmdqv_remove()

[Nicolin Chen]

On Mon, Apr 07, 2025 at 01:35:00AM -0700, Nicolin Chen wrote:
> When falling back to standard SMMU CMDQ, impl_ops must be set to NULL, so
> tegra241_cmdqv_remove() will not be re-entered by arm_smmu_impl_remove(),
> which will trigger a UAF.
> 
> Fixes: 483e0bd8883a ("iommu/tegra241-cmdqv: Do not allocate vcmdq until dma_set_mask_and_coherent")
> Cc: stable@vger.kernel.org
> Signed-off-by: Nicolin Chen <nicolinc@nvidia.com>
> ---
>  drivers/iommu/arm/arm-smmu-v3/tegra241-cmdqv.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/iommu/arm/arm-smmu-v3/tegra241-cmdqv.c b/drivers/iommu/arm/arm-smmu-v3/tegra241-cmdqv.c
> index ce7be8eeb43c..8249ef087af3 100644
> --- a/drivers/iommu/arm/arm-smmu-v3/tegra241-cmdqv.c
> +++ b/drivers/iommu/arm/arm-smmu-v3/tegra241-cmdqv.c
> @@ -803,6 +803,7 @@ static int tegra241_cmdqv_init_structures(struct arm_smmu_device *smmu)
>  	dev_info(smmu->impl_dev, "Falling back to standard SMMU CMDQ\n");
>  	smmu->options &= ~ARM_SMMU_OPT_TEGRA241_CMDQV;
>  	tegra241_cmdqv_remove(smmu);
> +	smmu->impl_ops = NULL;
>  	return 0;

Per discussion in the other patch, with a v2 this whole rewind
part would be dropped to ask SMMU driver to fail and then call 
tegra241_cmdqv_remove() via smmu->impl_ops->device_remove().

So we wouldn't need this patch after all.

Thanks
Nicolin