[PATCH 1/2] ACPI: pfr_telemetry: keep the state until the last file is closed

[PATCH 1/2] ACPI: pfr_telemetry: keep the state until the last file is closed

[Chad Aldreda]

struct pfrt_log_device is allocated with devm_kzalloc() 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() 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. A call to
ioctl() or mmap() that starts after remove() returns fails with -ENODEV;
one already running finishes against state that is still valid.

The problem was found with a Coccinelle script that reports device nodes
in devm memory [1]. The fix, the reproducer 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, using an SSDT whose
_DSM stands in for the firmware: ioctl() and mmap() worked before an
unbind and, on a file held open across it, failed with -ENODEV. A call
racing with the unbind was not tested, nor was hardware with this
firmware interface.

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>
---
The Coccinelle script reports a devm_k*() allocation when it, or an
address inside it, is passed to misc_register() or a similar
registration function in the same function, or when the allocated struct
embeds such an object. It reports this driver for the misc_register() of
&pfrt_log_dev->miscdev.

Reproduced in QEMU 10.2.4 (q35, KASAN, booted with kasan_multi_shot). An
SSDT adds a device with _HID INTC1081 and a _DSM that returns valid
packages, and memmap= reserves memory for the log buffer. A process
opens /dev/acpi_pfr_telemetry0, uses SET_INFO, GET_INFO, GET_DATA_INFO
and mmap(), and unbinds the driver through sysfs with the file open.
Repeating those calls gives, without this patch, 15 KASAN reports in
pfrt_log_ioctl(), set_pfrt_log_level() and get_pfrt_log_data_info().

With it there are none. All the calls succeed before the unbind and
return -ENODEV after it, a mapping made before the unbind was still
readable after it, and the same sequence behaves the same after a bind.
Unbinding with the file already closed, or never opened, gives no
reports either.

No lock is added. A call that passed the removed check can run during
the unbind, against state and a parent device that are still valid; the
driver has never serialized calls from different files. Mappings of
the firmware log buffer outlive the unbind, as they did before.

kzalloc() rather than kzalloc_obj(), which first appears in v7.0, for
the stable trees.

 drivers/acpi/pfr_telemetry.c | 55 +++++++++++++++++++++++++++++++++---
 1 file changed, 51 insertions(+), 4 deletions(-)

diff --git a/drivers/acpi/pfr_telemetry.c b/drivers/acpi/pfr_telemetry.c
index 2387376832a1..5ab8baabcc60 100644
--- a/drivers/acpi/pfr_telemetry.c
+++ b/drivers/acpi/pfr_telemetry.c
@@ -15,10 +15,12 @@
 #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/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 +64,8 @@ struct pfrt_log_device {
 	struct pfrt_log_info info;
 	struct device *parent_dev;
 	struct miscdevice miscdev;
+	struct kref kref;
+	bool removed;
 };
 
 /* pfrt_guid is the parameter for _DSM method */
@@ -247,6 +251,9 @@ static long pfrt_log_ioctl(struct file *file, unsigned int cmd, unsigned long ar
 	void __user *p;
 	int ret = 0;
 
+	if (READ_ONCE(pfrt_log_dev->removed))
+		return -ENODEV;
+
 	p = (void __user *)arg;
 
 	switch (cmd) {
@@ -303,14 +310,17 @@ pfrt_log_mmap(struct file *file, struct vm_area_struct *vma)
 	phys_addr_t base_addr;
 	int ret;
 
+	pfrt_log_dev = to_pfrt_log_dev(file);
+
+	if (READ_ONCE(pfrt_log_dev->removed))
+		return -ENODEV;
+
 	if (vma->vm_flags & VM_WRITE)
 		return -EROFS;
 
 	/* changing from read to write with mprotect is not allowed */
 	vm_flags_clear(vma, VM_MAYWRITE);
 
-	pfrt_log_dev = to_pfrt_log_dev(file);
-
 	ret = get_pfrt_log_data_info(&info, pfrt_log_dev);
 	if (ret)
 		return ret;
@@ -337,8 +347,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 +390,7 @@ 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);
+	WRITE_ONCE(pfrt_log_dev->removed, true);
 }
 
 static void pfrt_log_put_idx(void *data)
