[PATCH v3 0/3] debugfs: make debugfs_create_str() read-only

[PATCH v3 0/3] debugfs: make debugfs_create_str() read-only

[Yichong Chen]

debugfs_create_str() has a generic write implementation that replaces the
backing string.  Concurrent writers can race and free the same old string
twice.

Instead of adding more locking to the generic helper, convert the existing
writable in-tree users to local file operations and make
debugfs_create_str() read-only.

Changes since v2:
- Use scoped mutex guards in the interconnect and SoundWire conversions.
- Clarify why GFP_KERNEL is safe in the interconnect conversion after the
  RCU read-side critical section is removed.
- Drop the unnecessary firmware_file = NULL assignment in the SoundWire
  exit path.
- Use WARN() instead of WARN_ONCE() so each writable debugfs_create_str()
  caller can be reported.

Changes since v1:
- Follow Greg's suggestion to avoid adding locking to the generic
  debugfs_create_str() write path.
- Convert the existing writable in-tree users to local file operations.
- Make debugfs_create_str() read-only and refuse writable modes.
- Split the change into a 3-patch series.

Yichong Chen (3):
  interconnect: debugfs: replace writable string helper
  soundwire: debugfs: replace writable string helper
  debugfs: make debugfs_create_str() read-only

 drivers/interconnect/debugfs-client.c | 81 +++++++++++++++++++++------
 drivers/soundwire/debugfs.c           | 63 ++++++++++++++++++---
 fs/debugfs/file.c                     | 81 ++++-----------------------
 3 files changed, 129 insertions(+), 96 deletions(-)

-- 
2.51.0

[PATCH v3 1/3] interconnect: debugfs: replace writable string helper

[Yichong Chen]

debugfs_create_str() is being made read-only because its generic write
path is hard to make safe without adding more locking to the helper.

Convert the interconnect debugfs client src_node and dst_node entries to
local file operations before removing writable string support from
debugfs_create_str().  Protect the string replacement and path lookup with
the existing debugfs_lock.

The old code duplicated the strings under rcu_read_lock(), so it had to use
GFP_ATOMIC.  The local file operations protect src_node and dst_node with
debugfs_lock instead, so the allocation can use GFP_KERNEL.

Signed-off-by: Yichong Chen <chenyichong@uniontech.com>
---
 drivers/interconnect/debugfs-client.c | 81 +++++++++++++++++++++------
 1 file changed, 64 insertions(+), 17 deletions(-)

diff --git a/drivers/interconnect/debugfs-client.c b/drivers/interconnect/debugfs-client.c
index 08df9188ef94..c587fbf519c6 100644
--- a/drivers/interconnect/debugfs-client.c
+++ b/drivers/interconnect/debugfs-client.c
@@ -5,6 +5,7 @@
 #include <linux/debugfs.h>
 #include <linux/interconnect.h>
 #include <linux/platform_device.h>
+#include <linux/slab.h>
 
 #include "internal.h"
 
@@ -36,6 +37,59 @@ struct debugfs_path {
 	struct list_head list;
 };
 
