[PATCH] ACPI: pfr_update: wait for running calls in remove()

[Chad Aldreda <ca@modelmiser.com>]

Commit dacb7cef6f32 ("ACPI: pfr_update: keep the state until the last
file is closed") makes ioctl() and write() fail with -ENODEV once
remove() has run, but a call that started before remove() keeps running
after it returns. Such a call can evaluate _DSM while device_del()
unbinds the parent's ACPI companion, and after an unbind and bind it
acts on the firmware behind the new instance's back. Its message says
such a call "finishes against state that is still valid"; that was
wrong.

Run the firmware calls of ioctl() and write() under a new mutex, and set
the removed flag under it in remove(), so that remove() waits for calls
already running. The mutex also serializes rev_id and the use of the
shared communication buffer, which concurrent calls on different files
could interleave. User memory is copied outside the lock, so a page
fault that does not complete cannot hold up remove(): write() now copies
the capsule into a kernel buffer first, and a fault there returns
-EFAULT rather than -EINVAL. It queries the buffer again once it holds
the lock, since SET_REV may have run in between. An unbind during a
capsule stage or activation waits for it to finish, rather than
returning with the update still in progress.

The race was pointed out by Sashiko's review of the matching
pfr_telemetry patch [1]. The fix and this message were written with an
AI assistant (Claude) and checked by hand, and 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://sashiko.dev/#/patchset/20261108233900.4043941-3-ca%40modelmiser.com [1]
Fixes: dacb7cef6f32 ("ACPI: pfr_update: keep the state until the last file is closed")
Cc: stable@vger.kernel.org
Assisted-by: Claude:claude-opus-5-5
Signed-off-by: Chad Aldreda <ca@modelmiser.com>
---
This goes on top of commit dacb7cef6f32 in linux-pm, which Rafael
applied for 7.5. The matching pfr_telemetry change is in v2 of that
patch:
https://lore.kernel.org/r/20261110144436.1529000-2-ca@modelmiser.com

As in pfr_telemetry, user memory is copied outside the lock. For
write() that means a kernel copy of the capsule, of up to the firmware
buffer size, so a slow or stalled user fault no longer holds the lock.

Tested as for the pfr_telemetry v2 patch, with the SSDT device _HID
INTC1080. QUERY_CAP, SET_REV, write() of a minimal capsule and STAGE
work before a sysfs unbind and fail with -ENODEV after it on a file
held open, the same holds after a bind, and there are no KASAN or
lockdep reports. With a test-only msleep(2000) before the QUERY_CAP
firmware call and the driver unbound 0.5 s into the call, the unbind
returned at once without this patch and the firmware call completed
1.5 s after remove() returned; with it the unbind returned 2.0 s in,
after the call completed. ACTIVATE and STAGE_ACTIVATE were not run,
and neither was removal of the device itself.

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_update.c | 86 ++++++++++++++++++++++++++-------------
 1 file changed, 58 insertions(+), 28 deletions(-)

diff --git a/drivers/acpi/pfr_update.c b/drivers/acpi/pfr_update.c
index 797956d9defb..73cde535b34b 100644
--- a/drivers/acpi/pfr_update.c
+++ b/drivers/acpi/pfr_update.c
@@ -19,11 +19,11 @@
 #include <linux/kref.h>
 #include <linux/miscdevice.h>
 #include <linux/module.h>
+#include <linux/mutex.h>
 #include <linux/platform_device.h>
 #include <linux/slab.h>
 #include <linux/string.h>
 #include <linux/uaccess.h>
-#include <linux/uio.h>
 #include <linux/uuid.h>
 
 #include <uapi/linux/pfrut.h>
@@ -85,6 +85,12 @@ struct pfru_device {
 	struct device *parent_dev;
 	struct miscdevice miscdev;
 	struct kref kref;
+	/*
+	 * Serializes the firmware calls, rev_id and the communication buffer
+	 * of the file operations, and remove(), which sets removed under it
+	 * and so waits for calls already running.
+	 */
+	struct mutex lock;
 	bool removed;
 };
 
@@ -424,6 +430,15 @@ static int start_update(int action, struct pfru_device *pfru_dev)
 	return ret;
 }
 
+static int pfru_start_update(int action, struct pfru_device *pfru_dev)
+{
+	guard(mutex)(&pfru_dev->lock);
+	if (pfru_dev->removed)
+		return -ENODEV;
+
+	return start_update(action, pfru_dev);
+}
+
 static long pfru_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
 {
 	struct pfru_update_cap_info cap_hdr = {};
@@ -432,12 +447,14 @@ 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);
+		scoped_guard(mutex, &pfru_dev->lock) {
+			if (pfru_dev->removed)
+				return -ENODEV;
+
+			ret = query_capability(&cap_hdr, pfru_dev);
+		}
 		if (ret)
 			return ret;
 
