linux-kernel - Re: [PATCH] drivers: watchdog: pc87413

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]

Date:   Sun, 16 Aug 2020 12:54:41 +0530
From:   Madhuparna Bhowmik <madhuparnabhowmik10@...il.com>
To:     Guenter Roeck <linux@...ck-us.net>
Cc:     madhuparnabhowmik10@...il.com, wim@...ux-watchdog.org,
        linux-watchdog@...r.kernel.org, linux-kernel@...r.kernel.org,
        andrianov@...ras.ru, ldv-project@...uxtesting.org
Subject: Re: [PATCH] drivers: watchdog: pc87413_wdt: Fix Race condition bug

On Fri, Aug 14, 2020 at 08:07:40AM -0700, Guenter Roeck wrote:
> On Thu, Aug 13, 2020 at 06:24:51PM +0530, madhuparnabhowmik10@...il.com wrote:
> > From: Madhuparna Bhowmik <madhuparnabhowmik10@...il.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@...il.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;