[PATCH v2] ACPI: pfr_telemetry: keep the state until the last file is closed

[PATCH v2] ACPI: pfr_telemetry: keep the state until the last file is closed

[Chad Aldreda]

struct pfrt_log_device is allocated with devm_kzalloc_obj() and embeds
the miscdevice, so it is freed when the driver unbinds. A file that is
open on the node outlives the unbind and nothing holds a reference for
it: the next ioctl() or mmap() on that file uses freed memory.
Unbinding the driver needs root.

Allocate the structure with kzalloc_obj() and reference count it:
open() takes a reference, under misc_mtx, which misc_deregister() also
takes, release() drops it, and a devm action drops the driver's own.
The structure also holds a reference on the parent device it uses.

Also make remove() wait for calls already running. The firmware calls
in ioctl() and mmap() run under a mutex, and remove() sets a flag under
it that makes any later call that would reach the firmware fail with
-ENODEV. Without that, a call that started before remove() can
evaluate _DSM after remove() has returned, while device_del() unbinds
the parent's ACPI companion. The mutex also serializes the updates to
the log settings, which concurrent ioctl() calls could interleave.
ioctl() copies to and from user memory outside the lock, because mmap()
is called with mmap_lock held.

The problem was found with a Coccinelle script that reports device
nodes in devm memory [1]. The fix, the reproducers and this message
were written with an AI assistant (Claude) and checked by hand. The bug
was reproduced and the fix tested in QEMU with KASAN and lockdep, using
an SSDT whose _DSM stands in for the firmware. Hardware with this
firmware interface was not tested.

Link: https://lore.kernel.org/r/20261106193517.3267013-2-ca@modelmiser.com [1]
Fixes: b0013e037a8b ("ACPI: Introduce Platform Firmware Runtime Telemetry driver")
Cc: stable@vger.kernel.org
Assisted-by: Claude:claude-opus-5-5
Signed-off-by: Chad Aldreda <ca@modelmiser.com>
---
Changes in v2:
- remove() now waits for calls already running, under a new mutex, which
  also serializes the log settings. Sashiko's review of v1, which Rafael
  asked me to address, showed that a call that passed the removed check
  can evaluate _DSM after remove() returns, while device_del() unbinds
  the ACPI companion. The v1 notes said such a call finishes against
  state that is still valid; that was wrong.
- kzalloc_obj(), on top of the devm_kzalloc_obj() conversion in
  linux-pm.
v1: https://lore.kernel.org/r/20261108233900.4043941-3-ca@modelmiser.com

Tested in QEMU 10.2.4 (q35, KASAN with kasan_multi_shot, lockdep) with
the SSDT from v1, which adds a device with _HID INTC1081 and a _DSM that
returns valid packages. With a file held open across a sysfs unbind,
SET_INFO, GET_INFO, GET_DATA_INFO and mmap() all work before the unbind
and fail with -ENODEV after it, a mapping made before the unbind stays
readable, the same holds after a bind, and there are no KASAN or lockdep
reports.

For the race, a test-only msleep(2000) was put before the SET_INFO
firmware call and the driver unbound 0.5 s into the call. With v1 the
unbind returned at once and the firmware call completed 1.5 s after
remove() returned. With v2 the unbind returned 2.0 s in, after the call
completed. The test used a sysfs unbind, which keeps the ACPI companion
bound; removal of the device itself was not tested.

A test variant that calls copy_to_user() inside the lock makes lockdep
report a cycle between the new lock, taken in pfrt_log_mmap(), and
mmap_lock, which is why the user copies stay outside it.

Mainline and stable still allocate with devm_kzalloc(), so a backport
needs kzalloc(sizeof(*pfrt_log_dev), GFP_KERNEL) in that hunk.

The same race exists in pfr_update; a matching patch on top of the one
you applied will follow once that is in your tree.

