* [PATCH] drivers: watchdog: pc87413_wdt: Fix Race condition bug
@ 2020-08-13 12:54 madhuparnabhowmik10
2020-08-14 15:07 ` Guenter Roeck
0 siblings, 1 reply; 3+ messages in thread
From: madhuparnabhowmik10 @ 2020-08-13 12:54 UTC (permalink / raw)
To: wim, linux
Cc: linux-watchdog, linux-kernel, andrianov, ldv-project, Madhuparna Bhowmik
From: Madhuparna Bhowmik <madhuparnabhowmik10@gmail.com>
After misc_register the open() callback can be called.
However the base address (swc_base_addr) is set after misc_register()
in init.
As a result, if open callback is called before pc87413_get_swc_base_addr()
then in the following call chain: pc87413_open() -> pc87413_refresh() ->
pc87413_swc_bank3() : The value of swc_base_addr will be -1.
Therefore, do misc_register() after pc87413_get_swc_base_addr().
Found by Linux Driver Verification project (linuxtesting.org).
Signed-off-by: Madhuparna Bhowmik <madhuparnabhowmik10@gmail.com>
---
drivers/watchdog/pc87413_wdt.c | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/drivers/watchdog/pc87413_wdt.c b/drivers/watchdog/pc87413_wdt.c
index 73fbfc99083b..ad8b8af2bdc0 100644
--- a/drivers/watchdog/pc87413_wdt.c
+++ b/drivers/watchdog/pc87413_wdt.c
@@ -512,6 +512,10 @@ static int __init pc87413_init(void)
if (ret != 0)
pr_err("cannot register reboot notifier (err=%d)\n", ret);
+ pc87413_select_wdt_out();
+ pc87413_enable_swc();
+ pc87413_get_swc_base_addr();
+
ret = misc_register(&pc87413_miscdev);
if (ret != 0) {
pr_err("cannot register miscdev on minor=%d (err=%d)\n",
@@ -520,10 +524,6 @@ static int __init pc87413_init(void)
}
pr_info("initialized. timeout=%d min\n", timeout);
- pc87413_select_wdt_out();
- pc87413_enable_swc();
- pc87413_get_swc_base_addr();
-
if (!request_region(swc_base_addr, 0x20, MODNAME)) {
pr_err("cannot request SWC region at 0x%x\n", swc_base_addr);
ret = -EBUSY;
--
2.17.1
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH] drivers: watchdog: pc87413_wdt: Fix Race condition bug
2020-08-13 12:54 [PATCH] drivers: watchdog: pc87413_wdt: Fix Race condition bug madhuparnabhowmik10
@ 2020-08-14 15:07 ` Guenter Roeck
2020-08-16 7:24 ` Madhuparna Bhowmik
0 siblings, 1 reply; 3+ messages in thread
From: Guenter Roeck @ 2020-08-14 15:07 UTC (permalink / raw)
To: madhuparnabhowmik10
Cc: wim, linux-watchdog, linux-kernel, andrianov, ldv-project
On Thu, Aug 13, 2020 at 06:24:51PM +0530, madhuparnabhowmik10@gmail.com wrote:
> From: Madhuparna Bhowmik <madhuparnabhowmik10@gmail.com>
>
> After misc_register the open() callback can be called.
> However the base address (swc_base_addr) is set after misc_register()
> in init.
> As a result, if open callback is called before pc87413_get_swc_base_addr()
> then in the following call chain: pc87413_open() -> pc87413_refresh() ->
> pc87413_swc_bank3() : The value of swc_base_addr will be -1.
> Therefore, do misc_register() after pc87413_get_swc_base_addr().
>
> Found by Linux Driver Verification project (linuxtesting.org).
>
> Signed-off-by: Madhuparna Bhowmik <madhuparnabhowmik10@gmail.com>
Another candidate for removal. The entire driver is at the very least
questionable: It hard enables the watchdog after registering it, making it
mandatory to open it within one minute or the system will reboot. Also,
the driver doesn't even check if the hardware even exists; it just assumes
that it does. And then it reconfigures that potentially non-existing
hardware to use a specific chip pin as wdt output, as if that would be
useful if that pin is not wired up. Worst case that driver may actually
break something if it is loaded on an arbitrary system.
I really don't see the point of trying to patch it up unless there are users
left who can confirm that it even works on existing hardware, and then I'd
prefer to have it fixed for good and not just patched up.
Thanks,
Guenter
> ---
> drivers/watchdog/pc87413_wdt.c | 8 ++++----
> 1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/watchdog/pc87413_wdt.c b/drivers/watchdog/pc87413_wdt.c
> index 73fbfc99083b..ad8b8af2bdc0 100644
> --- a/drivers/watchdog/pc87413_wdt.c
> +++ b/drivers/watchdog/pc87413_wdt.c
> @@ -512,6 +512,10 @@ static int __init pc87413_init(void)
> if (ret != 0)
> pr_err("cannot register reboot notifier (err=%d)\n", ret);
>
> + pc87413_select_wdt_out();
> + pc87413_enable_swc();
> + pc87413_get_swc_base_addr();
> +
> ret = misc_register(&pc87413_miscdev);
> if (ret != 0) {
> pr_err("cannot register miscdev on minor=%d (err=%d)\n",
> @@ -520,10 +524,6 @@ static int __init pc87413_init(void)
> }
> pr_info("initialized. timeout=%d min\n", timeout);
>
> - pc87413_select_wdt_out();
> - pc87413_enable_swc();
> - pc87413_get_swc_base_addr();
> -
> if (!request_region(swc_base_addr, 0x20, MODNAME)) {
> pr_err("cannot request SWC region at 0x%x\n", swc_base_addr);
> ret = -EBUSY;
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH] drivers: watchdog: pc87413_wdt: Fix Race condition bug
2020-08-14 15:07 ` Guenter Roeck
@ 2020-08-16 7:24 ` Madhuparna Bhowmik
0 siblings, 0 replies; 3+ messages in thread
From: Madhuparna Bhowmik @ 2020-08-16 7:24 UTC (permalink / raw)
To: Guenter Roeck
Cc: madhuparnabhowmik10, wim, linux-watchdog, linux-kernel,
andrianov, ldv-project
On Fri, Aug 14, 2020 at 08:07:40AM -0700, Guenter Roeck wrote:
> On Thu, Aug 13, 2020 at 06:24:51PM +0530, madhuparnabhowmik10@gmail.com wrote:
> > From: Madhuparna Bhowmik <madhuparnabhowmik10@gmail.com>
> >
> > After misc_register the open() callback can be called.
> > However the base address (swc_base_addr) is set after misc_register()
> > in init.
> > As a result, if open callback is called before pc87413_get_swc_base_addr()
> > then in the following call chain: pc87413_open() -> pc87413_refresh() ->
> > pc87413_swc_bank3() : The value of swc_base_addr will be -1.
> > Therefore, do misc_register() after pc87413_get_swc_base_addr().
> >
> > Found by Linux Driver Verification project (linuxtesting.org).
> >
> > Signed-off-by: Madhuparna Bhowmik <madhuparnabhowmik10@gmail.com>
>
> Another candidate for removal. The entire driver is at the very least
> questionable: It hard enables the watchdog after registering it, making it
> mandatory to open it within one minute or the system will reboot. Also,
> the driver doesn't even check if the hardware even exists; it just assumes
> that it does. And then it reconfigures that potentially non-existing
> hardware to use a specific chip pin as wdt output, as if that would be
> useful if that pin is not wired up. Worst case that driver may actually
> break something if it is loaded on an arbitrary system.
>
> I really don't see the point of trying to patch it up unless there are users
> left who can confirm that it even works on existing hardware, and then I'd
> prefer to have it fixed for good and not just patched up.
>
Sure makes sense. Thank you for reviewing.
Regards,
Madhuparna
> Thanks,
> Guenter
>
> > ---
> > drivers/watchdog/pc87413_wdt.c | 8 ++++----
> > 1 file changed, 4 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/watchdog/pc87413_wdt.c b/drivers/watchdog/pc87413_wdt.c
> > index 73fbfc99083b..ad8b8af2bdc0 100644
> > --- a/drivers/watchdog/pc87413_wdt.c
> > +++ b/drivers/watchdog/pc87413_wdt.c
> > @@ -512,6 +512,10 @@ static int __init pc87413_init(void)
> > if (ret != 0)
> > pr_err("cannot register reboot notifier (err=%d)\n", ret);
> >
> > + pc87413_select_wdt_out();
> > + pc87413_enable_swc();
> > + pc87413_get_swc_base_addr();
> > +
> > ret = misc_register(&pc87413_miscdev);
> > if (ret != 0) {
> > pr_err("cannot register miscdev on minor=%d (err=%d)\n",
> > @@ -520,10 +524,6 @@ static int __init pc87413_init(void)
> > }
> > pr_info("initialized. timeout=%d min\n", timeout);
> >
> > - pc87413_select_wdt_out();
> > - pc87413_enable_swc();
> > - pc87413_get_swc_base_addr();
> > -
> > if (!request_region(swc_base_addr, 0x20, MODNAME)) {
> > pr_err("cannot request SWC region at 0x%x\n", swc_base_addr);
> > ret = -EBUSY;
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2020-08-16 7:24 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-08-13 12:54 [PATCH] drivers: watchdog: pc87413_wdt: Fix Race condition bug madhuparnabhowmik10
2020-08-14 15:07 ` Guenter Roeck
2020-08-16 7:24 ` Madhuparna Bhowmik
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).