[PATCH] net: via-rhine: net: Fix a resource leak in rhine_init()

[PATCH] net: via-rhine: net: Fix a resource leak in rhine_init()

[Jia-Ju Bai]

When platform_driver_register() fails, pci_unregister_driver() is not
called to release the resource allocated by pci_register_driver().

To fix this bug, error handling code for platform_driver_register() and
pci_register_driver() is separately implemented.

This bug is found by a runtime fuzzing tool named FIZZER written by us.

Signed-off-by: Jia-Ju Bai <baijiaju1990@gmail.com>
---
 drivers/net/ethernet/via/via-rhine.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/via/via-rhine.c b/drivers/net/ethernet/via/via-rhine.c
index 33949248c829..eb74e5a03aac 100644
--- a/drivers/net/ethernet/via/via-rhine.c
+++ b/drivers/net/ethernet/via/via-rhine.c
@@ -2633,10 +2633,15 @@ static int __init rhine_init(void)
 		pr_info("avoid_D3 set\n");
 
 	ret_pci = pci_register_driver(&rhine_driver_pci);
-	ret_platform = platform_driver_register(&rhine_driver_platform);
-	if ((ret_pci < 0) && (ret_platform < 0))
+	if (ret_pci)
 		return ret_pci;
 
+	ret_platform = platform_driver_register(&rhine_driver_platform);
+	if (ret_platform) {
+		pci_unregister_driver(&rhine_driver_pci);
+		return ret_platform;
+	}
+
 	return 0;
 }
 
-- 
2.17.0

Re: [PATCH] net: via-rhine: net: Fix a resource leak in rhine_init()

[David Miller]

From: Jia-Ju Bai <baijiaju1990@gmail.com>
Date: Sat,  4 May 2019 11:08:13 +0800

> When platform_driver_register() fails, pci_unregister_driver() is not
> called to release the resource allocated by pci_register_driver().
> 
> To fix this bug, error handling code for platform_driver_register() and
> pci_register_driver() is separately implemented.
> 
> This bug is found by a runtime fuzzing tool named FIZZER written by us.
> 
> Signed-off-by: Jia-Ju Bai <baijiaju1990@gmail.com>

I think the idea here is that PCI is not enabled in the kernel, it is
fine for the pci register to fail and only the platform register to
succeed.

You are breaking that.

Re: [PATCH] net: via-rhine: net: Fix a resource leak in rhine_init()

[Jia-Ju Bai]

On 2019/5/4 11:13, David Miller wrote:
> From: Jia-Ju Bai <baijiaju1990@gmail.com>
> Date: Sat,  4 May 2019 11:08:13 +0800
>
>> When platform_driver_register() fails, pci_unregister_driver() is not
>> called to release the resource allocated by pci_register_driver().
>>
>> To fix this bug, error handling code for platform_driver_register() and
>> pci_register_driver() is separately implemented.
>>
>> This bug is found by a runtime fuzzing tool named FIZZER written by us.
>>
>> Signed-off-by: Jia-Ju Bai <baijiaju1990@gmail.com>
> I think the idea here is that PCI is not enabled in the kernel, it is
> fine for the pci register to fail and only the platform register to
> succeed.
>
> You are breaking that.

Okay, I can understand it.
If so, I think that platform_driver_register() should be called before 
pci_register_driver(), and it is still necessary to separately handle 
their errors.
If you agree, I will send a v2 patch.


Best wishes,
Jia-Ju Bai

Re: [PATCH] net: via-rhine: net: Fix a resource leak in rhine_init()

[David Miller]

From: Jia-Ju Bai <baijiaju1990@gmail.com>
Date: Sat, 4 May 2019 11:23:06 +0800

> 
> 
> On 2019/5/4 11:13, David Miller wrote:
>> From: Jia-Ju Bai <baijiaju1990@gmail.com>
>> Date: Sat,  4 May 2019 11:08:13 +0800
>>
>>> When platform_driver_register() fails, pci_unregister_driver() is not
>>> called to release the resource allocated by pci_register_driver().
>>>
>>> To fix this bug, error handling code for platform_driver_register()
>>> and
>>> pci_register_driver() is separately implemented.
>>>
>>> This bug is found by a runtime fuzzing tool named FIZZER written by
>>> us.
>>>
>>> Signed-off-by: Jia-Ju Bai <baijiaju1990@gmail.com>
>> I think the idea here is that PCI is not enabled in the kernel, it is
>> fine for the pci register to fail and only the platform register to
>> succeed.
>>
>> You are breaking that.
> 
> Okay, I can understand it.
> If so, I think that platform_driver_register() should be called before
> pci_register_driver(), and it is still necessary to separately handle
> their errors.
> If you agree, I will send a v2 patch.

It is only a failure if both fail.

If at least one succeeds, the driver can potentially probe properly.

Re: [PATCH] net: via-rhine: net: Fix a resource leak in rhine_init()

[Jia-Ju Bai]

On 2019/5/4 11:41, David Miller wrote:
> From: Jia-Ju Bai <baijiaju1990@gmail.com>
> Date: Sat, 4 May 2019 11:23:06 +0800
>
>>
>> On 2019/5/4 11:13, David Miller wrote:
>>> From: Jia-Ju Bai <baijiaju1990@gmail.com>
>>> Date: Sat,  4 May 2019 11:08:13 +0800
>>>
>>>> When platform_driver_register() fails, pci_unregister_driver() is not
>>>> called to release the resource allocated by pci_register_driver().
>>>>
>>>> To fix this bug, error handling code for platform_driver_register()
>>>> and
>>>> pci_register_driver() is separately implemented.
>>>>
>>>> This bug is found by a runtime fuzzing tool named FIZZER written by
>>>> us.
>>>>
>>>> Signed-off-by: Jia-Ju Bai <baijiaju1990@gmail.com>
>>> I think the idea here is that PCI is not enabled in the kernel, it is
>>> fine for the pci register to fail and only the platform register to
>>> succeed.
>>>
>>> You are breaking that.
>> Okay, I can understand it.
>> If so, I think that platform_driver_register() should be called before
>> pci_register_driver(), and it is still necessary to separately handle
>> their errors.
>> If you agree, I will send a v2 patch.
> It is only a failure if both fail.
>
> If at least one succeeds, the driver can potentially probe properly.

Thanks for the explanation, I understand now :)
The code logic seems a little strange, but it should be correct in 
rhine_init()...
If so, if one fails, I wonder whether it is correct for rhine_cleanup() 
to call both platform_driver_unregister() and pci_unregister_driver()?


Best wishes,
Jia-Ju Bai