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

Changes since v3:
- Patch 1/2: drop GFP_ATOMIC as suggested by Danilo. Measure the string length under rcu_read_lock(), allocate with GFP_KERNEL outside the RCU critical section, then re-read and copy with strscpy() under a second rcu_read_lock(). If the current string no longer fits the allocated buffer, retry with a PAGE_SIZE allocation (upper bound enforced by the write path).
- Patch 2/2: unchanged.
- KASAN stress testing with both fixes applied completed with 0 reports; the concurrent-writer reproducer produces ~3900 double-free reports without patch 2.

Danilo also suggested introducing a struct debugfs_string with explicit synchronization to provide callers with a proper synchronization contract. I agree that would address the broader API issue, and I'd be happy to work on it as a separate follow-up series if you think that would be useful.

v3: regenerate patches with git format-patch (v2 failed to apply).
v2: split into two patches, add Assisted-by, include KASAN splat.
v1: https://lore.kernel.org/driver-core/[email protected]/

Aldo Ariel Panzardo (2):
debugfs: fix use-after-free in debugfs_read_file_str()
debugfs: serialize concurrent writers in debugfs_write_file_str()

 fs/debugfs/file.c | 55 ++++++++++++++++++++++++++++++-----------------
 1 file changed, 35 insertions(+), 20 deletions(-)

base-commit: 6812ce4e4379ffc99c52401ec28f0d7ffbc36206 --
2.43.0

debugfs_write_file_str() publishes a new string pointer via rcu_assign_pointer(), waits for a grace period with synchronize_rcu(), then frees the old string.

debugfs_read_file_str() dereferences file->private_data without holding an RCU read-side critical section: it loads the pointer, calls strlen() on it, and then copies the string. If a concurrent writer completes synchronize_rcu() and kfree()s the old string between the load and the use, the reader accesses freed memory.

Fix by measuring the string length under rcu_read_lock(), allocating with GFP_KERNEL outside the critical section, and then copying with strscpy() under a second rcu_read_lock(). If the current string no longer fits the allocated buffer, retry with a PAGE_SIZE allocation which is the upper bound enforced by the write path.

---
 fs/debugfs/file.c | 39 +++++++++++++++++++++++++--------------
 1 file changed, 25 insertions(+), 14 deletions(-)

