[PATCH 1/4] media: davinci: vpif: fix unbalanced runtime PM get

[PATCH 1/4] media: davinci: vpif: fix unbalanced runtime PM get

[Johan Hovold]

Make sure to balance the runtime PM usage counter on driver unbind.

Fixes: 407ccc65bfd2 ("[media] davinci: vpif: add pm_runtime support")
Cc: stable@vger.kernel.org      # 3.9
Cc: Lad, Prabhakar <prabhakar.csengg@gmail.com>
Signed-off-by: Johan Hovold <johan@kernel.org>
---
 drivers/media/platform/davinci/vpif.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/media/platform/davinci/vpif.c b/drivers/media/platform/davinci/vpif.c
index 5a89d885d0e3..9752a5ec36f7 100644
--- a/drivers/media/platform/davinci/vpif.c
+++ b/drivers/media/platform/davinci/vpif.c
@@ -495,6 +495,7 @@ static int vpif_probe(struct platform_device *pdev)
 
 static int vpif_remove(struct platform_device *pdev)
 {
+	pm_runtime_put(&pdev->dev);
 	pm_runtime_disable(&pdev->dev);
 	return 0;
 }
-- 
2.32.0

Re: [PATCH 1/4] media: davinci: vpif: fix unbalanced runtime PM get

[Lad, Prabhakar]

Hi Johan,

Thank you for the patch.

On Wed, Dec 22, 2021 at 2:20 PM Johan Hovold <johan@kernel.org> wrote:
>
> Make sure to balance the runtime PM usage counter on driver unbind.
>
> Fixes: 407ccc65bfd2 ("[media] davinci: vpif: add pm_runtime support")
> Cc: stable@vger.kernel.org      # 3.9
> Cc: Lad, Prabhakar <prabhakar.csengg@gmail.com>
> Signed-off-by: Johan Hovold <johan@kernel.org>
> ---
>  drivers/media/platform/davinci/vpif.c | 1 +
>  1 file changed, 1 insertion(+)
>
Reviewed-by: Lad Prabhakar <prabhakar.csengg@gmail.com>

> diff --git a/drivers/media/platform/davinci/vpif.c b/drivers/media/platform/davinci/vpif.c
> index 5a89d885d0e3..9752a5ec36f7 100644
> --- a/drivers/media/platform/davinci/vpif.c
> +++ b/drivers/media/platform/davinci/vpif.c
> @@ -495,6 +495,7 @@ static int vpif_probe(struct platform_device *pdev)
>
>  static int vpif_remove(struct platform_device *pdev)
>  {
> +       pm_runtime_put(&pdev->dev);
>         pm_runtime_disable(&pdev->dev);
>         return 0;
>  }
> --
> 2.32.0
>

Cheers,
Prabhakar

[PATCH 2/4] media: davinci: vpif: fix unbalanced runtime PM enable

[Johan Hovold]

Make sure to disable runtime PM before returning on probe errors.

Fixes: 479f7a118105 ("[media] davinci: vpif: adaptions for DT support")
Cc: stable@vger.kernel.org      # 4.12: 4024d6f601e3c
Cc: Kevin Hilman <khilman@baylibre.com>
Signed-off-by: Johan Hovold <johan@kernel.org>
---
 drivers/media/platform/davinci/vpif.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/drivers/media/platform/davinci/vpif.c b/drivers/media/platform/davinci/vpif.c
index 9752a5ec36f7..1f5eacf48580 100644
--- a/drivers/media/platform/davinci/vpif.c
+++ b/drivers/media/platform/davinci/vpif.c
@@ -428,6 +428,7 @@ static int vpif_probe(struct platform_device *pdev)
 	static struct resource *res_irq;
 	struct platform_device *pdev_capture, *pdev_display;
 	struct device_node *endpoint = NULL;
+	int ret;
 
 	vpif_base = devm_platform_ioremap_resource(pdev, 0);
 	if (IS_ERR(vpif_base))