@@ -373,10 +415,16 @@ static int acpi_pfrt_log_probe(struct platform_device *pdev)
 		return -ENODEV;
 	}
 
-	pfrt_log_dev = devm_kzalloc(&pdev->dev, sizeof(*pfrt_log_dev), GFP_KERNEL);
+	pfrt_log_dev = kzalloc(sizeof(*pfrt_log_dev), GFP_KERNEL);
 	if (!pfrt_log_dev)
 		return -ENOMEM;
 
+	kref_init(&pfrt_log_dev->kref);
+	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 +435,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

[PATCH 2/2] ACPI: pfr_update: keep the state until the last file is closed

[Chad Aldreda]

struct pfru_device is allocated with devm_kzalloc() 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 write() on that file uses freed memory. Unbinding
the driver needs root.

Allocate the structure with kzalloc() 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. A call to
ioctl() or write() that starts after remove() returns fails with
-ENODEV; one already running finishes against state that is still valid.

The problem was found with a Coccinelle script that reports device nodes
in devm memory [1]. The fix, the reproducer 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, using an SSDT whose
_DSM stands in for the firmware: ioctl() and write() worked before an
unbind and, on a file held open across it, failed with -ENODEV. A call
racing with the unbind was not tested, nor was hardware with this
firmware interface.

Link: https://lore.kernel.org/r/20261106193517.3267013-2-ca@modelmiser.com [1]
Fixes: 0db89fa243e5 ("ACPI: Introduce Platform Firmware Runtime Update device driver")
Cc: stable@vger.kernel.org
Assisted-by: Claude:claude-opus-5-5
Signed-off-by: Chad Aldreda <ca@modelmiser.com>
---
The same Coccinelle script reports this driver for the misc_register()
of &pfru_dev->miscdev.

Reproduced in QEMU 10.2.4 (q35, KASAN, booted with kasan_multi_shot). An
SSDT adds a device with _HID INTC1080 and a _DSM that returns valid
packages, and memmap= reserves the communication buffer. A process opens
/dev/acpi_pfr_update0, uses QUERY_CAP, SET_REV, write() of a minimal
capsule and STAGE, and unbinds the driver through sysfs with the file
open. Repeating those calls gives, without this patch, 9 KASAN reports
in pfru_ioctl(), query_capability(), pfru_write() and start_update().

With it there are none. All the calls succeed before the unbind and
return -ENODEV after it, and the same sequence behaves the same after a
bind. Unbinding with the file already closed, or never opened, gives no
reports either. ACTIVATE and STAGE_ACTIVATE were not run; they share
start_update() with STAGE.

No lock is added. A call that passed the removed check can run during
the unbind, against state and a parent device that are still valid; the
driver has never serialized calls from different files. An rwsem would
make the unbind wait for a firmware update to finish.

kzalloc() rather than kzalloc_obj(), which first appears in v7.0, for
the stable trees.

 drivers/acpi/pfr_update.c | 51 +++++++++++++++++++++++++++++++++++++--
 1 file changed, 49 insertions(+), 2 deletions(-)

diff --git a/drivers/acpi/pfr_update.c b/drivers/acpi/pfr_update.c
index 98ace679601b..b9e4c1065ad5 100644
--- a/drivers/acpi/pfr_update.c
+++ b/drivers/acpi/pfr_update.c
@@ -16,9 +16,11 @@
 #include <linux/file.h>
 #include <linux/fs.h>
 #include <linux/idr.h>
+#include <linux/kref.h>
 #include <linux/miscdevice.h>
 #include <linux/module.h>
 #include <linux/platform_device.h>
+#include <linux/slab.h>
 #include <linux/string.h>
 #include <linux/uaccess.h>
 #include <linux/uio.h>
