| Message ID | 20190504030813.17684-1-baijiaju1990@gmail.com |
|---|---|
| State | Changes Requested |
| Delegated to: | David Miller |
| Headers | show |
| Series | net: via-rhine: net: Fix a resource leak in rhine_init() | expand |
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.
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
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.
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
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; }
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(-)