[PATCH] ACPI: Make custom_method use per-open state

* [PATCH] ACPI: Make custom_method use per-open state
@ 2023-01-31 23:37 Pedro Falcato
  2023-02-01 18:34 ` Rafael J. Wysocki
  0 siblings, 1 reply; 9+ messages in thread
From: Pedro Falcato @ 2023-01-31 23:37 UTC (permalink / raw)
  To: linux-acpi
  Cc: Rafael J . Wysocki, Len Brown, linux-kernel, rui.zhang,
	Pedro Falcato, Hang Zhang, Swift Geek

Make custom_method keep its own per-file-open state instead of global
state in order to avoid race conditions[1] and other possible conflicts
with other concurrent users.

Link: https://lore.kernel.org/linux-acpi/20221227063335.61474-1-zh.nvgt@gmail.com/ # [1]
Reported-by: Hang Zhang <zh.nvgt@gmail.com>
Cc: Swift Geek <swiftgeek@gmail.com>
Signed-off-by: Pedro Falcato <pedro.falcato@gmail.com>
---
 This patch addresses Hang's problems plus the ones raised by Rafael in his review (see link above).
 https://lore.kernel.org/lkml/2667007.mvXUDI8C0e@kreacher/ was submitted but since there were still people
 that wanted this feature, I took my time to write up a patch that should fix the issues.
 Hopefully the linux-acpi maintainers have not decided to remove custom_method just yet.

 drivers/acpi/custom_method.c | 119 +++++++++++++++++++++++++++--------
 1 file changed, 92 insertions(+), 27 deletions(-)

diff --git a/drivers/acpi/custom_method.c b/drivers/acpi/custom_method.c
index d39a9b47472..034fb14f118 100644
--- a/drivers/acpi/custom_method.c
+++ b/drivers/acpi/custom_method.c
@@ -17,73 +17,138 @@ MODULE_LICENSE("GPL");
 
 static struct dentry *cm_dentry;
 
+struct custom_method_state {
+	char *buf;
+	u32 max_size;
+	u32 uncopied_bytes;
+	struct mutex lock;
+};
+
+static int cm_open(struct inode *inode, struct file *file)
+{
+	struct custom_method_state *state;
+
+	state = kzalloc(sizeof(struct custom_method_state), GFP_KERNEL);
+
+	if (!state)
+		return -ENOMEM;
+
+	file->private_data = state;
+	mutex_init(&state->lock);
+
+	return 0;
+}
+
+static int cm_release(struct inode *inode, struct file *file)
+{
+	struct custom_method_state *state;
+
+	state = file->private_data;
+
+	mutex_destroy(&state->lock);
+
+	/* Make sure the buf gets freed */
+	kfree(state->buf);
+
+	kfree(state);
+	return 0;
+}
+
 /* /sys/kernel/debug/acpi/custom_method */
 
 static ssize_t cm_write(struct file *file, const char __user *user_buf,
 			size_t count, loff_t *ppos)
 {
-	static char *buf;
-	static u32 max_size;
-	static u32 uncopied_bytes;
+	struct custom_method_state *state;
+	char *buf;
 
 	struct acpi_table_header table;
 	acpi_status status;
 	int ret;
 
+	state = file->private_data;
+	buf = state->buf;
+
 	ret = security_locked_down(LOCKDOWN_ACPI_TABLES);
 	if (ret)
 		return ret;
 
+	mutex_lock(&state->lock);
+
 	if (!(*ppos)) {
 		/* parse the table header to get the table length */
-		if (count <= sizeof(struct acpi_table_header))
-			return -EINVAL;
+		if (count <= sizeof(struct acpi_table_header)) {
+			count = -EINVAL;
+			goto out;
+		}
+
 		if (copy_from_user(&table, user_buf,
-				   sizeof(struct acpi_table_header)))
-			return -EFAULT;
-		uncopied_bytes = max_size = table.length;
+				   sizeof(struct acpi_table_header))) {
+			count = -EFAULT;
+			goto out;
+		}
+
+		state->uncopied_bytes = state->max_size = table.length;
 		/* make sure the buf is not allocated */
 		kfree(buf);
-		buf = kzalloc(max_size, GFP_KERNEL);
-		if (!buf)
-			return -ENOMEM;
+		buf = state->buf = kzalloc(state->max_size, GFP_KERNEL);
+		if (!buf) {
+			count = -ENOMEM;
+			goto out;
+		}
 	}
 
-	if (buf == NULL)
-		return -EINVAL;
+	/* Check if someone seeked ahead or if we errored out
+	 * (buf will be NULL)
+	 */
+	if (buf == NULL) {
+		count = -EINVAL;
+		goto out;
+	}
 
-	if ((*ppos > max_size) ||
-	    (*ppos + count > max_size) ||
+	if ((*ppos > state->max_size) ||
+	    (*ppos + count > state->max_size) ||
 	    (*ppos + count < count) ||
-	    (count > uncopied_bytes)) {
-		kfree(buf);
-		buf = NULL;
-		return -EINVAL;
+	    (count > state->uncopied_bytes)) {
+		count = -EINVAL;
+		goto err_free;
 	}
 
 	if (copy_from_user(buf + (*ppos), user_buf, count)) {
-		kfree(buf);
-		buf = NULL;
-		return -EFAULT;
+		count = -EFAULT;
+		goto err_free;
 	}
 
-	uncopied_bytes -= count;
+	state->uncopied_bytes -= count;
 	*ppos += count;
 
-	if (!uncopied_bytes) {
+	if (!state->uncopied_bytes) {
 		status = acpi_install_method(buf);
 		kfree(buf);
-		buf = NULL;
-		if (ACPI_FAILURE(status))
-			return -EINVAL;
+		state->buf = NULL;
+
+		if (ACPI_FAILURE(status)) {
+			count = -EINVAL;
+			goto out;
+		}
+
 		add_taint(TAINT_OVERRIDDEN_ACPI_TABLE, LOCKDEP_NOW_UNRELIABLE);
 	}
 
+out:
+	mutex_unlock(&state->lock);
+	return count;
+err_free:
+	mutex_unlock(&state->lock);
+	kfree(buf);
+	state->buf = NULL;
 	return count;
 }
 
 static const struct file_operations cm_fops = {
 	.write = cm_write,
+	.open = cm_open,
+	.release = cm_release,
 	.llseek = default_llseek,
 };
 
-- 
2.39.0


^ permalink raw reply related	[flat|nested] 9+ messages in thread