@@ -82,6 +84,8 @@ struct pfru_device {
 	u32 rev_id, index;
 	struct device *parent_dev;
 	struct miscdevice miscdev;
+	struct kref kref;
+	bool removed;
 };
 
 static DEFINE_IDA(pfru_ida);
@@ -428,6 +432,9 @@ static long pfru_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
 	u32 rev;
 	int ret;
 
+	if (READ_ONCE(pfru_dev->removed))
+		return -ENODEV;
+
 	switch (cmd) {
 	case PFRU_IOC_QUERY_CAP:
 		ret = query_capability(&cap_hdr, pfru_dev);
@@ -476,6 +483,9 @@ static ssize_t pfru_write(struct file *file, const char __user *buf,
 	char *buf_ptr;
 	int ret;
 
+	if (READ_ONCE(pfru_dev->removed))
+		return -ENODEV;
+
 	ret = query_buffer(&buf_info, pfru_dev);
 	if (ret)
 		return ret;
@@ -518,8 +528,39 @@ static ssize_t pfru_write(struct file *file, const char __user *buf,
 	return ret ?: len;
 }
 
+static void pfru_free(struct kref *kref)
+{
+	struct pfru_device *pfru_dev =
+		container_of(kref, struct pfru_device, kref);
+
+	put_device(pfru_dev->parent_dev);
+	kfree(pfru_dev);
+}
+
+static void pfru_put(void *data)
+{
+	struct pfru_device *pfru_dev = data;
+
+	kref_put(&pfru_dev->kref, pfru_free);
+}
+
+/* misc_open() holds misc_mtx here, and misc_deregister() takes it. */
+static int pfru_open(struct inode *inode, struct file *file)
+{
+	kref_get(&to_pfru_dev(file)->kref);
+	return 0;
+}
+
+static int pfru_release(struct inode *inode, struct file *file)
+{
+	pfru_put(to_pfru_dev(file));
+	return 0;
+}
+
 static const struct file_operations acpi_pfru_fops = {
 	.owner		= THIS_MODULE,
+	.open		= pfru_open,
+	.release	= pfru_release,
 	.write		= pfru_write,
 	.unlocked_ioctl = pfru_ioctl,
 	.llseek		= noop_llseek,
@@ -530,6 +571,7 @@ static void acpi_pfru_remove(struct platform_device *pdev)
 	struct pfru_device *pfru_dev = platform_get_drvdata(pdev);
 
 	misc_deregister(&pfru_dev->miscdev);
+	WRITE_ONCE(pfru_dev->removed, true);
 }
 
 static void pfru_put_idx(void *data)
@@ -554,10 +596,16 @@ static int acpi_pfru_probe(struct platform_device *pdev)
 		return -ENODEV;
 	}
 
-	pfru_dev = devm_kzalloc(&pdev->dev, sizeof(*pfru_dev), GFP_KERNEL);
+	pfru_dev = kzalloc(sizeof(*pfru_dev), GFP_KERNEL);
 	if (!pfru_dev)
 		return -ENOMEM;
 
+	kref_init(&pfru_dev->kref);
+	pfru_dev->parent_dev = get_device(&pdev->dev);
+	ret = devm_add_action_or_reset(&pdev->dev, pfru_put, pfru_dev);
+	if (ret)
+		return ret;
+
 	ret = ida_alloc(&pfru_ida, GFP_KERNEL);
 	if (ret < 0)
 		return ret;
@@ -568,7 +616,6 @@ static int acpi_pfru_probe(struct platform_device *pdev)
 		return ret;
 
 	pfru_dev->rev_id = PFRU_DEFAULT_REV_ID;
-	pfru_dev->parent_dev = &pdev->dev;
 
 	pfru_dev->miscdev.minor = MISC_DYNAMIC_MINOR;
 	pfru_dev->miscdev.name = devm_kasprintf(&pdev->dev, GFP_KERNEL,
-- 
2.43.0