[PATCH] debugfs: fix UAF and double-free in debugfs_str read/write

[Aldo Ariel Panzardo <qwe.aldo@gmail.com>]

debugfs_write_file_str() stores the new string via rcu_assign_pointer()
and frees the old one after synchronize_rcu().  However,
debugfs_read_file_str() dereferences file->private_data without holding
an RCU read-side critical section, so synchronize_rcu() in a concurrent
writer can return—and kfree() the old pointer—before the reader finishes
using it (use-after-free).

Additionally, two concurrent writers both read the same `old` pointer
before either calls rcu_assign_pointer().  When both subsequently call
kfree(old) a double-free results.

Fix both issues:

 - Read path: wrap the pointer dereference and string copy inside
   rcu_read_lock()/rcu_read_unlock(), using rcu_dereference() and
   GFP_ATOMIC to keep the allocation within the critical section.

 - Write path: serialize concurrent writers with inode_lock(), and use
   rcu_dereference_protected() to read `old` under the held lock.  The
   lock is released before synchronize_rcu()+kfree() so readers are not
   blocked during the grace period.

Cc: stable@vger.kernel.org
Signed-off-by: Aldo Ariel Panzardo <qwe.aldo@gmail.com>
---
 fs/debugfs/file.c | 50 ++++++++++++++++++++++++++--------------------
 1 file changed, 28 insertions(+), 22 deletions(-)

--- a/fs/debugfs/file.c
+++ b/fs/debugfs/file.c
@@ -1018,34 +1018,32 @@
 			      size_t count, loff_t *ppos)
 {
 	struct dentry *dentry = F_DENTRY(file);
-	char *str, *copy = NULL;
-	int copy_len, len;
+	char *str, *copy;
+	int len;
 	ssize_t ret;
 
 	ret = debugfs_file_get(dentry);
 	if (unlikely(ret))
 		return ret;
 
-	str = *(char **)file->private_data;
+	/*
+	 * Pair with rcu_assign_pointer() in debugfs_write_file_str().
+	 * Without this critical section, synchronize_rcu() in a concurrent
+	 * writer can complete and free the string before we finish reading it.
+	 */
+	rcu_read_lock();
+	str = rcu_dereference(*(char __rcu **)file->private_data);
 	len = strlen(str) + 1;
-	copy = kmalloc(len, GFP_KERNEL);
-	if (!copy) {
-		debugfs_file_put(dentry);
-		return -ENOMEM;
-	}
+	copy = kmemdup(str, len, GFP_ATOMIC);
+	rcu_read_unlock();
 
-	copy_len = strscpy(copy, str, len);
 	debugfs_file_put(dentry);
-	if (copy_len < 0) {
-		kfree(copy);
-		return copy_len;
-	}
-
-	copy[copy_len] = '\n';
+	if (!copy)
+		return -ENOMEM;
 
+	copy[len - 1] = '\n';
 	ret = simple_read_from_buffer(user_buf, count, ppos, copy, len);
 	kfree(copy);
-
 	return ret;
 }
 
@@ -1061,40 +1059,48 @@
 	if (unlikely(r))
 		return r;
 
-	old = *(char **)file->private_data;
+	/*
+	 * Serialize concurrent writers: without this lock two simultaneous
+	 * calls can both read the same 'old' pointer and double-free it.
+	 */
+	inode_lock(file_inode(file));
+	old = rcu_dereference_protected(*(char __rcu **)file->private_data,
+					lockdep_is_held(&file_inode(file)->i_rwsem));
 
 	/* only allow strict concatenation */
 	r = -EINVAL;
 	if (pos && pos != strlen(old))
-		goto error;
+		goto unlock;
 
 	r = -E2BIG;
 	if (pos + count + 1 > PAGE_SIZE)
-		goto error;
+		goto unlock;
 
 	r = -ENOMEM;
 	new = kmalloc(pos + count + 1, GFP_KERNEL);
 	if (!new)
-		goto error;
+		goto unlock;
 
 	if (pos)
 		memcpy(new, old, pos);
 
 	r = -EFAULT;
 	if (copy_from_user(new + pos, user_buf, count))
-		goto error;
+		goto unlock;
 
 	new[pos + count] = '\0';
 	strim(new);
 
 	rcu_assign_pointer(*(char __rcu **)file->private_data, new);
+	inode_unlock(file_inode(file));
 	synchronize_rcu();
 	kfree(old);
 
 	debugfs_file_put(dentry);
 	return count;
 
-error:
+unlock:
+	inode_unlock(file_inode(file));
 	kfree(new);
 	debugfs_file_put(dentry);
 	return r;

-- 
2.43.0