rtc: snvs: fix possible race condition
diff mbox series

Message ID 20190716071858.36750-1-Anson.Huang@nxp.com
State New
Headers show
Series
  • rtc: snvs: fix possible race condition
Related show

Commit Message

Anson Huang July 16, 2019, 7:18 a.m. UTC
From: Anson Huang <Anson.Huang@nxp.com>

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.

Signed-off-by: Anson Huang <Anson.Huang@nxp.com>
---
 drivers/rtc/rtc-snvs.c | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

Comments

Dong Aisheng July 17, 2019, 10:54 a.m. UTC | #1
> From: Anson.Huang@nxp.com <Anson.Huang@nxp.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?

Regards
Aisheng

> Signed-off-by: Anson Huang <Anson.Huang@nxp.com>
> ---
>  drivers/rtc/rtc-snvs.c | 11 +++++++----
>  1 file changed, 7 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/rtc/rtc-snvs.c b/drivers/rtc/rtc-snvs.c index
> 7ee673a2..4f9a107 100644
> --- a/drivers/rtc/rtc-snvs.c
> +++ b/drivers/rtc/rtc-snvs.c
> @@ -279,6 +279,10 @@ static int snvs_rtc_probe(struct platform_device
> *pdev)
>  	if (!data)
>  		return -ENOMEM;
> 
> +	data->rtc = devm_rtc_allocate_device(&pdev->dev);
> +	if (IS_ERR(data->rtc))
> +		return PTR_ERR(data->rtc);
> +
>  	data->regmap = syscon_regmap_lookup_by_phandle(pdev->dev.of_node,
> "regmap");
> 
>  	if (IS_ERR(data->regmap)) {
> @@ -343,10 +347,9 @@ static int snvs_rtc_probe(struct platform_device
> *pdev)
>  		goto error_rtc_device_register;
>  	}
> 
> -	data->rtc = devm_rtc_device_register(&pdev->dev, pdev->name,
> -					&snvs_rtc_ops, THIS_MODULE);
> -	if (IS_ERR(data->rtc)) {
> -		ret = PTR_ERR(data->rtc);
> +	data->rtc->ops = &snvs_rtc_ops;
> +	ret = rtc_register_device(data->rtc);
> +	if (ret) {
>  		dev_err(&pdev->dev, "failed to register rtc: %d\n", ret);
>  		goto error_rtc_device_register;
>  	}
> --
> 2.7.4
Anson Huang July 17, 2019, 1:57 p.m. UTC | #2
Hi, Aisheng

> > From: Anson.Huang@nxp.com <Anson.Huang@nxp.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:

commit 060711f5274dfc2d76a5b2cd65abf6ccbf061e40
Author: Alexandre Belloni <alexandre.belloni@bootlin.com>
Date:   Tue Apr 30 11:32:09 2019 +0200

    rtc: digicolor: fix possible race condition

Anson
Dong Aisheng July 18, 2019, 3:08 a.m. UTC | #3
> From: Anson Huang
> Sent: Wednesday, July 17, 2019 9:58 PM> 
> Hi, Aisheng
> 
> > > From: Anson.Huang@nxp.com <Anson.Huang@nxp.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 just feel like it's common issue. But seems community already did the same thing.
So:
Reviewed-by: Dong Aisheng <aisheng.dong@nxp.com>

Regards
Aisheng

> commit 060711f5274dfc2d76a5b2cd65abf6ccbf061e40
> Author: Alexandre Belloni <alexandre.belloni@bootlin.com>
> Date:   Tue Apr 30 11:32:09 2019 +0200
> 
>     rtc: digicolor: fix possible race condition
> 
> Anson
Trent Piepho July 18, 2019, 4:32 p.m. UTC | #4
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@nxp.com <Anson.Huang@nxp.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.

>
Anson Huang July 19, 2019, 2:57 a.m. UTC | #5
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@nxp.com <Anson.Huang@nxp.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
Trent Piepho July 19, 2019, 7:04 p.m. UTC | #6
On Fri, 2019-07-19 at 02:57 +0000, Anson Huang wrote:
> 
> > 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. 

That will mean registering the rtc, then unregistering it if the irq
request fails.  More of a pain to write this failure path.

Alexandre, is it part of rtc core design that rtc_update_irq() might be
called on a rtc device that is properly allocated, but not registered
yet?
Alexandre Belloni July 20, 2019, 7:55 p.m. UTC | #7
On 19/07/2019 19:04:21+0000, Trent Piepho wrote:
> On Fri, 2019-07-19 at 02:57 +0000, Anson Huang wrote:
> > 
> > > 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. 
> 
> That will mean registering the rtc, then unregistering it if the irq
> request fails.  More of a pain to write this failure path.
> 
> Alexandre, is it part of rtc core design that rtc_update_irq() might be
> called on a rtc device that is properly allocated, but not registered
> yet?

Yes, the main reason of the change of API was exactly to handle this.
Anson Huang Aug. 13, 2019, 9:22 a.m. UTC | #8
Hi, Alexandre

> On 19/07/2019 19:04:21+0000, Trent Piepho wrote:
> > On Fri, 2019-07-19 at 02:57 +0000, Anson Huang wrote:
> > >
> > > > 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.
> >
> > That will mean registering the rtc, then unregistering it if the irq
> > request fails.  More of a pain to write this failure path.
> >
> > Alexandre, is it part of rtc core design that rtc_update_irq() might
> > be called on a rtc device that is properly allocated, but not
> > registered yet?
> 
> Yes, the main reason of the change of API was exactly to handle this.

What about this patch's status? Should we continue or any change needed?

https://patchwork.ozlabs.org/patch/1132481/

Thanks,
Anson

Patch
diff mbox series

diff --git a/drivers/rtc/rtc-snvs.c b/drivers/rtc/rtc-snvs.c
index 7ee673a2..4f9a107 100644
--- a/drivers/rtc/rtc-snvs.c
+++ b/drivers/rtc/rtc-snvs.c
@@ -279,6 +279,10 @@  static int snvs_rtc_probe(struct platform_device *pdev)
 	if (!data)
 		return -ENOMEM;
 
+	data->rtc = devm_rtc_allocate_device(&pdev->dev);
+	if (IS_ERR(data->rtc))
+		return PTR_ERR(data->rtc);
+
 	data->regmap = syscon_regmap_lookup_by_phandle(pdev->dev.of_node, "regmap");
 
 	if (IS_ERR(data->regmap)) {
@@ -343,10 +347,9 @@  static int snvs_rtc_probe(struct platform_device *pdev)
 		goto error_rtc_device_register;
 	}
 
-	data->rtc = devm_rtc_device_register(&pdev->dev, pdev->name,
-					&snvs_rtc_ops, THIS_MODULE);
-	if (IS_ERR(data->rtc)) {
-		ret = PTR_ERR(data->rtc);
+	data->rtc->ops = &snvs_rtc_ops;
+	ret = rtc_register_device(data->rtc);
+	if (ret) {
 		dev_err(&pdev->dev, "failed to register rtc: %d\n", ret);
 		goto error_rtc_device_register;
 	}