linux-kernel - RE: [PATCH] rtc: snvs: fix possible race condition

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] [thread-next>] [day] [month] [year] [list]

Message-ID: <DB3PR0402MB3916053E6344520416BC976BF5CB0@DB3PR0402MB3916.eurprd04.prod.outlook.com>
Date:   Fri, 19 Jul 2019 02:57:20 +0000
From:   Anson Huang <anson.huang@....com>
To:     Trent Piepho <tpiepho@...inj.com>,
        "linux-rtc@...r.kernel.org" <linux-rtc@...r.kernel.org>,
        "a.zummo@...ertech.it" <a.zummo@...ertech.it>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "alexandre.belloni@...tlin.com" <alexandre.belloni@...tlin.com>,
        Aisheng Dong <aisheng.dong@....com>
CC:     dl-linux-imx <linux-imx@....com>
Subject: RE: [PATCH] rtc: snvs: fix possible race condition

Hi, Trent

> On Thu, 2019-07-18 at 03:08 +0000, Aisheng Dong wrote:
> > > From: Anson Huang
> > > Sent: Wednesday, July 17, 2019 9:58 PM> Hi, Aisheng
> > >
> > > > > From: Anson.Huang@....com <Anson.Huang@....com>
> > > > > Sent: Tuesday, July 16, 2019 3:19 PM
> > > > >
> > > > > The RTC IRQ is requested before the struct rtc_device is
> > > > > allocated, this may lead to a NULL pointer dereference in IRQ
> > > > > handler.
> > > > >
> > > > > To fix this issue, allocating the rtc_device struct before
> > > > > requesting the RTC IRQ using devm_rtc_allocate_device, and use
> > > > > rtc_register_device to register the RTC device.
> > > > >
> > > >
> > > > I saw other rtc drivers did the same way as us, so this looks like
> > > > a common problem.
> > > > My question is if we can clear interrupt status before register to
> > > > avoid this issue as other rtc drivers?
> > >
> > > I think we can NOT predict when the IRQ will be pending, IRQ could
> > > arrive at any time, the most safe way is to prepare everything
> > > before requesting/enabling IRQ.
> > > There is also patch to fix similar issue:
> 
> I think one could attempt to disable all irq sources in the device via its
> register space, then enable the interrupt.  But this seems more specific to
> each device than changing the pattern of device registration, so IMHO, it's
> not really better.
> 
> I do worry that handling the irq before the rtc device is registered could still
> result in a crash.  From what I saw, the irq path in snvs only uses driver state
> members that are fully initialized for the most part, and the allocated but
> unregistered data->rtc is only used in one call to rtc_update_irq(), which
> appears to be ok with this.
> 
> But it is not that hard to imagine that something could go into the rtc core
> that assumes call like rtc_update_irq() are only made on registered devices.
> 
> If there was a way to do it, I think allocating the irq in a masked state and
> then unmasking it as part of the final registration call to make the device go
> live would be a safer and more general pattern.

It makes sense, I think we can just move the devm_request_irq() to after rtc_register_device(),
It will make sure everything is ready before IRQ is enabled. Will send out a V2 patch. 

Thanks,
Anson