linux-kernel - Re: [PATCH v4] clocksource/timer-imx-gpt: Prevent resource leake in error path.

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]

Date:   Wed, 28 Sep 2016 15:10:27 -0400 (EDT)
From:   Thomas Gleixner <tglx@...utronix.de>
To:     Arvind Yadav <arvind.yadav.cs@...il.com>
cc:     baohua@...nel.org, daniel.lezcano@...aro.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH v4] clocksource/timer-imx-gpt: Prevent resource leake in
 error path.

On Wed, 28 Sep 2016, Arvind Yadav wrote:

> -Free previously allocated memory.
> -Unmap I/O memory from kernel address space.

Ok. So we have proper looking patch with a proper looking changelog
now. Though

> @@ -507,11 +511,17 @@ static int __init mxc_timer_init_dt(struct device_node *np,  enum imx_gpt_type t
>  
>  	ret = _mxc_timer_init(imxtm);
>  	if (ret)
> -		return ret;
> +		goto error_iounmap;

Have you actually thought about the implications of this?

Assume the last function call() which gets invoked via _mxc_timer_init()
fails and then figure out what the iounmap() and the kfree() are going to
cause.

Hint: You just turned a resource leak into a fatal crash.

Just slapping unmap/kfree blindly at everything which can return a failure
is not a good idea if you do not check what the functions actually do and
what the error pathes there are.

While I agree that the error handling in this and in other drivers is
lousy, fixing it just mechanicaly and thereby introducing harder to debug
wreckage is certainly not the right approach.

Thanks,

	tglx