@@ -456,8 +457,8 @@ static int vpif_probe(struct platform_device *pdev)
 	res_irq = platform_get_resource(pdev, IORESOURCE_IRQ, 0);
 	if (!res_irq) {
 		dev_warn(&pdev->dev, "Missing IRQ resource.\n");
-		pm_runtime_put(&pdev->dev);
-		return -EINVAL;
+		ret = -EINVAL;
+		goto err_put_rpm;
 	}
 
 	pdev_capture = devm_kzalloc(&pdev->dev, sizeof(*pdev_capture),
@@ -491,6 +492,12 @@ static int vpif_probe(struct platform_device *pdev)
 	}
 
 	return 0;
+
+err_put_rpm:
+	pm_runtime_put(&pdev->dev);
+	pm_runtime_disable(&pdev->dev);
+
+	return ret;
 }
 
 static int vpif_remove(struct platform_device *pdev)
-- 
2.32.0

Re: [PATCH 2/4] media: davinci: vpif: fix unbalanced runtime PM enable

[Lad, Prabhakar]

Hi Johan,

Thank you for the patch.

On Wed, Dec 22, 2021 at 2:20 PM Johan Hovold <johan@kernel.org> wrote:
>
> Make sure to disable runtime PM before returning on probe errors.
>
> Fixes: 479f7a118105 ("[media] davinci: vpif: adaptions for DT support")
> Cc: stable@vger.kernel.org      # 4.12: 4024d6f601e3c
> Cc: Kevin Hilman <khilman@baylibre.com>
> Signed-off-by: Johan Hovold <johan@kernel.org>
> ---
>  drivers/media/platform/davinci/vpif.c | 11 +++++++++--
>  1 file changed, 9 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/media/platform/davinci/vpif.c b/drivers/media/platform/davinci/vpif.c
> index 9752a5ec36f7..1f5eacf48580 100644
> --- a/drivers/media/platform/davinci/vpif.c
> +++ b/drivers/media/platform/davinci/vpif.c
> @@ -428,6 +428,7 @@ static int vpif_probe(struct platform_device *pdev)
>         static struct resource *res_irq;
>         struct platform_device *pdev_capture, *pdev_display;
>         struct device_node *endpoint = NULL;
> +       int ret;
>
>         vpif_base = devm_platform_ioremap_resource(pdev, 0);
>         if (IS_ERR(vpif_base))
> @@ -456,8 +457,8 @@ static int vpif_probe(struct platform_device *pdev)
>         res_irq = platform_get_resource(pdev, IORESOURCE_IRQ, 0);
>         if (!res_irq) {
>                 dev_warn(&pdev->dev, "Missing IRQ resource.\n");
> -               pm_runtime_put(&pdev->dev);
Maybe just add pm_runtime_disable(&pdev->dev); here, rest diff won't
be required.

Cheers,
Prabhakar

> -               return -EINVAL;
> +               ret = -EINVAL;
> +               goto err_put_rpm;
>         }
>
>         pdev_capture = devm_kzalloc(&pdev->dev, sizeof(*pdev_capture),
> @@ -491,6 +492,12 @@ static int vpif_probe(struct platform_device *pdev)
>         }
>
>         return 0;
> +
> +err_put_rpm:
> +       pm_runtime_put(&pdev->dev);
> +       pm_runtime_disable(&pdev->dev);
> +
> +       return ret;
>  }
>
>  static int vpif_remove(struct platform_device *pdev)
> --
> 2.32.0
>

Re: [PATCH 2/4] media: davinci: vpif: fix unbalanced runtime PM enable

[Johan Hovold]

Hi Lad,

and sorry about the late reply.

