From: Rafael J. Wysocki <[email protected]>
Make cmos_do_remove() drop the ACPI RTC fixed event handler so as to
prevent it from operating on stale data in case the event triggers
after driver removal.
While at it, make cmos_do_remove() also clear the driver data pointer
of the device and make cmos_acpi_wake_setup() do that in the error path
too.
Signed-off-by: Rafael J. Wysocki <[email protected]>
---
drivers/rtc/rtc-cmos.c | 15 +++++++++++++++
1 file changed, 15 insertions(+)
Index: linux-pm/drivers/rtc/rtc-cmos.c
===================================================================
--- linux-pm.orig/drivers/rtc/rtc-cmos.c
+++ linux-pm/drivers/rtc/rtc-cmos.c
@@ -798,6 +798,12 @@ static inline void acpi_rtc_event_setup(
acpi_disable_event(ACPI_EVENT_RTC, 0);
}
+static inline void acpi_rtc_event_cleanup(void)
+{
+ if (!acpi_disabled)
+ acpi_remove_fixed_event_handler(ACPI_EVENT_RTC, rtc_handler);
+}
+
static void rtc_wake_on(struct device *dev)
{
acpi_clear_event(ACPI_EVENT_RTC);
@@ -884,6 +890,10 @@ static inline void acpi_rtc_event_setup(
{
}
+static inline void acpi_rtc_event_cleanup(void)
+{
+}
+
static inline void cmos_acpi_wake_setup(struct device *dev)
{
}
@@ -1109,6 +1119,7 @@ cleanup2:
free_irq(rtc_irq, cmos_rtc.rtc);
cleanup1:
cmos_rtc.dev = NULL;
+ dev_set_drvdata(dev, NULL);
cleanup0:
if (RTC_IOMAPPED)
release_region(ports->start, resource_size(ports));
@@ -1138,6 +1149,9 @@ static void cmos_do_remove(struct device
hpet_unregister_irq_handler(cmos_interrupt);
}
+ if (!dev_get_platdata(dev))
+ acpi_rtc_event_cleanup();
+
cmos->rtc = NULL;
ports = cmos->iomem;
@@ -1148,6 +1162,7 @@ static void cmos_do_remove(struct device
cmos->iomem = NULL;
cmos->dev = NULL;
+ dev_set_drvdata(dev, NULL);
}
static int cmos_aie_poweroff(struct device *dev)
On Mon, Nov 07, 2022 at 09:03:06PM +0100, Rafael J. Wysocki wrote:
...
> +static inline void acpi_rtc_event_cleanup(void)
> +{
> + if (!acpi_disabled)
Btw, other functions look like using
if (acpi_disabled)
return;
pattern. Maybe here the same for the sake of consistency?
> + acpi_remove_fixed_event_handler(ACPI_EVENT_RTC, rtc_handler);
> +}
--
With Best Regards,
Andy Shevchenko
On Mon, Nov 07, 2022 at 09:03:06PM +0100, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <[email protected]>
>
> Make cmos_do_remove() drop the ACPI RTC fixed event handler so as to
> prevent it from operating on stale data in case the event triggers
> after driver removal.
>
> While at it, make cmos_do_remove() also clear the driver data pointer
> of the device and make cmos_acpi_wake_setup() do that in the error path
> too.
...
> + dev_set_drvdata(dev, NULL);
> + dev_set_drvdata(dev, NULL);
Maybe I'm missing something, but the cmos_do_remove() is called by ->remove()
callback of the real drivers (pnp and platform) and device core is already
doing this. So, don't know why you need these calls to be explicit.
--
With Best Regards,
Andy Shevchenko
On Mon, Nov 7, 2022 at 10:21 PM Andy Shevchenko
<[email protected]> wrote:
>
> On Mon, Nov 07, 2022 at 09:03:06PM +0100, Rafael J. Wysocki wrote:
> > From: Rafael J. Wysocki <[email protected]>
> >
> > Make cmos_do_remove() drop the ACPI RTC fixed event handler so as to
> > prevent it from operating on stale data in case the event triggers
> > after driver removal.
> >
> > While at it, make cmos_do_remove() also clear the driver data pointer
> > of the device and make cmos_acpi_wake_setup() do that in the error path
> > too.
>
> ...
>
> > + dev_set_drvdata(dev, NULL);
>
> > + dev_set_drvdata(dev, NULL);
>
> Maybe I'm missing something, but the cmos_do_remove() is called by ->remove()
> callback of the real drivers (pnp and platform) and device core is already
> doing this. So, don't know why you need these calls to be explicit.
Good point, but then I guess I should move this patch to the front,
because the issue fixed by it may trigger a use-after-free in
rtc_handler() already.
On Mon, Nov 7, 2022 at 10:28 PM Andy Shevchenko
<[email protected]> wrote:
>
> On Mon, Nov 07, 2022 at 09:03:06PM +0100, Rafael J. Wysocki wrote:
>
> ...
>
> > +static inline void acpi_rtc_event_cleanup(void)
> > +{
> > + if (!acpi_disabled)
>
> Btw, other functions look like using
>
> if (acpi_disabled)
> return;
>
> pattern. Maybe here the same for the sake of consistency?
>
> > + acpi_remove_fixed_event_handler(ACPI_EVENT_RTC, rtc_handler);
> > +}
>
Well, it is more lines of code, but whatever.