[PATCH] rtc: snvs: fix possible race condition

[PATCH] rtc: snvs: fix possible race condition

[Anson.Huang]

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(-)

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

RE: [PATCH] rtc: snvs: fix possible race condition

[Aisheng Dong]

> 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

RE: [PATCH] rtc: snvs: fix possible race condition

[Anson Huang]

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 

RE: [PATCH] rtc: snvs: fix possible race condition

[Aisheng Dong]

> 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

Re: [PATCH] rtc: snvs: fix possible race condition

[Trent Piepho]

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.

> 

RE: [PATCH] rtc: snvs: fix possible race condition

[Anson Huang]

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

Re: [PATCH] rtc: snvs: fix possible race condition

[Trent Piepho]

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?

Re: [PATCH] rtc: snvs: fix possible race condition

[Alexandre Belloni]

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.

-- 
Alexandre Belloni, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

RE: [PATCH] rtc: snvs: fix possible race condition

[Anson Huang]

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

Re: [PATCH] rtc: snvs: fix possible race condition

[Alexandre Belloni]

On 16/07/2019 15:18:58+0800, Anson.Huang@nxp.com wrote:
> 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(-)
> 
Applied, thanks.

-- 
Alexandre Belloni, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com