On Tue, Jan 04, 2022 at 06:11:08PM +0000, Lad, Prabhakar wrote:
> Hi Johan,
> 
> Thank you for the patch.
> 
> On Wed, Dec 22, 2021 at 2:20 PM Johan Hovold <johan@kernel.org> wrote:
> >
> > Make sure to disable runtime PM before returning on probe errors.
> >
> > Fixes: 479f7a118105 ("[media] davinci: vpif: adaptions for DT support")
> > Cc: stable@vger.kernel.org      # 4.12: 4024d6f601e3c
> > Cc: Kevin Hilman <khilman@baylibre.com>
> > Signed-off-by: Johan Hovold <johan@kernel.org>
> > ---
> >  drivers/media/platform/davinci/vpif.c | 11 +++++++++--
> >  1 file changed, 9 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/media/platform/davinci/vpif.c b/drivers/media/platform/davinci/vpif.c
> > index 9752a5ec36f7..1f5eacf48580 100644
> > --- a/drivers/media/platform/davinci/vpif.c
> > +++ b/drivers/media/platform/davinci/vpif.c
> > @@ -428,6 +428,7 @@ static int vpif_probe(struct platform_device *pdev)
> >         static struct resource *res_irq;
> >         struct platform_device *pdev_capture, *pdev_display;
> >         struct device_node *endpoint = NULL;
> > +       int ret;
> >
> >         vpif_base = devm_platform_ioremap_resource(pdev, 0);
> >         if (IS_ERR(vpif_base))
> > @@ -456,8 +457,8 @@ static int vpif_probe(struct platform_device *pdev)
> >         res_irq = platform_get_resource(pdev, IORESOURCE_IRQ, 0);
> >         if (!res_irq) {
> >                 dev_warn(&pdev->dev, "Missing IRQ resource.\n");
> > -               pm_runtime_put(&pdev->dev);
> Maybe just add pm_runtime_disable(&pdev->dev); here, rest diff won't
> be required.

I chose to do it this way in order to make the following patches
smaller and easier to review.

Thanks for reviewing.

Johan

[PATCH 3/4] media: davinci: vpif: fix use-after-free on driver unbind

[Johan Hovold]

The driver allocates and registers two platform device structures during
probe, but the devices were never deregistered on driver unbind.

This results in a use-after-free on driver unbind as the device
structures were allocated using devres and would be freed by driver
core when remove() returns.

Fix this by adding the missing deregistration calls to the remove()
callback and failing probe on registration errors.

Note that the platform device structures must be freed using a proper
release callback to avoid leaking associated resources like device
names.

Fixes: 479f7a118105 ("[media] davinci: vpif: adaptions for DT support")
Cc: stable@vger.kernel.org      # 4.12
Cc: Kevin Hilman <khilman@baylibre.com>
Signed-off-by: Johan Hovold <johan@kernel.org>
---
 drivers/media/platform/davinci/vpif.c | 97 ++++++++++++++++++++-------
 1 file changed, 71 insertions(+), 26 deletions(-)

diff --git a/drivers/media/platform/davinci/vpif.c b/drivers/media/platform/davinci/vpif.c
index 1f5eacf48580..4a260f4ed236 100644
--- a/drivers/media/platform/davinci/vpif.c
+++ b/drivers/media/platform/davinci/vpif.c
@@ -41,6 +41,11 @@ MODULE_ALIAS("platform:" VPIF_DRIVER_NAME);
 #define VPIF_CH2_MAX_MODES	15
 #define VPIF_CH3_MAX_MODES	2
 
+struct vpif_data {
+	struct platform_device *capture;
+	struct platform_device *display;
+};
+
 DEFINE_SPINLOCK(vpif_lock);
 EXPORT_SYMBOL_GPL(vpif_lock);
 
@@ -423,17 +428,31 @@ int vpif_channel_getfid(u8 channel_id)
 }
 EXPORT_SYMBOL(vpif_channel_getfid);
 
