Re: [PATCH 12/14] drm/kmb: Fix possible oops in error handling

[Dan Carpenter <dan.carpenter@oracle.com>]

On Tue, Jul 27, 2021 at 05:31:24PM -0700, Anitha Chrisanthus wrote:
> If kmb_dsi_init() fails the "kmb->kmb_dsi" variable is an error pointer.
> This can potentially result in kernel panic when kmb_dsi_host_unregister is
> called.
> 
> Fixes: 7f7b96a8a0a1 ("drm/kmb: Add support for KeemBay Display")
> Fixes: 98521f4d4b4c ("drm/kmb: Mipi DSI part of the display driver")
> Cc: Dan Carpenter <dan.carpenter@oracle.com>
> Signed-off-by: Anitha Chrisanthus <anitha.chrisanthus@intel.com>
> ---
>  drivers/gpu/drm/kmb/kmb_drv.c | 9 ++++++---
>  drivers/gpu/drm/kmb/kmb_dsi.c | 9 +++++----
>  drivers/gpu/drm/kmb/kmb_dsi.h | 3 ++-
>  3 files changed, 13 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/gpu/drm/kmb/kmb_drv.c b/drivers/gpu/drm/kmb/kmb_drv.c
> index bb7eca9e13ae..12f35c43d838 100644
> --- a/drivers/gpu/drm/kmb/kmb_drv.c
> +++ b/drivers/gpu/drm/kmb/kmb_drv.c
> @@ -454,8 +454,9 @@ static int kmb_remove(struct platform_device *pdev)
>  	dev_set_drvdata(dev, NULL);
>  
>  	/* Unregister DSI host */
> -	kmb_dsi_host_unregister(kmb->kmb_dsi);
> +	kmb_dsi_host_unregister();
>  	drm_atomic_helper_shutdown(drm);
> +	drm_dev_put(drm);
>  	return 0;
>  }
>  
> @@ -519,7 +520,7 @@ static int kmb_probe(struct platform_device *pdev)
>  	if (IS_ERR(kmb->kmb_dsi)) {
>  		drm_err(&kmb->drm, "failed to initialize DSI\n");
>  		ret = PTR_ERR(kmb->kmb_dsi);
> -		goto err_free1;
> +		goto err_free2;

Don't use numberred labels.  The label names should say what the goto
does just like a function name says what calling the function does.
The existing label names in this function mostly use "Come From" label
style which is not useful either.

When you're writing probe function keep track in your head of the most
recent successful allocation and then when an error occurs that's what
you have to free.

	a = alloc();
	if (!a)
		return;  <-- nothing to free

	b = alloc();
	if (!b)
		goto free_a;  <-- good name.  a is the most recent.

	c = alloc();
	if (!c)
		goto free_b;

	return 0;

free_b:
	free(b);
free_a:
	free(a);

Then copy and past the error handling and add a free(c) to create the
release function:

void release(struct foo *p)
{
	free(c);
	free(b);
	free(a);

}

regards,
dan carpenter