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

[Chad Aldreda <ca@modelmiser.com>]

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