The code, changelog and reproducers were written with an AI assistant
(Claude Opus 5.5), and the assistant ran the tests above. I ran the
final version myself and got the same results.

 drivers/acpi/pfr_telemetry.c | 102 +++++++++++++++++++++++++++++------
 1 file changed, 86 insertions(+), 16 deletions(-)

diff --git a/drivers/acpi/pfr_telemetry.c b/drivers/acpi/pfr_telemetry.c
index 458cb99ae216..53d5e25497e9 100644
--- a/drivers/acpi/pfr_telemetry.c
+++ b/drivers/acpi/pfr_telemetry.c
@@ -15,10 +15,13 @@
 #include <linux/errno.h>
 #include <linux/file.h>
 #include <linux/fs.h>
+#include <linux/kref.h>
 #include <linux/miscdevice.h>
 #include <linux/module.h>
+#include <linux/mutex.h>
 #include <linux/mm.h>
 #include <linux/platform_device.h>
+#include <linux/slab.h>
 #include <linux/string.h>
 #include <linux/uaccess.h>
 #include <linux/uio.h>
@@ -62,6 +65,14 @@ struct pfrt_log_device {
 	struct pfrt_log_info info;
 	struct device *parent_dev;
 	struct miscdevice miscdev;
+	struct kref kref;
+	/*
+	 * Serializes the firmware calls and info of the file operations, and
+	 * remove(), which sets removed under it and so waits for calls
+	 * already running.
+	 */
+	struct mutex lock;
+	bool removed;
 };
 
 /* pfrt_guid is the parameter for _DSM method */
@@ -254,33 +265,49 @@ static long pfrt_log_ioctl(struct file *file, unsigned int cmd, unsigned long ar
 		if (copy_from_user(&info, p, sizeof(info)))
 			return -EFAULT;
 
-		if (valid_log_revid(info.log_revid))
-			pfrt_log_dev->info.log_revid = info.log_revid;
+		scoped_guard(mutex, &pfrt_log_dev->lock) {
+			if (pfrt_log_dev->removed)
+				return -ENODEV;
 
-		if (valid_log_level(info.log_level)) {
-			ret = set_pfrt_log_level(info.log_level, pfrt_log_dev);
-			if (ret < 0)
-				return ret;
+			if (valid_log_revid(info.log_revid))
+				pfrt_log_dev->info.log_revid = info.log_revid;
 
-			pfrt_log_dev->info.log_level = info.log_level;
-		}
+			if (valid_log_level(info.log_level)) {
+				ret = set_pfrt_log_level(info.log_level, pfrt_log_dev);
+				if (ret < 0)
+					return ret;
+
+				pfrt_log_dev->info.log_level = info.log_level;
+			}
 
-		if (valid_log_type(info.log_type))
-			pfrt_log_dev->info.log_type = info.log_type;
+			if (valid_log_type(info.log_type))
+				pfrt_log_dev->info.log_type = info.log_type;
+		}
 
 		return 0;
 
 	case PFRT_LOG_IOC_GET_INFO:
-		info.log_level = get_pfrt_log_level(pfrt_log_dev);
-		info.log_type = pfrt_log_dev->info.log_type;
-		info.log_revid = pfrt_log_dev->info.log_revid;
+		scoped_guard(mutex, &pfrt_log_dev->lock) {
+			if (pfrt_log_dev->removed)
+				return -ENODEV;
+
+			info.log_level = get_pfrt_log_level(pfrt_log_dev);
+			info.log_type = pfrt_log_dev->info.log_type;
+			info.log_revid = pfrt_log_dev->info.log_revid;
+		}
+
 		if (copy_to_user(p, &info, sizeof(info)))
 			return -EFAULT;
 
 		return 0;
 
 	case PFRT_LOG_IOC_GET_DATA_INFO:
-		ret = get_pfrt_log_data_info(&data_info, pfrt_log_dev);
+		scoped_guard(mutex, &pfrt_log_dev->lock) {
+			if (pfrt_log_dev->removed)
+				return -ENODEV;
+
+			ret = get_pfrt_log_data_info(&data_info, pfrt_log_dev);
+		}
 		if (ret)
 			return ret;
 
@@ -311,6 +338,10 @@ pfrt_log_mmap(struct file *file, struct vm_area_struct *vma)
 
 	pfrt_log_dev = to_pfrt_log_dev(file);
 
+	guard(mutex)(&pfrt_log_dev->lock);
+	if (pfrt_log_dev->removed)
+		return -ENODEV;
+
 	ret = get_pfrt_log_data_info(&info, pfrt_log_dev);
 	if (ret)
 		return ret;
@@ -337,8 +368,39 @@ pfrt_log_mmap(struct file *file, struct vm_area_struct *vma)
 	return 0;
 }
 
