[PATCH v2 06/14] iommu/mediatek: fix device leaks on probe()

Johan Hovold posted 14 patches 4 months, 1 week ago
There is a newer version of this series
[PATCH v2 06/14] iommu/mediatek: fix device leaks on probe()
Posted by Johan Hovold 4 months, 1 week ago
Make sure to drop the references taken to the larb devices during
probe on probe failure (e.g. probe deferral) and on driver unbind.

Note that commit 26593928564c ("iommu/mediatek: Add error path for loop
of mm_dts_parse") fixed the leaks in a couple of error paths, but the
references are still leaking on success and late failures.

Fixes: 0df4fabe208d ("iommu/mediatek: Add mt8173 IOMMU driver")
Cc: stable@vger.kernel.org	# 4.6
Cc: Yong Wu <yong.wu@mediatek.com>
Acked-by: Robin Murphy <robin.murphy@arm.com>
Signed-off-by: Johan Hovold <johan@kernel.org>
---
 drivers/iommu/mtk_iommu.c | 24 ++++++++++++++++++------
 1 file changed, 18 insertions(+), 6 deletions(-)

diff --git a/drivers/iommu/mtk_iommu.c b/drivers/iommu/mtk_iommu.c
index 8d8e85186188..20a5ba80f983 100644
--- a/drivers/iommu/mtk_iommu.c
+++ b/drivers/iommu/mtk_iommu.c
@@ -1216,13 +1216,17 @@ static int mtk_iommu_mm_dts_parse(struct device *dev, struct component_match **m
 		platform_device_put(plarbdev);
 	}
 
-	if (!frst_avail_smicomm_node)
-		return -EINVAL;
+	if (!frst_avail_smicomm_node) {
+		ret = -EINVAL;
+		goto err_larbdev_put;
+	}
 
 	pcommdev = of_find_device_by_node(frst_avail_smicomm_node);
 	of_node_put(frst_avail_smicomm_node);
-	if (!pcommdev)
-		return -ENODEV;
+	if (!pcommdev) {
+		ret = -ENODEV;
+		goto err_larbdev_put;
+	}
 	data->smicomm_dev = &pcommdev->dev;
 
 	link = device_link_add(data->smicomm_dev, dev,
@@ -1230,7 +1234,8 @@ static int mtk_iommu_mm_dts_parse(struct device *dev, struct component_match **m
 	platform_device_put(pcommdev);
 	if (!link) {
 		dev_err(dev, "Unable to link %s.\n", dev_name(data->smicomm_dev));
-		return -EINVAL;
+		ret = -EINVAL;
+		goto err_larbdev_put;
 	}
 	return 0;
 
@@ -1402,8 +1407,12 @@ static int mtk_iommu_probe(struct platform_device *pdev)
 	iommu_device_sysfs_remove(&data->iommu);
 out_list_del:
 	list_del(&data->list);
-	if (MTK_IOMMU_IS_TYPE(data->plat_data, MTK_IOMMU_TYPE_MM))
+	if (MTK_IOMMU_IS_TYPE(data->plat_data, MTK_IOMMU_TYPE_MM)) {
 		device_link_remove(data->smicomm_dev, dev);
+
+		for (i = 0; i < MTK_LARB_NR_MAX; i++)
+			put_device(data->larb_imu[i].dev);
+	}
 out_runtime_disable:
 	pm_runtime_disable(dev);
 	return ret;
@@ -1423,6 +1432,9 @@ static void mtk_iommu_remove(struct platform_device *pdev)
 	if (MTK_IOMMU_IS_TYPE(data->plat_data, MTK_IOMMU_TYPE_MM)) {
 		device_link_remove(data->smicomm_dev, &pdev->dev);
 		component_master_del(&pdev->dev, &mtk_iommu_com_ops);
+
+		for (i = 0; i < MTK_LARB_NR_MAX; i++)
+			put_device(data->larb_imu[i].dev);
 	}
 	pm_runtime_disable(&pdev->dev);
 	for (i = 0; i < data->plat_data->banks_num; i++) {
-- 
2.49.1
Re: [PATCH v2 06/14] iommu/mediatek: fix device leaks on probe()
Posted by Yong Wu (吴勇) 3 months, 3 weeks ago
On Tue, 2025-10-07 at 11:43 +0200, Johan Hovold wrote:
> External email : Please do not click links or open attachments until
> you have verified the sender or the content.
> 
> 
> Make sure to drop the references taken to the larb devices during
> probe on probe failure (e.g. probe deferral) and on driver unbind.
> 
> Note that commit 26593928564c ("iommu/mediatek: Add error path for
> loop
> of mm_dts_parse") fixed the leaks in a couple of error paths, but the
> references are still leaking on success and late failures.
> 
> Fixes: 0df4fabe208d ("iommu/mediatek: Add mt8173 IOMMU driver")
> Cc: stable@vger.kernel.org      # 4.6
> Cc: Yong Wu <yong.wu@mediatek.com>
> Acked-by: Robin Murphy <robin.murphy@arm.com>
> Signed-off-by: Johan Hovold <johan@kernel.org>
> ---
>  drivers/iommu/mtk_iommu.c | 24 ++++++++++++++++++------
>  1 file changed, 18 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/iommu/mtk_iommu.c b/drivers/iommu/mtk_iommu.c
> index 8d8e85186188..20a5ba80f983 100644
> --- a/drivers/iommu/mtk_iommu.c
> +++ b/drivers/iommu/mtk_iommu.c
> @@ -1216,13 +1216,17 @@ static int mtk_iommu_mm_dts_parse(struct
> device *dev, struct component_match **m
>                 platform_device_put(plarbdev);
>         }
> 
> -       if (!frst_avail_smicomm_node)
> -               return -EINVAL;
> +       if (!frst_avail_smicomm_node) {
> +               ret = -EINVAL;
> +               goto err_larbdev_put;

There already is a "platform_device_put(plarbdev);" at the end of "for"
loop, then no need put_device for it outside the "for" loop or outside
this function?

Thanks.

> +       }
> 
>         pcommdev = of_find_device_by_node(frst_avail_smicomm_node);
>         of_node_put(frst_avail_smicomm_node);
> -       if (!pcommdev)
> -               return -ENODEV;
> +       if (!pcommdev) {
> +               ret = -ENODEV;
> +               goto err_larbdev_put;
> +       }
>         data->smicomm_dev = &pcommdev->dev;
> 
>         link = device_link_add(data->smicomm_dev, dev,
> @@ -1230,7 +1234,8 @@ static int mtk_iommu_mm_dts_parse(struct device
> *dev, struct component_match **m
>         platform_device_put(pcommdev);
>         if (!link) {
>                 dev_err(dev, "Unable to link %s.\n", dev_name(data-
> >smicomm_dev));
> -               return -EINVAL;
> +               ret = -EINVAL;
> +               goto err_larbdev_put;
>         }
>         return 0;
> 
> @@ -1402,8 +1407,12 @@ static int mtk_iommu_probe(struct
> platform_device *pdev)
>         iommu_device_sysfs_remove(&data->iommu);
>  out_list_del:
>         list_del(&data->list);
> -       if (MTK_IOMMU_IS_TYPE(data->plat_data, MTK_IOMMU_TYPE_MM))
> +       if (MTK_IOMMU_IS_TYPE(data->plat_data, MTK_IOMMU_TYPE_MM)) {
>                 device_link_remove(data->smicomm_dev, dev);
> +
> +               for (i = 0; i < MTK_LARB_NR_MAX; i++)
> +                       put_device(data->larb_imu[i].dev);
> +       }
>  out_runtime_disable:
>         pm_runtime_disable(dev);
>         return ret;
> @@ -1423,6 +1432,9 @@ static void mtk_iommu_remove(struct
> platform_device *pdev)
>         if (MTK_IOMMU_IS_TYPE(data->plat_data, MTK_IOMMU_TYPE_MM)) {
>                 device_link_remove(data->smicomm_dev, &pdev->dev);
>                 component_master_del(&pdev->dev, &mtk_iommu_com_ops);
> +
> +               for (i = 0; i < MTK_LARB_NR_MAX; i++)
> +                       put_device(data->larb_imu[i].dev);
>         }
>         pm_runtime_disable(&pdev->dev);
>         for (i = 0; i < data->plat_data->banks_num; i++) {
> --
> 2.49.1
> 
Re: [PATCH v2 06/14] iommu/mediatek: fix device leaks on probe()
Posted by Johan Hovold 3 months, 3 weeks ago
On Sat, Oct 18, 2025 at 06:54:39AM +0000, Yong Wu (吴勇) wrote:
> On Tue, 2025-10-07 at 11:43 +0200, Johan Hovold wrote:

> > Make sure to drop the references taken to the larb devices during
> > probe on probe failure (e.g. probe deferral) and on driver unbind.
> > 
> > Note that commit 26593928564c ("iommu/mediatek: Add error path for
> > loop
> > of mm_dts_parse") fixed the leaks in a couple of error paths, but the
> > references are still leaking on success and late failures.

> > @@ -1216,13 +1216,17 @@ static int mtk_iommu_mm_dts_parse(struct
> > device *dev, struct component_match **m
> >                 platform_device_put(plarbdev);
> >         }
> > 
> > -       if (!frst_avail_smicomm_node)
> > -               return -EINVAL;
> > +       if (!frst_avail_smicomm_node) {
> > +               ret = -EINVAL;
> > +               goto err_larbdev_put;
> 
> There already is a "platform_device_put(plarbdev);" at the end of "for"
> loop, then no need put_device for it outside the "for" loop or outside
> this function?

You're right, thanks for catching that.

But this means that we have an existing potential use-after-free as if,
for example, the driver probe defers we would put the reference to any
previously looked up larbs twice.

I've just sent a v3 which fixes this by dropping the
platform_device_put() after successful lookup as it is expected that the
driver keeps the references while it uses the larb devices:

	https://lore.kernel.org/lkml/20251020045318.30690-1-johan@kernel.org/

Johan