+static void vpif_pdev_release(struct device *dev)
+{
+	struct platform_device *pdev = to_platform_device(dev);
+
+	kfree(pdev);
+}
+
 static int vpif_probe(struct platform_device *pdev)
 {
 	static struct resource *res_irq;
 	struct platform_device *pdev_capture, *pdev_display;
 	struct device_node *endpoint = NULL;
+	struct vpif_data *data;
 	int ret;
 
 	vpif_base = devm_platform_ioremap_resource(pdev, 0);
 	if (IS_ERR(vpif_base))
 		return PTR_ERR(vpif_base);
 
+	data = kzalloc(sizeof(*data), GFP_KERNEL);
+	if (!data)
+		return -ENOMEM;
+
+	platform_set_drvdata(pdev, data);
+
 	pm_runtime_enable(&pdev->dev);
 	pm_runtime_get(&pdev->dev);
 
@@ -461,49 +480,75 @@ static int vpif_probe(struct platform_device *pdev)
 		goto err_put_rpm;
 	}
 
-	pdev_capture = devm_kzalloc(&pdev->dev, sizeof(*pdev_capture),
-				    GFP_KERNEL);
-	if (pdev_capture) {
-		pdev_capture->name = "vpif_capture";
-		pdev_capture->id = -1;
-		pdev_capture->resource = res_irq;
-		pdev_capture->num_resources = 1;
-		pdev_capture->dev.dma_mask = pdev->dev.dma_mask;
-		pdev_capture->dev.coherent_dma_mask = pdev->dev.coherent_dma_mask;
-		pdev_capture->dev.parent = &pdev->dev;
-		platform_device_register(pdev_capture);
-	} else {
-		dev_warn(&pdev->dev, "Unable to allocate memory for pdev_capture.\n");
+	pdev_capture = kzalloc(sizeof(*pdev_capture), GFP_KERNEL);
+	if (!pdev_capture) {
+		ret = -ENOMEM;
+		goto err_put_rpm;
 	}
 
-	pdev_display = devm_kzalloc(&pdev->dev, sizeof(*pdev_display),
-				    GFP_KERNEL);
-	if (pdev_display) {
-		pdev_display->name = "vpif_display";
-		pdev_display->id = -1;
-		pdev_display->resource = res_irq;
-		pdev_display->num_resources = 1;
-		pdev_display->dev.dma_mask = pdev->dev.dma_mask;
-		pdev_display->dev.coherent_dma_mask = pdev->dev.coherent_dma_mask;
-		pdev_display->dev.parent = &pdev->dev;
-		platform_device_register(pdev_display);
-	} else {
-		dev_warn(&pdev->dev, "Unable to allocate memory for pdev_display.\n");
+	pdev_capture->name = "vpif_capture";
+	pdev_capture->id = -1;
+	pdev_capture->resource = res_irq;
+	pdev_capture->num_resources = 1;
+	pdev_capture->dev.dma_mask = pdev->dev.dma_mask;
+	pdev_capture->dev.coherent_dma_mask = pdev->dev.coherent_dma_mask;
+	pdev_capture->dev.parent = &pdev->dev;
+	pdev_capture->dev.release = vpif_pdev_release;
+
+	ret = platform_device_register(pdev_capture);
+	if (ret)
+		goto err_put_pdev_capture;
+
+	pdev_display = kzalloc(sizeof(*pdev_display), GFP_KERNEL);
+	if (!pdev_display) {
+		ret = -ENOMEM;
+		goto err_put_pdev_capture;
 	}
 
+	pdev_display->name = "vpif_display";
+	pdev_display->id = -1;
+	pdev_display->resource = res_irq;
+	pdev_display->num_resources = 1;
+	pdev_display->dev.dma_mask = pdev->dev.dma_mask;
+	pdev_display->dev.coherent_dma_mask = pdev->dev.coherent_dma_mask;
+	pdev_display->dev.parent = &pdev->dev;
+	pdev_display->dev.release = vpif_pdev_release;
+
+	ret = platform_device_register(pdev_display);
+	if (ret)
+		goto err_put_pdev_display;
+
+	data->capture = pdev_capture;
+	data->display = pdev_display;
+
 	return 0;
 
+err_put_pdev_display:
+	platform_device_put(pdev_display);
+err_put_pdev_capture:
+	platform_device_put(pdev_capture);
 err_put_rpm:
 	pm_runtime_put(&pdev->dev);
 	pm_runtime_disable(&pdev->dev);
+	kfree(data);
 
 	return ret;
 }
 
 static int vpif_remove(struct platform_device *pdev)
 {
+	struct vpif_data *data = platform_get_drvdata(pdev);
+
+	if (data->capture)
+		platform_device_unregister(data->capture);
+	if (data->display)
+		platform_device_unregister(data->display);
+
 	pm_runtime_put(&pdev->dev);
 	pm_runtime_disable(&pdev->dev);
+
+	kfree(data);
+
 	return 0;
 }
 
-- 
2.32.0

Re: [PATCH 3/4] media: davinci: vpif: fix use-after-free on driver unbind

[Lad, Prabhakar]

Hi Johan,

Thank you for the patch.

On Wed, Dec 22, 2021 at 2:20 PM Johan Hovold <johan@kernel.org> wrote:
>
> The driver allocates and registers two platform device structures during
> probe, but the devices were never deregistered on driver unbind.
>
> This results in a use-after-free on driver unbind as the device
> structures were allocated using devres and would be freed by driver
> core when remove() returns.
>
> Fix this by adding the missing deregistration calls to the remove()
> callback and failing probe on registration errors.
>
> Note that the platform device structures must be freed using a proper
> release callback to avoid leaking associated resources like device
> names.
>
> Fixes: 479f7a118105 ("[media] davinci: vpif: adaptions for DT support")
> Cc: stable@vger.kernel.org      # 4.12
> Cc: Kevin Hilman <khilman@baylibre.com>
> Signed-off-by: Johan Hovold <johan@kernel.org>
> ---
>  drivers/media/platform/davinci/vpif.c | 97 ++++++++++++++++++++-------
>  1 file changed, 71 insertions(+), 26 deletions(-)
>
Reviewed-by: Lad Prabhakar <prabhakar.csengg@gmail.com>

Cheers,
Prabhakar

> diff --git a/drivers/media/platform/davinci/vpif.c b/drivers/media/platform/davinci/vpif.c
> index 1f5eacf48580..4a260f4ed236 100644
> --- a/drivers/media/platform/davinci/vpif.c
> +++ b/drivers/media/platform/davinci/vpif.c
> @@ -41,6 +41,11 @@ MODULE_ALIAS("platform:" VPIF_DRIVER_NAME);
>  #define VPIF_CH2_MAX_MODES     15
>  #define VPIF_CH3_MAX_MODES     2
>
> +struct vpif_data {
> +       struct platform_device *capture;
> +       struct platform_device *display;
> +};
> +
>  DEFINE_SPINLOCK(vpif_lock);
>  EXPORT_SYMBOL_GPL(vpif_lock);
>
> @@ -423,17 +428,31 @@ int vpif_channel_getfid(u8 channel_id)
>  }
>  EXPORT_SYMBOL(vpif_channel_getfid);
>
> +static void vpif_pdev_release(struct device *dev)
> +{
> +       struct platform_device *pdev = to_platform_device(dev);
> +
> +       kfree(pdev);
> +}
> +
>  static int vpif_probe(struct platform_device *pdev)
>  {
>         static struct resource *res_irq;
>         struct platform_device *pdev_capture, *pdev_display;
>         struct device_node *endpoint = NULL;
> +       struct vpif_data *data;
>         int ret;
>
>         vpif_base = devm_platform_ioremap_resource(pdev, 0);
>         if (IS_ERR(vpif_base))
>                 return PTR_ERR(vpif_base);
>
> +       data = kzalloc(sizeof(*data), GFP_KERNEL);
> +       if (!data)
> +               return -ENOMEM;
> +
> +       platform_set_drvdata(pdev, data);
> +
>         pm_runtime_enable(&pdev->dev);
>         pm_runtime_get(&pdev->dev);
>
> @@ -461,49 +480,75 @@ static int vpif_probe(struct platform_device *pdev)
>                 goto err_put_rpm;
>         }
>
> -       pdev_capture = devm_kzalloc(&pdev->dev, sizeof(*pdev_capture),
> -                                   GFP_KERNEL);
> -       if (pdev_capture) {
> -               pdev_capture->name = "vpif_capture";
> -               pdev_capture->id = -1;
> -               pdev_capture->resource = res_irq;
> -               pdev_capture->num_resources = 1;
> -               pdev_capture->dev.dma_mask = pdev->dev.dma_mask;
> -               pdev_capture->dev.coherent_dma_mask = pdev->dev.coherent_dma_mask;
> -               pdev_capture->dev.parent = &pdev->dev;
> -               platform_device_register(pdev_capture);
> -       } else {
> -               dev_warn(&pdev->dev, "Unable to allocate memory for pdev_capture.\n");
> +       pdev_capture = kzalloc(sizeof(*pdev_capture), GFP_KERNEL);
> +       if (!pdev_capture) {
> +               ret = -ENOMEM;
> +               goto err_put_rpm;
>         }
>
> -       pdev_display = devm_kzalloc(&pdev->dev, sizeof(*pdev_display),
> -                                   GFP_KERNEL);
> -       if (pdev_display) {
> -               pdev_display->name = "vpif_display";
> -               pdev_display->id = -1;
> -               pdev_display->resource = res_irq;
> -               pdev_display->num_resources = 1;
> -               pdev_display->dev.dma_mask = pdev->dev.dma_mask;
> -               pdev_display->dev.coherent_dma_mask = pdev->dev.coherent_dma_mask;
> -               pdev_display->dev.parent = &pdev->dev;
> -               platform_device_register(pdev_display);
> -       } else {
> -               dev_warn(&pdev->dev, "Unable to allocate memory for pdev_display.\n");
> +       pdev_capture->name = "vpif_capture";
> +       pdev_capture->id = -1;
> +       pdev_capture->resource = res_irq;
> +       pdev_capture->num_resources = 1;
> +       pdev_capture->dev.dma_mask = pdev->dev.dma_mask;
> +       pdev_capture->dev.coherent_dma_mask = pdev->dev.coherent_dma_mask;
> +       pdev_capture->dev.parent = &pdev->dev;
> +       pdev_capture->dev.release = vpif_pdev_release;
> +
> +       ret = platform_device_register(pdev_capture);
> +       if (ret)
> +               goto err_put_pdev_capture;
> +
> +       pdev_display = kzalloc(sizeof(*pdev_display), GFP_KERNEL);
> +       if (!pdev_display) {
> +               ret = -ENOMEM;
> +               goto err_put_pdev_capture;
>         }
>
> +       pdev_display->name = "vpif_display";
> +       pdev_display->id = -1;
> +       pdev_display->resource = res_irq;
> +       pdev_display->num_resources = 1;
> +       pdev_display->dev.dma_mask = pdev->dev.dma_mask;
> +       pdev_display->dev.coherent_dma_mask = pdev->dev.coherent_dma_mask;
> +       pdev_display->dev.parent = &pdev->dev;
> +       pdev_display->dev.release = vpif_pdev_release;
> +
> +       ret = platform_device_register(pdev_display);
> +       if (ret)
> +               goto err_put_pdev_display;
> +
> +       data->capture = pdev_capture;
> +       data->display = pdev_display;
> +
>         return 0;
>
> +err_put_pdev_display:
> +       platform_device_put(pdev_display);
> +err_put_pdev_capture:
> +       platform_device_put(pdev_capture);
>  err_put_rpm:
>         pm_runtime_put(&pdev->dev);
>         pm_runtime_disable(&pdev->dev);
> +       kfree(data);
>
>         return ret;
>  }
>
>  static int vpif_remove(struct platform_device *pdev)
>  {
> +       struct vpif_data *data = platform_get_drvdata(pdev);
> +
> +       if (data->capture)
> +               platform_device_unregister(data->capture);
> +       if (data->display)
> +               platform_device_unregister(data->display);
> +
>         pm_runtime_put(&pdev->dev);
>         pm_runtime_disable(&pdev->dev);
> +
> +       kfree(data);
> +
>         return 0;
>  }
>
> --
> 2.32.0
>