[PATCH] rtc: atcrtc100: drain the alarm work before the RTC device is freed

[PATCH] rtc: atcrtc100: drain the alarm work before the RTC device is freed

[hyunjungg]

From: Hyunjung Ko <hj351016@gmail.com>

atcrtc_alarm_isr() schedules atcrtc_dev->rtc_work (atcrtc_alarm_clear),
whose handler obtains the atcrtc_dev via container_of() and dereferences
rtc->rtc_dev (guard(rtc_ops)(rtc->rtc_dev)).

atcrtc_probe() requested the alarm IRQ with devm_request_irq() *before*
allocating rtc_dev with devm_rtc_allocate_device(), and INIT_WORK()'d
rtc_work last with no teardown cancel.  Because devres unwinds in reverse
registration order, on unbind the RTC device is freed before the IRQ, and
rtc_work is never cancelled at all: an alarm interrupt arriving around
unbind can run atcrtc_alarm_clear() after rtc->rtc_dev (and the devm
driver data) have been freed -> use-after-free.

Allocate the RTC device first, then register rtc_work for device-managed
cancellation with devm_work_autocancel(), and only then request the IRQ.
devres now frees the IRQ first (so it can no longer schedule the work),
then drains the work while rtc_dev is still valid, then frees rtc_dev and
the driver data.

Found by code review; not reproduced (requires ATCRTC100 hardware and an
alarm interrupt racing device unbind).

Fixes: 7adca706fe16 ("rtc: atcrtc100: Add ATCRTC100 RTC driver")
Cc: stable@vger.kernel.org
Signed-off-by: Hyunjung Ko <hj351016@gmail.com>
---
 drivers/rtc/rtc-atcrtc100.c | 30 +++++++++++++++++++++---------
 1 file changed, 21 insertions(+), 9 deletions(-)

diff --git a/drivers/rtc/rtc-atcrtc100.c b/drivers/rtc/rtc-atcrtc100.c
index 2433df3..51a38f0 100644
--- a/drivers/rtc/rtc-atcrtc100.c
+++ b/drivers/rtc/rtc-atcrtc100.c
@@ -7,6 +7,7 @@
 
 #include <linux/bitfield.h>
 #include <linux/delay.h>
+#include <linux/devm-helpers.h>
 #include <linux/interrupt.h>
 #include <linux/math64.h>
 #include <linux/module.h>
@@ -301,6 +302,26 @@ static int atcrtc_probe(struct platform_device *pdev)
 				     "Failed to get IRQ for alarm\n");
 	atcrtc_dev->alarm_irq = ret;
 
+	atcrtc_dev->rtc_dev = devm_rtc_allocate_device(&pdev->dev);
+	if (IS_ERR(atcrtc_dev->rtc_dev))
+		return dev_err_probe(&pdev->dev, PTR_ERR(atcrtc_dev->rtc_dev),
+				     "Failed to allocate RTC device\n");
+
+	set_bit(RTC_FEATURE_ALARM, atcrtc_dev->rtc_dev->features);
+	atcrtc_dev->rtc_dev->ops = &rtc_ops;
+
+	/*
+	 * Register rtc_work for device-managed cancellation after the RTC
+	 * device it dereferences and before the IRQ that schedules it, so on
+	 * unbind devres frees the IRQ, then drains the work, then frees the
+	 * RTC device and driver data -- never the reverse (atcrtc_alarm_clear()
+	 * touches rtc->rtc_dev).
+	 */
+	ret = devm_work_autocancel(&pdev->dev, &atcrtc_dev->rtc_work,
+				   atcrtc_alarm_clear);
+	if (ret)
+		return ret;
+
 	ret = devm_request_irq(&pdev->dev,
 			       atcrtc_dev->alarm_irq,
 			       atcrtc_alarm_isr,
@@ -312,12 +333,6 @@ static int atcrtc_probe(struct platform_device *pdev)
 				     "Failed to request IRQ %d for alarm\n",
 				     atcrtc_dev->alarm_irq);
 
-	atcrtc_dev->rtc_dev = devm_rtc_allocate_device(&pdev->dev);
-	if (IS_ERR(atcrtc_dev->rtc_dev))
-		return dev_err_probe(&pdev->dev, PTR_ERR(atcrtc_dev->rtc_dev),
-				     "Failed to allocate RTC device\n");
-
-	set_bit(RTC_FEATURE_ALARM, atcrtc_dev->rtc_dev->features);
 	ret = device_init_wakeup(&pdev->dev, true);
 	if (ret)
 		return dev_err_probe(&pdev->dev, ret,
@@ -330,9 +345,6 @@ static int atcrtc_probe(struct platform_device *pdev)
 				     "Failed to set wake IRQ\n");
 	}
 
-	atcrtc_dev->rtc_dev->ops = &rtc_ops;
-
-	INIT_WORK(&atcrtc_dev->rtc_work, atcrtc_alarm_clear);
 	return devm_rtc_register_device(atcrtc_dev->rtc_dev);
 }
 
-- 
2.43.0