[PATCH] platform/x86: lenovo-yoga-tab2-pro-1380-fastcharger: cancel work on teardown

[PATCH] platform/x86: lenovo-yoga-tab2-pro-1380-fastcharger: cancel work on teardown

[hyunjungg]

From: Hyunjung Ko <hj351016@gmail.com>

fc->work is scheduled from the extcon notifier callback
yt2_1380_fc_extcon_evt() (and once from probe), but it is never cancelled.
The containing struct yt2_1380_fc and the extcon notifier are both
device-managed on the serdev device, and the serdev driver has no
->remove callback, so teardown happens entirely via devres unwinding
when yt2_1380_fc_pdev_remove() calls serdev_device_remove().

devm_extcon_register_notifier_all() unregistering on teardown only stops
future notifications; it does not drain a work item that the notifier has
already queued. A fast-charger extcon event that races device unbind can
therefore queue fc->work just before teardown; the work then runs after
devres has freed fc, dereferencing freed memory (fc->extcon, fc->dev and
the serdev device) in yt2_1380_fc_worker() -> use-after-free.

Cancel the work via devm_add_action_or_reset(), registered before
devm_extcon_register_notifier_all(). Because devres runs cleanup in
reverse order, the notifier is unregistered first (so it can no longer
queue the work), and only then is the work cancelled, before fc is freed.
This mirrors the teardown-ordering fix used in the axp288 charger driver.

Found by static analysis / code review; compile-tested only, not
reproduced on hardware (requires the Lenovo Yoga Tablet 2 1380 charger).

Fixes: b2ed33e8d486 ("platform/x86: Add lenovo-yoga-tab2-pro-1380-fastcharger driver")
Signed-off-by: Hyunjung Ko <hj351016@gmail.com>
---
--- a/drivers/platform/x86/lenovo/yoga-tab2-pro-1380-fastcharger.c
+++ b/drivers/platform/x86/lenovo/yoga-tab2-pro-1380-fastcharger.c
@@ -148,6 +148,11 @@
 	.write_wakeup = serdev_device_write_wakeup,
 };
 
+static void yt2_1380_fc_cancel_work(void *data)
+{
+	cancel_work_sync(data);
+}
+
 static int yt2_1380_fc_serdev_probe(struct serdev_device *serdev)
 {
 	struct device *dev = &serdev->dev;
@@ -163,6 +168,15 @@
 	INIT_WORK(&fc->work, yt2_1380_fc_worker);
 
 	/*
+	 * Register the work cancellation before the extcon notifier, so that
+	 * devm's reverse-order teardown unregisters the notifier (which may
+	 * queue the work) before the work is cancelled and fc is freed.
+	 */
+	ret = devm_add_action_or_reset(dev, yt2_1380_fc_cancel_work, &fc->work);
+	if (ret)
+		return ret;
+
+	/*
 	 * Do this first since it may return -EPROBE_DEFER.
 	 * There is no extcon_put(), so there is no need to free this.
 	 */
-- 
2.43.0

[PATCH v2] platform/x86: lenovo-yoga-tab2-pro-1380-fastcharger: cancel work on teardown

[hyunjungg]

From: Hyunjung Ko <hj351016@gmail.com>

fc->work is scheduled from the extcon notifier callback
yt2_1380_fc_extcon_evt() (and once from probe), but it is never cancelled.
The containing struct yt2_1380_fc and the extcon notifier are both
device-managed on the serdev device, and the serdev driver has no
->remove callback, so teardown happens entirely via devres unwinding
when yt2_1380_fc_pdev_remove() calls serdev_device_remove().

devm_extcon_register_notifier_all() unregistering on teardown only stops
future notifications; it does not drain a work item that the notifier has
already queued. A fast-charger extcon event that races device unbind can
therefore queue fc->work just before teardown; the work then runs after
devres has torn the device down. yt2_1380_fc_worker() dereferences not
only fc itself but also fc->pinctrl (pinctrl_select_state()) and the
serdev (serdev_device_write_buf()), so it must be drained before those
device-managed resources are released, not merely before fc is freed.

Cancel the work via devm_add_action_or_reset(), registered after the
serdev is opened and pinctrl is acquired but before
devm_extcon_register_notifier_all(). Because devres runs cleanup in
reverse order, teardown then unregisters the notifier first (so it can no
longer queue the work), next cancels the work while fc->pinctrl and the
serdev are still valid, and only then closes the serdev, releases pinctrl
and frees fc. This closes both the re-arm window and the use-after-free
of fc and of its sub-resources in the worker.

Found by static analysis / code review; compile-tested only, not
reproduced on hardware (requires the Lenovo Yoga Tablet 2 1380 charger).

Fixes: b2ed33e8d486 ("platform/x86: Add lenovo-yoga-tab2-pro-1380-fastcharger driver")
Signed-off-by: Hyunjung Ko <hj351016@gmail.com>
---
v2:
 - Move the devm_add_action_or_reset(cancel_work) registration from right
   after INIT_WORK to after devm_serdev_device_open()/devm_pinctrl_get()
   and before devm_extcon_register_notifier_all(). The v1 placement
   registered the cancel action first, so devres (LIFO) drained the work
   last - after the serdev was closed and pinctrl released - leaving the
   worker's fc->pinctrl / serdev dereferences racing freed resources.
   The new placement drains the work while those resources are still live.

 drivers/platform/x86/lenovo/yoga-tab2-pro-1380-fastcharger.c | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

diff --git a/drivers/platform/x86/lenovo/yoga-tab2-pro-1380-fastcharger.c b/drivers/platform/x86/lenovo/yoga-tab2-pro-1380-fastcharger.c
--- a/drivers/platform/x86/lenovo/yoga-tab2-pro-1380-fastcharger.c
+++ b/drivers/platform/x86/lenovo/yoga-tab2-pro-1380-fastcharger.c
@@ -148,6 +148,11 @@
 	.write_wakeup = serdev_device_write_wakeup,
 };

+static void yt2_1380_fc_cancel_work(void *data)
+{
+	cancel_work_sync(data);
+}
+
 static int yt2_1380_fc_serdev_probe(struct serdev_device *serdev)
 {
 	struct device *dev = &serdev->dev;
@@ -209,6 +214,18 @@
 	serdev_device_set_baudrate(serdev, 600);
 	serdev_device_set_flow_control(serdev, false);

+	/*
+	 * Register work cancellation after the serdev is opened and pinctrl
+	 * is acquired, but before the extcon notifier.  devres unwinds in
+	 * reverse, so teardown unregisters the notifier first (no new work
+	 * can be queued), then cancels the work while fc->pinctrl and the
+	 * serdev are still valid, and only then closes the serdev and
+	 * releases pinctrl.
+	 */
+	ret = devm_add_action_or_reset(dev, yt2_1380_fc_cancel_work, &fc->work);
+	if (ret)
+		return ret;
+
 	ret = devm_extcon_register_notifier_all(dev, fc->extcon, &fc->nb);
 	if (ret)
 		return dev_err_probe(dev, ret, "registering extcon notifier\n");