+static ssize_t icc_node_read(struct file *file, char __user *user_buf,
+			     size_t count, loff_t *ppos)
+{
+	char **node = file->private_data;
+	char *copy;
+	size_t len;
+	ssize_t ret;
+
+	scoped_guard(mutex, &debugfs_lock) {
+		copy = kstrdup(*node ?: "", GFP_KERNEL);
+	}
+	if (!copy)
+		return -ENOMEM;
+
+	len = strlen(copy);
+	copy[len++] = '\n';
+	ret = simple_read_from_buffer(user_buf, count, ppos, copy, len);
+	kfree(copy);
+	return ret;
+}
+
+static ssize_t icc_node_write(struct file *file, const char __user *user_buf,
+			      size_t count, loff_t *ppos)
+{
+	char **node = file->private_data;
+	char *old, *new;
+
+	if (*ppos)
+		return -EINVAL;
+	if (count + 1 > PAGE_SIZE)
+		return -E2BIG;
+
+	new = memdup_user_nul(user_buf, count);
+	if (IS_ERR(new))
+		return PTR_ERR(new);
+	strim(new);
+
+	scoped_guard(mutex, &debugfs_lock) {
+		old = *node;
+		*node = new;
+	}
+
+	kfree(old);
+	return count;
+}
+
+static const struct file_operations icc_node_fops = {
+	.open = simple_open,
+	.read = icc_node_read,
+	.write = icc_node_write,
+	.llseek = default_llseek,
+};
+
 static struct icc_path *get_path(const char *src, const char *dst)
 {
 	struct debugfs_path *path;
@@ -54,26 +108,19 @@ static int icc_get_set(void *data, u64 val)
 	char *src, *dst;
 	int ret = 0;
 
-	mutex_lock(&debugfs_lock);
-
-	rcu_read_lock();
-	src = rcu_dereference(src_node);
-	dst = rcu_dereference(dst_node);
+	guard(mutex)(&debugfs_lock);
 
 	/*
 	 * If we've already looked up a path, then use the existing one instead
 	 * of calling icc_get() again. This allows for updating previous BW
 	 * votes when "get" is written to multiple times for multiple paths.
 	 */
-	cur_path = get_path(src, dst);
-	if (cur_path) {
-		rcu_read_unlock();
+	cur_path = get_path(src_node, dst_node);
+	if (cur_path)
 		goto out;
-	}
 
-	src = kstrdup(src, GFP_ATOMIC);
-	dst = kstrdup(dst, GFP_ATOMIC);
-	rcu_read_unlock();
+	src = kstrdup(src_node, GFP_KERNEL);
+	dst = kstrdup(dst_node, GFP_KERNEL);
 
 	if (!src || !dst) {
 		ret = -ENOMEM;
@@ -105,7 +152,6 @@ static int icc_get_set(void *data, u64 val)
 	kfree(src);
 	kfree(dst);
 out:
-	mutex_unlock(&debugfs_lock);
 	return ret;
 }
 
@@ -115,7 +161,7 @@ static int icc_commit_set(void *data, u64 val)
 {
 	int ret;
 
-	mutex_lock(&debugfs_lock);
+	guard(mutex)(&debugfs_lock);
 
 	if (!cur_path) {
 		ret = -EINVAL;
@@ -130,7 +176,6 @@ static int icc_commit_set(void *data, u64 val)
 	icc_set_tag(cur_path, tag);
 	ret = icc_set_bw(cur_path, avg_bw, peak_bw);
 out:
-	mutex_unlock(&debugfs_lock);
 	return ret;
 }
 
@@ -160,8 +205,10 @@ int icc_debugfs_client_init(struct dentry *icc_dir)
 
 	client_dir = debugfs_create_dir("test_client", icc_dir);
 
-	debugfs_create_str("src_node", 0600, client_dir, &src_node);
-	debugfs_create_str("dst_node", 0600, client_dir, &dst_node);
+	debugfs_create_file("src_node", 0600, client_dir, &src_node,
+			    &icc_node_fops);
+	debugfs_create_file("dst_node", 0600, client_dir, &dst_node,
+			    &icc_node_fops);
 	debugfs_create_file("get", 0200, client_dir, NULL, &icc_get_fops);
 	debugfs_create_u32("avg_bw", 0600, client_dir, &avg_bw);
 	debugfs_create_u32("peak_bw", 0600, client_dir, &peak_bw);
-- 
2.51.0

[PATCH v3 2/3] soundwire: debugfs: replace writable string helper

[Yichong Chen]

debugfs_create_str() is being made read-only because its generic write
path is hard to make safe without adding more locking to the helper.

Convert the SoundWire firmware_file entry to a local write-only file
operation before removing writable string support from
debugfs_create_str().

Copy the firmware name before request_firmware() so later debugfs writes
cannot replace the string while it is being used.

Signed-off-by: Yichong Chen <chenyichong@uniontech.com>
---
 drivers/soundwire/debugfs.c | 63 ++++++++++++++++++++++++++++++++-----
 1 file changed, 55 insertions(+), 8 deletions(-)

diff --git a/drivers/soundwire/debugfs.c b/drivers/soundwire/debugfs.c
index 099eb84a548e..a8a6db164fe7 100644
--- a/drivers/soundwire/debugfs.c
+++ b/drivers/soundwire/debugfs.c
@@ -145,6 +145,38 @@ static u32 start_addr;
 static size_t num_bytes;
 static u8 read_buffer[MAX_CMD_BYTES];
 static char *firmware_file;
+static DEFINE_MUTEX(firmware_file_lock);
+
+static ssize_t firmware_file_write(struct file *file,
+				   const char __user *user_buf,
+				   size_t count, loff_t *ppos)
+{
+	char *old, *new;
+
+	if (*ppos)
+		return -EINVAL;
+	if (count + 1 > PAGE_SIZE)
+		return -E2BIG;
+
+	new = memdup_user_nul(user_buf, count);
+	if (IS_ERR(new))
+		return PTR_ERR(new);
+	strim(new);
+
+	scoped_guard(mutex, &firmware_file_lock) {
+		old = firmware_file;
+		firmware_file = new;
+	}
+
+	kfree(old);
+	return count;
+}
+
+static const struct file_operations firmware_file_fops = {
+	.open = simple_open,
+	.write = firmware_file_write,
+	.llseek = default_llseek,
+};
 
 static int set_command(void *data, u64 value)
 {
@@ -246,6 +278,7 @@ static int cmd_go(void *data, u64 value)
 {
 	const struct firmware *fw = NULL;
 	struct sdw_slave *slave = data;
+	char *fw_name = NULL;
 	ktime_t start_t;
 	ktime_t finish_t;
 	int ret;
@@ -265,15 +298,24 @@ static int cmd_go(void *data, u64 value)
 	}
 
 	if (cmd == 0) {
-		ret = request_firmware(&fw, firmware_file, &slave->dev);
+		scoped_guard(mutex, &firmware_file_lock) {
+			if (firmware_file)
+				fw_name = kstrdup(firmware_file, GFP_KERNEL);
+		}
+		if (!fw_name) {
+			ret = -ENOMEM;
+			goto out;
+		}
+
+		ret = request_firmware(&fw, fw_name, &slave->dev);
 		if (ret < 0) {
-			dev_err(&slave->dev, "firmware %s not found\n", firmware_file);
+			dev_err(&slave->dev, "firmware %s not found\n", fw_name);
 			goto out;
 		}
 		if (fw->size < num_bytes) {
 			dev_err(&slave->dev,
 				"firmware %s: firmware size %zd, desired %zd\n",
-				firmware_file, fw->size, num_bytes);
+				fw_name, fw->size, num_bytes);
 			goto out;
 		}
 	}
@@ -306,6 +348,7 @@ static int cmd_go(void *data, u64 value)
 out:
 	if (fw)
 		release_firmware(fw);
+	kfree(fw_name);
 
 	pm_runtime_mark_last_busy(&slave->dev);
 	pm_runtime_put(&slave->dev);
@@ -358,7 +401,8 @@ void sdw_slave_debugfs_init(struct sdw_slave *slave)
 
 	debugfs_create_file("read_buffer", 0400, d, slave, &read_buffer_fops);
 	if (firmware_file)
-		debugfs_create_str("firmware_file", 0200, d, &firmware_file);
+		debugfs_create_file("firmware_file", 0200, d, NULL,
+				    &firmware_file_fops);
 
 	slave->debugfs = d;
 }
@@ -370,8 +414,10 @@ void sdw_slave_debugfs_exit(struct sdw_slave *slave)
 
 void sdw_debugfs_init(void)
 {
-	if (!firmware_file)
-		firmware_file = kstrdup("", GFP_KERNEL);
+	scoped_guard(mutex, &firmware_file_lock) {
+		if (!firmware_file)
+			firmware_file = kstrdup("", GFP_KERNEL);
+	}
 
 	sdw_debugfs_root = debugfs_create_dir("soundwire", NULL);
 }
@@ -379,6 +425,7 @@ void sdw_debugfs_init(void)
 void sdw_debugfs_exit(void)
 {
 	debugfs_remove_recursive(sdw_debugfs_root);
-	kfree(firmware_file);
-	firmware_file = NULL;
+	scoped_guard(mutex, &firmware_file_lock) {
+		kfree(firmware_file);
+	}
 }
-- 
2.51.0

[PATCH v3 3/3] debugfs: make debugfs_create_str() read-only

[Yichong Chen]

debugfs_create_str() supports replacing the backing string from userspace.
Concurrent writers can race and free the same old string twice.

All writable in-tree users have been converted to local file operations.
Remove the generic write support from debugfs_create_str(), and refuse to
create a file when the caller passes write permission bits.

This makes unsupported writable use visible instead of silently creating a
file with different permissions.

Fixes: 86b5488121db ("debugfs: Add write support to debugfs_create_str()")
Signed-off-by: Yichong Chen <chenyichong@uniontech.com>
---
 fs/debugfs/file.c | 81 ++++++-----------------------------------------
 1 file changed, 10 insertions(+), 71 deletions(-)

diff --git a/fs/debugfs/file.c b/fs/debugfs/file.c
index 08de6652a4f3..170feb75317f 100644
--- a/fs/debugfs/file.c
+++ b/fs/debugfs/file.c
@@ -1049,98 +1049,37 @@ ssize_t debugfs_read_file_str(struct file *file, char __user *user_buf,
 	return ret;
 }
 
-static ssize_t debugfs_write_file_str(struct file *file, const char __user *user_buf,
-				      size_t count, loff_t *ppos)
-{
-	struct dentry *dentry = F_DENTRY(file);
-	char *old, *new = NULL;
-	int pos = *ppos;
-	int r;
-
-	r = debugfs_file_get(dentry);
-	if (unlikely(r))
-		return r;
-
-	old = *(char **)file->private_data;
-
-	/* only allow strict concatenation */
-	r = -EINVAL;
-	if (pos && pos != strlen(old))
-		goto error;
-
-	r = -E2BIG;
-	if (pos + count + 1 > PAGE_SIZE)
-		goto error;
-
-	r = -ENOMEM;
-	new = kmalloc(pos + count + 1, GFP_KERNEL);
-	if (!new)
-		goto error;
-
-	if (pos)
-		memcpy(new, old, pos);
-
-	r = -EFAULT;
-	if (copy_from_user(new + pos, user_buf, count))
-		goto error;
-
-	new[pos + count] = '\0';
-	strim(new);
-
-	rcu_assign_pointer(*(char __rcu **)file->private_data, new);
-	synchronize_rcu();
-	kfree(old);
-
-	debugfs_file_put(dentry);
-	return count;
-
-error:
-	kfree(new);
-	debugfs_file_put(dentry);
-	return r;
-}
-
 static const struct file_operations fops_str = {
 	.read =		debugfs_read_file_str,
-	.write =	debugfs_write_file_str,
-	.open =		simple_open,
-	.llseek =	default_llseek,
-};
-
-static const struct file_operations fops_str_ro = {
-	.read =		debugfs_read_file_str,
-	.open =		simple_open,
-	.llseek =	default_llseek,
-};
-
-static const struct file_operations fops_str_wo = {
-	.write =	debugfs_write_file_str,
 	.open =		simple_open,
 	.llseek =	default_llseek,
 };
 
 /**
- * debugfs_create_str - create a debugfs file that is used to read and write a string value
+ * debugfs_create_str - create a debugfs file that is used to read a string value
  * @name: a pointer to a string containing the name of the file to create.
  * @mode: the permission that the file should have
  * @parent: a pointer to the parent dentry for this file.  This should be a
  *          directory dentry if set.  If this parameter is %NULL, then the
  *          file will be created in the root of the debugfs filesystem.
- * @value: a pointer to the variable that the file should read to and write
- *         from. This pointer and the string it points to must not be %NULL.
+ * @value: a pointer to the variable that the file should read from. This
+ *         pointer and the string it points to must not be %NULL.
  *
  * This function creates a file in debugfs with the given name that
- * contains the value of the variable @value.  If the @mode variable is so
- * set, it can be read from, and written to.
+ * contains the value of the variable @value.  The file can be read from.
+ * Writable files are not supported; if @mode contains write permission bits,
+ * no file is created.
  */
 void debugfs_create_str(const char *name, umode_t mode,
 			struct dentry *parent, char **value)
 {
 	if (WARN_ON(!value || !*value))
 		return;
+	if (WARN(mode & 0222,
+		 "%s() does not support writable files\n", __func__))
+		return;
 
-	debugfs_create_mode_unsafe(name, mode, parent, value, &fops_str,
-				   &fops_str_ro, &fops_str_wo);
+	debugfs_create_file_unsafe(name, mode, parent, value, &fops_str);
 }
 EXPORT_SYMBOL_GPL(debugfs_create_str);
 
-- 
2.51.0