index 08de6652a..f9a1f7739 100644
@@ -1018,7 +1018,7 @@ ssize_t debugfs_read_file_str(struct file *file, char __user *user_buf,
 			      size_t count, loff_t *ppos)
 {
 	struct dentry *dentry = F_DENTRY(file);
-	char *str, *copy = NULL;
+	char *str, *copy;
 	int copy_len, len;
 	ssize_t ret;
 

@@ -1026,26 +1026,37 @@ ssize_t debugfs_read_file_str(struct file *file, char __user *user_buf,
 	if (unlikely(ret))
 		return ret;
 
-	str = *(char **)file->private_data;
-	len = strlen(str) + 1;
-	copy = kmalloc(len, GFP_KERNEL);
-	if (!copy) {
-		debugfs_file_put(dentry);
-		return -ENOMEM;
-	}
+	len = 0;
+	for (;;) {
+		rcu_read_lock();
+		str = rcu_dereference(*(char __rcu **)file->private_data);
+		len = max_t(int, strlen(str) + 1, len);
+		rcu_read_unlock();
+
+		copy = kmalloc(len, GFP_KERNEL);
+		if (!copy) {
+			debugfs_file_put(dentry);
+			return -ENOMEM;
+		}
+
+		rcu_read_lock();
+		str = rcu_dereference(*(char __rcu **)file->private_data);
+		copy_len = strscpy(copy, str, len);
+		rcu_read_unlock();
+
+		if (copy_len >= 0)
+			break;
 
-	copy_len = strscpy(copy, str, len);
-	debugfs_file_put(dentry);
-	if (copy_len < 0) {
 		kfree(copy);
-		return copy_len;
+		len = PAGE_SIZE;
 	}
 
-	copy[copy_len] = '\n';
+	debugfs_file_put(dentry);
 
+	copy[copy_len] = '\n';
+	len = copy_len + 1;
 	ret = simple_read_from_buffer(user_buf, count, ppos, copy, len);
 	kfree(copy);
-
 	return ret;
 }
 

2.43.0

Please don't use a LLM to write a changelog text without crediting it :(

Please see [1], where I listed a couple of alternatives (e.g. use GFP_NOWAIT and accept allocation failures). I'm not very opinionated about which of the options we use, but GFP_ATOMIC seems wrong for this.

Not sure if this was hit in the field, but in any case, the reader lacks the RCU read-side critical section required by the writer's reclamation.

As mentioned in [1], there is also a separate caller-side race in SoundWire [2]:
nothing prevents a concurrent write from freeing the string while request_firmware() is using it.

This is also why I think the API is a bit of a footgun. Fixing the debugfs reader won't address these caller-side accesses; something like the synchronized accessors suggested in [1] could address the broader API issue.

Thanks,
Danilo

[1] https://lore.kernel.org/driver-core/[email protected]/
[2] https://elixir.bootlin.com/linux/v7.2.7/source/drivers/soundwire/debugfs.c#L268

Ick, how about we just delete it then? I thought we had removed debugfs string functions already because of problems like this in the past...

thanks,

greg k-h

That's an option, but it seems people really seek for having a convinient API for this. And I think we can provide something that is "hard to get wrong" based on what I suggested, which I think is better than letting people open-code this.

To clarify, I did use Claude Code during the process, but only as an additional reviewer for spelling and grammar, minor rewording, formatting, and as a sanity check for obvious mistakes.
I wrote the patch and changelog myself, and the technical analysis, implementation, KASAN testing, reproducer, and verification were all done by me.
I should have been clearer about that when you asked earlier, apologies. Given that limited use, would you still prefer that I add an Assisted-by tag for the LLM in v5?

My background is security research -- I spend most of my time auditing code for memory safety issues and race conditions, among other things, which is how I ended up looking at this code after sashiko flagged the missing RCU protection.

Agreed. I'll follow Danilo's guidance from [1] and keep the implementation simple -- single allocation with GFP_NOWAIT, no retry.

The writable callers in mainline are:

drivers/interconnect/debugfs-client.c src_node/dst_node (0600)
drivers/soundwire/debugfs.c firmware_file (0200)

I am not aware of this being hit in the field. The race exists because the write path uses synchronize_rcu() + kfree() but the read path does not hold rcu_read_lock().

[1] https://lore.kernel.org/driver-core/[email protected]/

thanks,
Aldo

debugfs_write_file_str() reads the current string pointer into old, builds a replacement, publishes it with rcu_assign_pointer(), then frees old after synchronize_rcu(). There is no mutual exclusion between concurrent writers through the same inode: two simultaneous calls can both load the same old pointer before either publishes a replacement, and when both later call kfree(old), the result is a double-free.

Serialize writers with inode_lock(). Use rcu_dereference_protected() to load old under the held lock, satisfying lockdep annotation. Release the lock before synchronize_rcu() + kfree() so RCU readers are not stalled during the grace period.

Tested on QEMU/KVM (7.3.0-rc4 + KASAN). A reproducer that spawns concurrent write() calls on a debugfs string file triggers the double-free reliably (~3900 KASAN reports in a single run):

  BUG: KASAN: double-free in debugfs_write_file_str+0x194/0x200
  Free of addr ffff888104542ec0 by task poc/305

  Call Trace:
   kasan_report_invalid_free+0xb8/0xe0
   check_slab_allocation+0xd9/0x100
   kfree+0x163/0x510
   debugfs_write_file_str+0x194/0x200
   vfs_write+0x1ed/0x920
   ksys_write+0xe5/0x1a0

  Allocated by task 307:
   __kmalloc_noprof+0x264/0x680
   debugfs_write_file_str+0x106/0x200

  Freed by task 306:
   kfree+0x2e5/0x510
   debugfs_write_file_str+0x194/0x200

---
 fs/debugfs/file.c | 16 ++++++++++------
 1 file changed, 10 insertions(+), 6 deletions(-)

index f9a1f7739..7a9d33772 100644
@@ -1072,40 +1072,44 @@ static ssize_t debugfs_write_file_str(struct file *file, const char __user *user
 	if (unlikely(r))
 		return r;
 
-	old = *(char **)file->private_data;
+	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