@@ -453,18 +470,23 @@ static long pfru_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
 		if (!pfru_valid_revid(rev))
 			return -EINVAL;
 
-		pfru_dev->rev_id = rev;
+		scoped_guard(mutex, &pfru_dev->lock) {
+			if (pfru_dev->removed)
+				return -ENODEV;
+
+			pfru_dev->rev_id = rev;
+		}
 
 		return 0;
 
 	case PFRU_IOC_STAGE:
-		return start_update(START_STAGE, pfru_dev);
+		return pfru_start_update(START_STAGE, pfru_dev);
 
 	case PFRU_IOC_ACTIVATE:
-		return start_update(START_ACTIVATE, pfru_dev);
+		return pfru_start_update(START_ACTIVATE, pfru_dev);
 
 	case PFRU_IOC_STAGE_ACTIVATE:
-		return start_update(START_STAGE_ACTIVATE, pfru_dev);
+		return pfru_start_update(START_STAGE_ACTIVATE, pfru_dev);
 
 	default:
 		return -ENOTTY;
@@ -478,14 +500,33 @@ static ssize_t pfru_write(struct file *file, const char __user *buf,
 	struct pfru_update_cap_info cap;
 	struct pfru_com_buf_info buf_info;
 	phys_addr_t phy_addr;
-	struct iov_iter iter;
-	struct iovec iov;
 	char *buf_ptr;
 	int ret;
 
-	if (READ_ONCE(pfru_dev->removed))
+	scoped_guard(mutex, &pfru_dev->lock) {
+		if (pfru_dev->removed)
+			return -ENODEV;
+
+		ret = query_buffer(&buf_info, pfru_dev);
+	}
+	if (ret)
+		return ret;
+
+	if (len > buf_info.buf_size) {
+		dev_dbg(pfru_dev->parent_dev, "Capsule image size too large\n");
+		return -EINVAL;
+	}
+
+	/* Copy from the user outside the lock, which remove() waits for */
+	void *data __free(kvfree) = vmemdup_user(buf, len);
+	if (IS_ERR(data))
+		return PTR_ERR(data);
+
+	guard(mutex)(&pfru_dev->lock);
+	if (pfru_dev->removed)
 		return -ENODEV;
 
+	/* Query again: SET_REV may have run while the lock was dropped */
 	ret = query_buffer(&buf_info, pfru_dev);
 	if (ret)
 		return ret;
@@ -495,10 +536,6 @@ static ssize_t pfru_write(struct file *file, const char __user *buf,
 		return -EINVAL;
 	}
 
-	iov.iov_base = (void __user *)buf;
-	iov.iov_len = len;
-	iov_iter_init(&iter, ITER_SOURCE, &iov, 1, len);
-
 	/* map the communication buffer */
 	phy_addr = (phys_addr_t)((buf_info.addr_hi << 32) | buf_info.addr_lo);
 	buf_ptr = memremap(phy_addr, buf_info.buf_size, MEMREMAP_WB);
@@ -507,22 +544,13 @@ static ssize_t pfru_write(struct file *file, const char __user *buf,
 		return -ENOMEM;
 	}
 
-	if (!copy_from_iter_full(buf_ptr, len, &iter)) {
-		dev_dbg(pfru_dev->parent_dev,
-			"Failed to copy the data from the user space buffer\n");
-		ret = -EINVAL;
-		goto unmap;
-	}
+	memcpy(buf_ptr, data, len);
 
 	/* check if the capsule header has a valid version number */
 	ret = query_capability(&cap, pfru_dev);
-	if (ret)
-		goto unmap;
-
-	if (!applicable_image(buf_ptr, &cap, pfru_dev))
+	if (!ret && !applicable_image(buf_ptr, &cap, pfru_dev))
 		ret = -EINVAL;
 
-unmap:
 	memunmap(buf_ptr);
 
 	return ret ?: len;
@@ -571,7 +599,8 @@ 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);
+	scoped_guard(mutex, &pfru_dev->lock)
+		pfru_dev->removed = true;
 }
 
 static void pfru_put_idx(void *data)
@@ -601,6 +630,7 @@ static int acpi_pfru_probe(struct platform_device *pdev)
 		return -ENOMEM;
 
 	kref_init(&pfru_dev->kref);
+	mutex_init(&pfru_dev->lock);
 	pfru_dev->parent_dev = get_device(&pdev->dev);
 	ret = devm_add_action_or_reset(&pdev->dev, pfru_put, pfru_dev);
 	if (ret)
-- 
2.43.0