diff mbox series

[v1,5/5] rtc: rtc-cmos: Disable ACPI RTC event on removal

Message ID 2219830.iZASKD2KPV@kreacher
State New
Headers
Series rtc: rtc-cmos: Assorted ACPI-related cleanups and fixes |

Commit Message

Rafael J. Wysocki Nov. 7, 2022, 8:03 p.m. UTC 
  From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

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 <rafael.j.wysocki@intel.com>
---
 drivers/rtc/rtc-cmos.c |   15 +++++++++++++++
 1 file changed, 15 insertions(+)

Comments

Andy Shevchenko Nov. 7, 2022, 9:20 p.m. UTC | #1 
On Mon, Nov 07, 2022 at 09:03:06PM +0100, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> 
> 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.
Andy Shevchenko Nov. 7, 2022, 9:28 p.m. UTC | #2 
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);
> +}
Rafael J. Wysocki Nov. 8, 2022, 2:54 p.m. UTC | #3 
On Mon, Nov 7, 2022 at 10:21 PM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
>
> On Mon, Nov 07, 2022 at 09:03:06PM +0100, Rafael J. Wysocki wrote:
> > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> >
> > 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.
Rafael J. Wysocki Nov. 8, 2022, 2:54 p.m. UTC | #4 
On Mon, Nov 7, 2022 at 10:28 PM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> 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.
diff mbox series

Patch

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)