linux-kernel - Re: [PATCH] board-dm644x-evm: fix 2 missing-check bugs in evm_led

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: <20191228154039.GA8713@zhanggen-UX430UQ.lan>
Date:   Sat, 28 Dec 2019 23:40:39 +0800
From:   Gen Zhang <blackgod016574@...il.com>
To:     Russell King - ARM Linux admin <linux@...linux.org.uk>
Cc:     bgolaszewski@...libre.com, nsekhar@...com,
        linux-kernel@...r.kernel.org, linux-arm-kernel@...ts.infradead.org
Subject: Re: [PATCH] board-dm644x-evm: fix 2 missing-check bugs in
 evm_led_setup()

On Sat, Dec 28, 2019 at 01:48:24PM +0000, Russell King - ARM Linux admin wrote:
 
> This is the old everything-successful path through the code:
> 
> 	platform_device_alloc()
> 	platform_device_add_data()
> 	platform_device_add()
> 	evm_led_dev is set to the device
> 
> This is the new everything-successful path through the code:
> 
> 	platform_device_alloc()
> 	platform_device_add_data()
> 	platform_device_add()
> 	platform_device_put()
> 	evm_led_dev = NULL
Thanks for your reply. Adding a return may handle this.
	successful path:
	platform_device_alloc()
 	platform_device_add_data()
 	platform_device_add()
	return status
 	
	error path:
	platform_device_put()
 	evm_led_dev = NULL
	return status
correct?

> 
> And, specifically, the code sequence (I quote from your patch):
> 
> 	if (status)
> 		goto err;
> err:
> 
> is very stupid; it might as well not exist at all.

Well, you have to admit that the result of platform_device_alloc(),
platform_device_add_data() and platform_device_add() should be checked,
and error should be handled.

e.g., there are 124 call sites of platform_device_add_data() in Linux. Only 24
are not checked, and most of them are in arm subsystem.

Look forward to deeper discussion of this problem.

Best,
Gen
> 
> Since other code references evm_led_dev, one can assume that we do
> not want it to be NULL for the success path. So, taking all this
> together, your patch is very very wrong, and I also find it very
> worrying too.
> 
> -- 
> RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
> FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up
> According to speedtest.net: 11.9Mbps down 500kbps up