linux-kernel - Re: [PATCH] gpu: gma500: psb_drv: add missing unwind goto

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <be7cfdac-29bf-47ee-8efa-4a5d06faf165@kili.mountain>
Date:   Mon, 17 Apr 2023 09:37:33 +0300
From:   Dan Carpenter <error27@...il.com>
To:     Hongqi Chen <U202112190@...t.edu.cn>
Cc:     Patrik Jakobsson <patrik.r.jakobsson@...il.com>,
        David Airlie <airlied@...il.com>,
        Daniel Vetter <daniel@...ll.ch>,
        Dave Airlie <airlied@...hat.com>,
        Alan Cox <alan@...ux.intel.com>,
        hust-os-kernel-patches@...glegroups.com,
        Dongliang Mu <dzm91@...t.edu.cn>,
        dri-devel@...ts.freedesktop.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] gpu: gma500: psb_drv: add missing unwind goto

On Mon, Apr 17, 2023 at 02:01:05PM +0800, Hongqi Chen wrote:
> Smatch reports that missing unwind goto in psb_driver_load().
> drivers/gpu/drm/gma500/psb_drv.c:350 psb_driver_load() warn: missing
> unwind goto?
> 
> psb_driver_unload() and psb_driver_load() exist in correspondence, 
> and psb_driver_unload() should be executed when the psb_do_init() 
> fails to initialize.
> 
> Fixes: 5c49fd3aa0ab ("gma500: Add the core DRM files and headers")
> Signed-off-by: Hongqi Chen <U202112190@...t.edu.cn>
> Reviewed-by: Dongliang Mu <dzm91@...t.edu.cn>
> ---
>  drivers/gpu/drm/gma500/psb_drv.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/gma500/psb_drv.c b/drivers/gpu/drm/gma500/psb_drv.c
> index cd9c73f5a64a..b5a276342129 100644
> --- a/drivers/gpu/drm/gma500/psb_drv.c
> +++ b/drivers/gpu/drm/gma500/psb_drv.c
> @@ -347,7 +347,7 @@ static int psb_driver_load(struct drm_device *dev, unsigned long flags)
>  
>  	ret = psb_do_init(dev);
>  	if (ret)
> -		return ret;
> +		goto out_err;

This kind of error handling is called One Magical Cleanup Function.
These functions are always buggy.  My instinct is that it's better to
just return directly and leak resources instead of doing whatever
horrible thing psb_driver_unload() does.

Update:  I started to look at the psb_driver_unload() and the first line
is gma_backlight_exit() which will lead to a crash if
backlight_device_register() fails.

Reviewing One Magical Cleanup Function is a waste of time, easier to
just re-write it.

The problem as well is that in olden days if the probe() function failed
you were screwed and so no one cared about error handling and cleanup.
Who cares about the details when the result is the same (dead system).
But these days we return -EPROBE_DEFER which is not a fatal error and
we're supposed to recover from that without crashing.  So on modern
drivers changing the error path from a leak to a crash has a huge
negative impact.

regards,
dan carpenter