Hi Johan,
Thank you for the patch.
On Wed, Dec 22, 2021 at 2:20 PM Johan Hovold <johan@xxxxxxxxxx> 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@xxxxxxxxxxxxxxx # 4.12
> Cc: Kevin Hilman <khilman@xxxxxxxxxxxx>
> Signed-off-by: Johan Hovold <johan@xxxxxxxxxx>
> ---
> drivers/media/platform/davinci/vpif.c | 97 ++++++++++++++++++++-------
> 1 file changed, 71 insertions(+), 26 deletions(-)
>
Reviewed-by: Lad Prabhakar <prabhakar.csengg@xxxxxxxxx>
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
>
