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

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

[Aldo Ariel Panzardo]

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/20260925175831.3701812-1-qwe.aldo@gmail.com/

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

[PATCH v4 1/2] debugfs: fix use-after-free in debugfs_read_file_str()

[Aldo Ariel Panzardo]

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.

Cc: stable@vger.kernel.org
Assisted-by: sashiko.dev <sashiko-bot@kernel.org>
Signed-off-by: Aldo Ariel Panzardo <qwe.aldo@gmail.com>
---
 fs/debugfs/file.c | 39 +++++++++++++++++++++++++--------------
 1 file changed, 25 insertions(+), 14 deletions(-)

diff --git a/fs/debugfs/file.c b/fs/debugfs/file.c
index 08de6652a..f9a1f7739 100644
--- a/fs/debugfs/file.c
+++ b/fs/debugfs/file.c
@@ -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

[PATCH v4 2/2] debugfs: serialize concurrent writers in debugfs_write_file_str()

[Aldo Ariel Panzardo]

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

Cc: stable@vger.kernel.org
Assisted-by: sashiko.dev <sashiko-bot@kernel.org>
Signed-off-by: Aldo Ariel Panzardo <qwe.aldo@gmail.com>
---
 fs/debugfs/file.c | 16 ++++++++++------
 1 file changed, 10 insertions(+), 6 deletions(-)

diff --git a/fs/debugfs/file.c b/fs/debugfs/file.c
index f9a1f7739..7a9d33772 100644
--- a/fs/debugfs/file.c
+++ b/fs/debugfs/file.c
@@ -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

Re: [PATCH v4 1/2] debugfs: fix use-after-free in debugfs_read_file_str()

[Greg KH]

On Sat, Sep 26, 2026 at 04:58:43PM -0300, Aldo Ariel Panzardo wrote:
> 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.

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

> 
> Cc: stable@vger.kernel.org
> Assisted-by: sashiko.dev <sashiko-bot@kernel.org>
> Signed-off-by: Aldo Ariel Panzardo <qwe.aldo@gmail.com>
> ---
>  fs/debugfs/file.c | 39 +++++++++++++++++++++++++--------------
>  1 file changed, 25 insertions(+), 14 deletions(-)
> 
> diff --git a/fs/debugfs/file.c b/fs/debugfs/file.c
> index 08de6652a..f9a1f7739 100644
> --- a/fs/debugfs/file.c
> +++ b/fs/debugfs/file.c
> @@ -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 (;;) {

This is probably not right, don't do loops like this.  Either fail or
succeed, don't loop.

Again, let's see the real use case here, what is hitting this in
userspace today and what debugfs kernel files are causing it?

thanks,

greg k-h

Re: [PATCH v4 1/2] debugfs: fix use-after-free in debugfs_read_file_str()

[Danilo Krummrich]

On Sun Sep 27, 2026 at 6:37 PM CEST, Greg KH wrote:
> This is probably not right, don't do loops like this.  Either fail or
> succeed, don't loop.

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.

> Again, let's see the real use case here, what is hitting this in
> userspace today and what debugfs kernel files are causing it?

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/DLPDB44JJRGJ.3K6JNS746M7QC@kernel.org/
[2] https://elixir.bootlin.com/linux/v7.2.7/source/drivers/soundwire/debugfs.c#L268

Re: [PATCH v4 1/2] debugfs: fix use-after-free in debugfs_read_file_str()

[Aldo Ariel Panzardo]

On Sun, Sep 27, 2026 at 06:37:40PM +0200, Greg Kroah-Hartman wrote:
> Please don't use a LLM to write a changelog text without crediting it :(

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.

> This is probably not right, don't do loops like this.  Either fail or
> succeed, don't loop.

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

> Again, let's see the real use case here, what is hitting this in
> userspace today and what debugfs kernel files are causing it?

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/DLPDB44JJRGJ.3K6JNS746M7QC@kernel.org/

thanks,
Aldo

Re: [PATCH v4 1/2] debugfs: fix use-after-free in debugfs_read_file_str()

[Greg KH]

On Sun, Sep 27, 2026 at 07:00:04PM +0200, Danilo Krummrich wrote:
> On Sun Sep 27, 2026 at 6:37 PM CEST, Greg KH wrote:
> > This is probably not right, don't do loops like this.  Either fail or
> > succeed, don't loop.
> 
> 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.
> 
> > Again, let's see the real use case here, what is hitting this in
> > userspace today and what debugfs kernel files are causing it?
> 
> 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.

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

Re: [PATCH v4 1/2] debugfs: fix use-after-free in debugfs_read_file_str()

[Danilo Krummrich]

On Mon Sep 28, 2026 at 7:35 AM CEST, Greg KH wrote:
> On Sun, Sep 27, 2026 at 07:00:04PM +0200, Danilo Krummrich wrote:
>> On Sun Sep 27, 2026 at 6:37 PM CEST, Greg KH wrote:
>> > This is probably not right, don't do loops like this.  Either fail or
>> > succeed, don't loop.
>> 
>> 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.
>> 
>> > Again, let's see the real use case here, what is hitting this in
>> > userspace today and what debugfs kernel files are causing it?
>> 
>> 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.
>
> 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...

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.