MIME-Version: 1.0
References: <20211222142025.30364-1-johan@kernel.org> <20211222142025.30364-4-johan@kernel.org>
In-Reply-To: <20211222142025.30364-4-johan@kernel.org>
From:   "Lad, Prabhakar" <prabhakar.csengg@gmail.com>
Date:   Tue, 4 Jan 2022 18:30:23 +0000
Message-ID: <CA+V-a8vGkHnGABm=AiNjkvp3S-JU8qkqQZoX=-ZkK4njUSAv8w@mail.gmail.com>
Subject: Re: [PATCH 3/4] media: davinci: vpif: fix use-after-free on driver unbind
To:     Johan Hovold <johan@kernel.org>
Cc:     Mauro Carvalho Chehab <mchehab@kernel.org>,
        Hans Verkuil <hans.verkuil@cisco.com>,
        Kevin Hilman <khilman@baylibre.com>,
        Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        linux-media <linux-media@vger.kernel.org>,
        LKML <linux-kernel@vger.kernel.org>, stable@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

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
>