+static void pfrt_log_free(struct kref *kref)
+{
+	struct pfrt_log_device *pfrt_log_dev =
+		container_of(kref, struct pfrt_log_device, kref);
+
+	put_device(pfrt_log_dev->parent_dev);
+	kfree(pfrt_log_dev);
+}
+
+static void pfrt_log_put(void *data)
+{
+	struct pfrt_log_device *pfrt_log_dev = data;
+
+	kref_put(&pfrt_log_dev->kref, pfrt_log_free);
+}
+
+/* misc_open() holds misc_mtx here, and misc_deregister() takes it. */
+static int pfrt_log_open(struct inode *inode, struct file *file)
+{
+	kref_get(&to_pfrt_log_dev(file)->kref);
+	return 0;
+}
+
+static int pfrt_log_release(struct inode *inode, struct file *file)
+{
+	pfrt_log_put(to_pfrt_log_dev(file));
+	return 0;
+}
+
 static const struct file_operations acpi_pfrt_log_fops = {
 	.owner		= THIS_MODULE,
+	.open		= pfrt_log_open,
+	.release	= pfrt_log_release,
 	.mmap		= pfrt_log_mmap,
 	.unlocked_ioctl = pfrt_log_ioctl,
 	.llseek		= noop_llseek,
@@ -349,6 +411,8 @@ static void acpi_pfrt_log_remove(struct platform_device *pdev)
 	struct pfrt_log_device *pfrt_log_dev = platform_get_drvdata(pdev);
 
 	misc_deregister(&pfrt_log_dev->miscdev);
+	scoped_guard(mutex, &pfrt_log_dev->lock)
+		pfrt_log_dev->removed = true;
 }
 
 static void pfrt_log_put_idx(void *data)
@@ -373,10 +437,17 @@ static int acpi_pfrt_log_probe(struct platform_device *pdev)
 		return -ENODEV;
 	}
 
-	pfrt_log_dev = devm_kzalloc_obj(&pdev->dev, *pfrt_log_dev);
+	pfrt_log_dev = kzalloc_obj(*pfrt_log_dev);
 	if (!pfrt_log_dev)
 		return -ENOMEM;
 
+	kref_init(&pfrt_log_dev->kref);
+	mutex_init(&pfrt_log_dev->lock);
+	pfrt_log_dev->parent_dev = get_device(&pdev->dev);
+	ret = devm_add_action_or_reset(&pdev->dev, pfrt_log_put, pfrt_log_dev);
+	if (ret)
+		return ret;
+
 	ret = ida_alloc(&pfrt_log_ida, GFP_KERNEL);
 	if (ret < 0)
 		return ret;
@@ -387,7 +458,6 @@ static int acpi_pfrt_log_probe(struct platform_device *pdev)
 		return ret;
 
 	pfrt_log_dev->info.log_revid = PFRT_DEFAULT_REV_ID;
-	pfrt_log_dev->parent_dev = &pdev->dev;
 
 	pfrt_log_dev->miscdev.minor = MISC_DYNAMIC_MINOR;
 	pfrt_log_dev->miscdev.name = devm_kasprintf(&pdev->dev, GFP_KERNEL,
-- 
2.43.0