Re: [PATCH] board-dm644x-evm: fix 2 missing-check bugs in evm_led_setup()

From: Gen Zhang
Date: Sat Dec 28 2019 - 10:41:05 EST


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