iio: backend: fix uninitialized data in debugfs

PATCH In review Dan Carpenter 5 participants 25 messages May 25 – Jun 10

Subsystems (3)

Kernel janitors IIO IIO backend framework

lore

Share & tools

lore

Dan Carpenter

author

2026-05-25 07:16 UTC #0 lore raw

PATCH 1/1 iio: backend: fix uninitialized data in debugfs

If the *ppos value is non-zero then simple_write_to_buffer() will not initialize the start of the buf[] buffer. Non-zero ppos values aren't going to work at all. Check for that at the start of the function and return -EINVAL.

Fixes cdf01e0809a4 ("iio: backend: add debugFs interface")

Signed-off-by Dan Carpenter <[email protected]>

---
 drivers/iio/industrialio-backend.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

drivers/iio/industrialio-backend.c +1 -1

index 138ebebc9c0d..4763e224ebc6 100644
@@ -156,7 +156,7 @@ static ssize_t iio_backend_debugfs_write_reg(struct file *file,
 	ssize_t rc;
 	int ret;
 
-	if (count >= sizeof(buf))
+	if (*ppos != 0 || count >= sizeof(buf))
 		return -ENOSPC;
 
 	rc = simple_write_to_buffer(buf, sizeof(buf) - 1, ppos, userbuf, count);

Signature · 1 line

2.53.0

Maxwell Doose

2026-05-25 13:22 UTC #1 lore raw

Quoted 6 lines · Dan Carpenter

If the *ppos value is non-zero then simple_write_to_buffer() will not initialize the start of the buf[] buffer. Non-zero ppos values aren't going to work at all. Check for that at the start of the function and return -EINVAL.

Commit message is incorrect, it looks like you're returning -ENOSPC here...

Quoted 12 lines

Fixes cdf01e0809a4 ("iio: backend: add debugFs interface")
Signed-off-by Dan Carpenter <[email protected]>
---
 drivers/iio/industrialio-backend.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/iio/industrialio-backend.c +0 -0
index 138ebebc9c0d..4763e224ebc6 100644

drivers/iio/industrialio-backend.c static ssize_t iio_backend_debugfs_write_reg(struct file *file, · line 161

Quoted 7 lines

drivers/iio/industrialio-backend.c +1 -1

@@ -156,7 +156,7 @@ static ssize_t iio_backend_debugfs_write_reg(struct file *file,
        ssize_t rc;
        int ret;
 
-       if (count >= sizeof(buf))
+       if (*ppos != 0 || count >= sizeof(buf))
                return -ENOSPC;

See above comment.

best regards,
max

Quoted 6 lines

drivers/iio/industrialio-backend.c +0 -1

        rc = simple_write_to_buffer(buf, sizeof(buf) - 1, ppos, userbuf, count);
--

2.53.0

Jonathan Cameron

maintainer

2026-05-26 18:19 UTC #2 lore raw

Quoted 9 lines · On Mon, 25 May 2026 08:20:31 -0500 Maxwell Doose <[email protected]> wrote:

Quoted 6 lines · Dan Carpenter
If the *ppos value is non-zero then simple_write_to_buffer() will not initialize the start of the buf[] buffer. Non-zero ppos values aren't going to work at all. Check for that at the start of the function and return -EINVAL.
Commit message is incorrect, it looks like you're returning -ENOSPC here...

Tweaked and applied to the fixes-togreg branch of iio.git + marked for stable.

thanks,

Quoted 33 lines

Quoted 13 lines
Fixes cdf01e0809a4 ("iio: backend: add debugFs interface")
Signed-off-by Dan Carpenter <[email protected]>
---
 drivers/iio/industrialio-backend.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/iio/industrialio-backend.c +0 -0
index 138ebebc9c0d..4763e224ebc6 100644
drivers/iio/industrialio-backend.c static ssize_t iio_backend_debugfs_write_reg(struct file *file, · line 161
Quoted 7 lines
drivers/iio/industrialio-backend.c +1 -1
@@ -156,7 +156,7 @@ static ssize_t iio_backend_debugfs_write_reg(struct file *file,
        ssize_t rc;
        int ret;
 
-       if (count >= sizeof(buf))
+       if (*ppos != 0 || count >= sizeof(buf))
                return -ENOSPC;
 
See above comment.
best regards,
max
Quoted 6 lines
drivers/iio/industrialio-backend.c +0 -1
        rc = simple_write_to_buffer(buf, sizeof(buf) - 1, ppos, userbuf, count);
--
2.53.0

Andy Shevchenko

2026-06-04 07:30 UTC #3 lore raw

Quoted 11 lines · Jonathan Cameron

Quoted 8 lines · On Mon, 25 May 2026 08:20:31 -0500 Maxwell Doose <[email protected]> wrote:
Quoted 5 lines · Dan Carpenter
If the *ppos value is non-zero then simple_write_to_buffer() will not initialize the start of the buf[] buffer. Non-zero ppos values aren't going to work at all. Check for that at the start of the function and return -EINVAL.
Commit message is incorrect, it looks like you're returning -ENOSPC here...
Tweaked and applied to the fixes-togreg branch of iio.git + marked for stable.

I was stumbled over these patches by Dan. Since the file_operations for the code in question do not define .llseek, the seek is not supported and will return -ESPIPE. I'm not sure why we have these patches to begin with. Dan, can you elaborate on the case where the *ppos is not 0, please?

Signature · 2 lines

With Best Regards,
Andy Shevchenko

Andy Shevchenko

2026-06-04 07:38 UTC #4 lore raw

Quoted 10 lines · Andy Shevchenko

Quoted 9 lines · Jonathan Cameron
Quoted 7 lines · On Mon, 25 May 2026 08:20:31 -0500 Maxwell Doose <[email protected]> wrote:
Quoted 5 lines · Dan Carpenter
If the *ppos value is non-zero then simple_write_to_buffer() will not initialize the start of the buf[] buffer. Non-zero ppos values aren't going to work at all. Check for that at the start of the function and return -EINVAL.

Quoted 7 lines

Quoted 2 lines
Quoted 1 line
Commit message is incorrect, it looks like you're returning -ENOSPC here...
Tweaked and applied to the fixes-togreg branch of iio.git + marked for stable.
I was stumbled over these patches by Dan. Since the file_operations for the code in question do not define .llseek, the seek is not supported and will return -ESPIPE. I'm not sure why we have these patches to begin with. Dan, can you elaborate on the case where the *ppos is not 0, please?

FWIW, I also left a comment in that blog post.

Signature · 2 lines

With Best Regards,
Andy Shevchenko

Dan Carpenter

author

2026-06-04 07:45 UTC #5 lore raw

In reply to Andy Shevchenko

Quoted 10 lines · Andy Shevchenko

Quoted 9 lines · Jonathan Cameron
Quoted 7 lines · On Mon, 25 May 2026 08:20:31 -0500 Maxwell Doose <[email protected]> wrote:
Quoted 5 lines · Dan Carpenter
If the *ppos value is non-zero then simple_write_to_buffer() will not initialize the start of the buf[] buffer. Non-zero ppos values aren't going to work at all. Check for that at the start of the function and return -EINVAL.

Quoted 7 lines

Quoted 2 lines
Quoted 1 line
Commit message is incorrect, it looks like you're returning -ENOSPC here...
Tweaked and applied to the fixes-togreg branch of iio.git + marked for stable.
I was stumbled over these patches by Dan. Since the file_operations for the code in question do not define .llseek, the seek is not supported and will return -ESPIPE. I'm not sure why we have these patches to begin with. Dan, can you elaborate on the case where the *ppos is not 0, please?

The simple_write_to_buffer() will update *ppos so partial writes are supported in that way.

regards,
dan carpenter

Andy Shevchenko

2026-06-04 08:03 UTC #6 lore raw

Quoted 14 lines · Dan Carpenter

Quoted 13 lines · Andy Shevchenko
Quoted 11 lines · Jonathan Cameron
Quoted 8 lines · On Mon, 25 May 2026 08:20:31 -0500 Maxwell Doose <[email protected]> wrote:
Quoted 5 lines · Dan Carpenter
If the *ppos value is non-zero then simple_write_to_buffer() will not initialize the start of the buf[] buffer. Non-zero ppos values aren't going to work at all. Check for that at the start of the function and return -EINVAL.
Commit message is incorrect, it looks like you're returning -ENOSPC here...
Tweaked and applied to the fixes-togreg branch of iio.git + marked for stable.

Quoted 7 lines

Quoted 4 lines
I was stumbled over these patches by Dan. Since the file_operations for the code in question do not define .llseek, the seek is not supported and will return -ESPIPE. I'm not sure why we have these patches to begin with. Dan, can you elaborate on the case where the *ppos is not 0, please?
The simple_write_to_buffer() will update *ppos so partial writes are supported in that way.

Can you show the step-by-step scenario? I'm still fail to see how it may be happen. Is it somewhere inside the kernel loop? Which VFS function(s) is responsible for that in such a case?

Signature · 2 lines

With Best Regards,
Andy Shevchenko

Andy Shevchenko

2026-06-04 08:28 UTC #7 lore raw

Quoted 20 lines · Andy Shevchenko

Quoted 18 lines · Dan Carpenter
Quoted 17 lines · Andy Shevchenko
Quoted 11 lines · Jonathan Cameron
Quoted 8 lines · On Mon, 25 May 2026 08:20:31 -0500 Maxwell Doose <[email protected]> wrote:
Quoted 5 lines · Dan Carpenter
If the *ppos value is non-zero then simple_write_to_buffer() will not initialize the start of the buf[] buffer. Non-zero ppos values aren't going to work at all. Check for that at the start of the function and return -EINVAL.
Commit message is incorrect, it looks like you're returning -ENOSPC here...
Tweaked and applied to the fixes-togreg branch of iio.git + marked for stable.
I was stumbled over these patches by Dan. Since the file_operations for the code in question do not define .llseek, the seek is not supported and will return -ESPIPE. I'm not sure why we have these patches to begin with. Dan, can you elaborate on the case where the *ppos is not 0, please?

Quoted 6 lines

Quoted 2 lines
The simple_write_to_buffer() will update *ppos so partial writes are supported in that way.
Can you show the step-by-step scenario? I'm still fail to see how it may be happen. Is it somewhere inside the kernel loop? Which VFS function(s) is responsible for that in such a case?

Even if ppos is advanced, the simple_write_to_buffer()
https://elixir.bootlin.com/linux/v7.1-rc6/source/fs/libfs.c#L1188
won't write more than available in the buffer. Is the available also being advanced somehow?

Signature · 2 lines

With Best Regards,
Andy Shevchenko

Dan Carpenter

author

2026-06-04 10:38 UTC #8 lore raw

Quoted 24 lines · Andy Shevchenko

Quoted 23 lines · Andy Shevchenko
Quoted 21 lines · Dan Carpenter
Quoted 10 lines · Andy Shevchenko
Quoted 9 lines · Jonathan Cameron
Quoted 7 lines · On Mon, 25 May 2026 08:20:31 -0500 Maxwell Doose <[email protected]> wrote:
Quoted 5 lines · Dan Carpenter
If the *ppos value is non-zero then simple_write_to_buffer() will not initialize the start of the buf[] buffer. Non-zero ppos values aren't going to work at all. Check for that at the start of the function and return -EINVAL.
Quoted 7 lines
Quoted 2 lines
Quoted 1 line
Commit message is incorrect, it looks like you're returning -ENOSPC here...
Tweaked and applied to the fixes-togreg branch of iio.git + marked for stable.
I was stumbled over these patches by Dan. Since the file_operations for the code in question do not define .llseek, the seek is not supported and will return -ESPIPE. I'm not sure why we have these patches to begin with. Dan, can you elaborate on the case where the *ppos is not 0, please?
The simple_write_to_buffer() will update *ppos so partial writes are supported in that way.

Quoted 8 lines

Quoted 3 lines
Can you show the step-by-step scenario? I'm still fail to see how it may be happen. Is it somewhere inside the kernel loop? Which VFS function(s) is responsible for that in such a case?
Even if ppos is advanced, the simple_write_to_buffer()
https://elixir.bootlin.com/linux/v7.1-rc6/source/fs/libfs.c#L1188
won't write more than available in the buffer. Is the available also being advanced somehow?

It's not a buffer overflow, it's an uninitialized data bug.

I used Google AI to create a test case but my qemu system is arm64 and the test case is in assembly so I'm not sure how useful it is. (Also I modified the Google AI code and the test case is garbage). My test case writes 10 bytes of data at a time. I've attached the test debugfs kernel module.

Here is the code from the kernel:

drivers/iio/industrialio-backend.c
149 static ssize_t iio_backend_debugfs_write_reg(struct file *file,
150 const char __user *userbuf,
151 size_t count, loff_t *ppos)
152 {

   153          struct iio_backend *back = file->private_data;
   154          unsigned int val;
   155          char buf[80];

buf is uninitialized. In my test code, I initialized it to all U characters which stands for uninitialized.

156 ssize_t rc;
157 int ret;
158
159 if (*ppos != 0 || count >= sizeof(buf))

                    ^^^^^^^^^^

I added this check on *ppos but imagine it's not there and *ppos is non-zero.

   160                  return -ENOSPC;
   161  
   162          rc = simple_write_to_buffer(buf, sizeof(buf) - 1, ppos, userbuf, count);

The simple_write_to_buffer() function is designed to support partial writes so it leaves the first 0 to *ppos characters alone.

   163          if (rc < 0)
   164                  return rc;
   165  
   166          buf[rc] = '\0';

The return is the number of characters written (starting at *ppos). I'm doing 10 character writes so it is 10. The first 10 characters are uninitialized. In my test output you can see it prints ten U characters.

167

   168          ret = sscanf(buf, "%i %i", &back->cached_reg_addr, &val);
                             ^^^

Uninitialized variable.

169

root@test:/home/ubuntu/mnt/progs/tmp/sysfs# ./a.out 
Enter a line of text to save: 123456789012345678901234567890
[ 4266.853201] pos=0 count=10
[ 4266.853451] rc=10 buf 1234567890
[ 4266.854774] pos=10 count=10
Writing to file character by character...
[ 4266.858582] rc=10 buf UUUUUUUUUU
[ 4266.858698] pos=20 count=10
[ 4266.858740] rc=10 buf UUUUUUUUUU
[ 4266.858798] pos=30 count=10
[ 4266.858834] rc=10 buf UUUUUUUUUU
[ 4266.858888] pos=40 count=10
[ 4266.858924] rc=10 buf UUUUUUUUUU
[ 4266.859015] pos=50 count=10

regards,
dan carpenter

#include <linux/init.h>
#include <linux/module.h>
#include <linux/kernel.h>
#include <linux/debugfs.h>
#include <linux/fs.h>
#include <linux/uaccess.h>

MODULE_LICENSE("GPL");
MODULE_AUTHOR("Developer");
MODULE_DESCRIPTION("A Hello World Module with debugfs functionality"); MODULE_VERSION("1.0");

static struct dentry *dir_ret = NULL;
static struct dentry *file_ret = NULL;

// Triggered when a userspace process reads the debugfs file
static ssize_t hello_write(struct file *fp, const char __user *user_buffer, size_t count, loff_t *position)

{
	char buf[60] = "UUUUUUUUUUUUUUUUUUUUUUUUUUUUUUUUUUUUUUUUUUUUUUUUUUUUUUUUUUU";
	int rc;

	pr_info("pos=%lld count=%lu\n", *position, count);

	rc = simple_write_to_buffer(buf, sizeof(buf), position, user_buffer, count);
	if (rc < 0)
		return rc;
	buf[rc] = '\0';

	pr_info("rc=%d buf %s\n", rc, buf);

	return count;
}

// Map the custom read logic to file operations structure static const struct file_operations hello_fops = {
.owner = THIS_MODULE,
.write = hello_write,
};

// Module Initialization
static int __init hello_init(void)

{
	pr_info("hello_debugfs: Module loaded successfully\n");

	// 1. Create top-level parent directory in debugfs root
	dir_ret = debugfs_create_dir("hello_dir", NULL);
	if (IS_ERR(dir_ret)) {
		pr_err("hello_debugfs: Failed to create parent directory\n");
		return PTR_ERR(dir_ret);
	}

	// 2. Create target communication file inside the parent directory (Read-only for all: 0444)
	file_ret = debugfs_create_file("hello_file", 0444, dir_ret, NULL, &hello_fops);
	if (IS_ERR(file_ret)) {
		pr_err("hello_debugfs: Failed to create communication file\n");
		debugfs_remove_recursive(dir_ret);
		return PTR_ERR(file_ret);
	}

	return 0;
}

// Module Cleanup
static void __exit hello_exit(void)

{
	// Recursively destroys the created files and directory nodes
	debugfs_remove_recursive(dir_ret);
	pr_info("hello_debugfs: Module unloaded successfully\n");
}

module_init(hello_init);
module_exit(hello_exit);

Dan Carpenter

author

2026-06-04 10:42 UTC #9 lore raw

Quoted 3 lines · Dan Carpenter

   168          ret = sscanf(buf, "%i %i", &back->cached_reg_addr, &val);
                             ^^^

Uninitialized variable.

s/variable/data/.

regards,
dan carpenter

Andy Shevchenko

2026-06-04 14:55 UTC #10 lore raw

Quoted 6 lines · Dan Carpenter

Quoted 3 lines · Dan Carpenter
   168          ret = sscanf(buf, "%i %i", &back->cached_reg_addr, &val);
                             ^^^
Uninitialized variable.
s/variable/data/.

With what I asked in the previous reply and what you explained there (thanks, btw!) I still think your patches are not fully correct. They will require to atomically write all or nothing. If we want support partial writes we need to go with that differently (reset ppos when we got enough or more than enough data).

Signature · 2 lines

With Best Regards,
Andy Shevchenko

Dan Carpenter

author

2026-06-05 06:12 UTC #11 lore raw

Quoted 6 lines · Andy Shevchenko

Quoted 5 lines · Dan Carpenter
Quoted 3 lines · Dan Carpenter
   168          ret = sscanf(buf, "%i %i", &back->cached_reg_addr, &val);
                             ^^^
Uninitialized variable.

Quoted 7 lines

Quoted 1 line
s/variable/data/.
With what I asked in the previous reply and what you explained there (thanks, btw!) I still think your patches are not fully correct. They will require to atomically write all or nothing. If we want support partial writes we need to go with that differently (reset ppos when we got enough or more than enough data).

Requiring writes to syfs and debugfs be atomic is pretty normal and works well in practice. These are very small writes.

regards,
dan carpenter

Andy Shevchenko

2026-06-05 08:28 UTC #12 lore raw

Quoted 9 lines · Dan Carpenter

Quoted 8 lines · Andy Shevchenko
Quoted 6 lines · Dan Carpenter
Quoted 3 lines · Dan Carpenter
   168          ret = sscanf(buf, "%i %i", &back->cached_reg_addr, &val);
                             ^^^
Uninitialized variable.
s/variable/data/.

Quoted 8 lines

Quoted 5 lines
With what I asked in the previous reply and what you explained there (thanks, btw!) I still think your patches are not fully correct. They will require to atomically write all or nothing. If we want support partial writes we need to go with that differently (reset ppos when we got enough or more than enough data).
Requiring writes to syfs and debugfs be atomic is pretty normal and works well in practice. These are very small writes.

Perhaps. In any case your patch will break existing partial writes, right? I'm still considering that resetting ppos is the right thing to do. Just need to find where the best place is to do that.

Signature · 2 lines

With Best Regards,
Andy Shevchenko

Maxwell Doose

2026-06-05 14:40 UTC #13 lore raw

Quoted 17 lines · Andy Shevchenko

Quoted 15 lines · Dan Carpenter
Quoted 13 lines · Andy Shevchenko
Quoted 6 lines · Dan Carpenter
Quoted 3 lines · Dan Carpenter
   168          ret = sscanf(buf, "%i %i", &back->cached_reg_addr, &val);
                             ^^^
Uninitialized variable.
s/variable/data/.
With what I asked in the previous reply and what you explained there (thanks, btw!) I still think your patches are not fully correct. They will require to atomically write all or nothing. If we want support partial writes we need to go with that differently (reset ppos when we got enough or more than enough data).

Quoted 7 lines

Quoted 2 lines
Requiring writes to syfs and debugfs be atomic is pretty normal and works well in practice. These are very small writes.
Perhaps. In any case your patch will break existing partial writes, right? I'm still considering that resetting ppos is the right thing to do. Just need to find where the best place is to do that.

I'm certainly no expert on simple_write_to_buffer() but something in me tells me that writing in the middle of a buffer is a code smell. But you all seem much more informed in that regard so don't trust me :)

Nuno Sá

2026-06-08 20:30 UTC #14 lore raw

In reply to Andy Shevchenko

Quoted 16 lines · Andy Shevchenko

Quoted 15 lines · Dan Carpenter
Quoted 13 lines · Andy Shevchenko
Quoted 6 lines · Dan Carpenter
Quoted 3 lines · Dan Carpenter
168 ret = sscanf(buf, "%i %i", &back->cached_reg_addr, &val); ^^^
Uninitialized variable.
s/variable/data/.
With what I asked in the previous reply and what you explained there (thanks, btw!) I still think your patches are not fully correct. They will require to atomically write all or nothing. If we want support partial writes we need to go with that differently (reset ppos when we got enough or more than enough data).

Quoted 6 lines

Quoted 2 lines
Requiring writes to syfs and debugfs be atomic is pretty normal and works well in practice. These are very small writes.
Perhaps. In any case your patch will break existing partial writes, right? I'm still considering that resetting ppos is the right thing to do. Just need to find where the best place is to do that.

I think anyone doing partial writes on a debugfs interface like this one is very unlikely but it is a fair point, yes. But can't we be more relaxed on debugfs? No userspace app should be relying on debugfs in order to work (though I know that actually happens).

Anyways, this is one of those interesting edge cases and easy enough to get wrong. I guess we should either:

1. Improve simple_write_to_buffer() docs;
2. Or come up with a new simple_write_once_to_buffer() helper?

- Nuno Sá

Maxwell Doose

2026-06-08 21:51 UTC #15 lore raw

Quoted 29 lines · On Mon, 08 Jun 2026 21:31:14 +0100 Nuno Sá <[email protected]> wrote:

Quoted 16 lines · Andy Shevchenko
Quoted 15 lines · Dan Carpenter
Quoted 13 lines · Andy Shevchenko
Quoted 6 lines · Dan Carpenter
Quoted 3 lines · Dan Carpenter
168 ret = sscanf(buf, "%i %i", &back->cached_reg_addr, &val); ^^^
Uninitialized variable.
s/variable/data/.
With what I asked in the previous reply and what you explained there (thanks, btw!) I still think your patches are not fully correct. They will require to atomically write all or nothing. If we want support partial writes we need to go with that differently (reset ppos when we got enough or more than enough data).
Quoted 6 lines
Quoted 2 lines
Requiring writes to syfs and debugfs be atomic is pretty normal and works well in practice. These are very small writes.
Perhaps. In any case your patch will break existing partial writes, right? I'm still considering that resetting ppos is the right thing to do. Just need to find where the best place is to do that.
I think anyone doing partial writes on a debugfs interface like this one is very unlikely but it is a fair point, yes. But can't we be more relaxed on debugfs? No userspace app should be relying on debugfs in order to work (though I know that actually happens).

Quoted 6 lines

Anyways, this is one of those interesting edge cases and easy enough to get wrong. I guess we should either:
1. Improve simple_write_to_buffer() docs;
2. Or come up with a new simple_write_once_to_buffer() helper?

When you say "simple_write_once_to_buffer()" do you mean a wrapper around simple_write_to_buffer() that also checks if *ppos is 0 and returns -EINVAL if *ppos isn't 0? If so I can start writing that function. Though I guess we probably want to ask Jonathan about it first since obviously he'll have his own opinions.

Signature · 2 lines

best regards,
max

Nuno Sá

2026-06-09 08:09 UTC #16 lore raw

Quoted 39 lines · Maxwell Doose

Quoted 29 lines · On Mon, 08 Jun 2026 21:31:14 +0100 Nuno Sá <[email protected]> wrote:
Quoted 16 lines · Andy Shevchenko
Quoted 15 lines · Dan Carpenter
Quoted 13 lines · Andy Shevchenko
Quoted 6 lines · Dan Carpenter
Quoted 3 lines · Dan Carpenter
168 ret = sscanf(buf, "%i %i", &back->cached_reg_addr, &val); ^^^
Uninitialized variable.
s/variable/data/.
With what I asked in the previous reply and what you explained there (thanks, btw!) I still think your patches are not fully correct. They will require to atomically write all or nothing. If we want support partial writes we need to go with that differently (reset ppos when we got enough or more than enough data).
Quoted 6 lines
Quoted 2 lines
Requiring writes to syfs and debugfs be atomic is pretty normal and works well in practice. These are very small writes.
Perhaps. In any case your patch will break existing partial writes, right? I'm still considering that resetting ppos is the right thing to do. Just need to find where the best place is to do that.
I think anyone doing partial writes on a debugfs interface like this one is very unlikely but it is a fair point, yes. But can't we be more relaxed on debugfs? No userspace app should be relying on debugfs in order to work (though I know that actually happens).
Quoted 6 lines
Anyways, this is one of those interesting edge cases and easy enough to get wrong. I guess we should either:
1. Improve simple_write_to_buffer() docs;
2. Or come up with a new simple_write_once_to_buffer() helper?

Quoted 5 lines

When you say "simple_write_once_to_buffer()" do you mean a wrapper around simple_write_to_buffer() that also checks if *ppos is 0 and returns -EINVAL if *ppos isn't 0? If so I can start writing that function. Though I guess we probably want to ask Jonathan about it first since obviously he'll have his own opinions.

Yeah basically not allowing partial writes. Partial writes are also very tricky to handle in terms of concurrency (assuming userspace can do whatever they want). But I would not rush into writing the helper without understanding how people feel about it (at least the ones in this thread). Might not be super beneficial.

- Nuno Sá

Quoted 4 lines

Signature · 3 lines
-- 
best regards,
max

Andy Shevchenko

2026-06-09 12:16 UTC #17 lore raw

Quoted 45 lines · Nuno Sá

Quoted 38 lines · Maxwell Doose
Quoted 29 lines · On Mon, 08 Jun 2026 21:31:14 +0100 Nuno Sá <[email protected]> wrote:
Quoted 16 lines · Andy Shevchenko
Quoted 15 lines · Dan Carpenter
Quoted 13 lines · Andy Shevchenko
Quoted 6 lines · Dan Carpenter
Quoted 3 lines · Dan Carpenter
168 ret = sscanf(buf, "%i %i", &back->cached_reg_addr, &val); ^^^
Uninitialized variable.
s/variable/data/.
With what I asked in the previous reply and what you explained there (thanks, btw!) I still think your patches are not fully correct. They will require to atomically write all or nothing. If we want support partial writes we need to go with that differently (reset ppos when we got enough or more than enough data).
Quoted 6 lines
Quoted 2 lines
Requiring writes to syfs and debugfs be atomic is pretty normal and works well in practice. These are very small writes.
Perhaps. In any case your patch will break existing partial writes, right? I'm still considering that resetting ppos is the right thing to do. Just need to find where the best place is to do that.
I think anyone doing partial writes on a debugfs interface like this one is very unlikely but it is a fair point, yes. But can't we be more relaxed on debugfs? No userspace app should be relying on debugfs in order to work (though I know that actually happens).
Quoted 6 lines
Anyways, this is one of those interesting edge cases and easy enough to get wrong. I guess we should either:
1. Improve simple_write_to_buffer() docs;
2. Or come up with a new simple_write_once_to_buffer() helper?
Quoted 5 lines
When you say "simple_write_once_to_buffer()" do you mean a wrapper around simple_write_to_buffer() that also checks if *ppos is 0 and returns -EINVAL if *ppos isn't 0? If so I can start writing that function. Though I guess we probably want to ask Jonathan about it first since obviously he'll have his own opinions.

Quoted 5 lines

Yeah basically not allowing partial writes. Partial writes are also very tricky to handle in terms of concurrency (assuming userspace can do whatever they want). But I would not rush into writing the helper without understanding how people feel about it (at least the ones in this thread). Might not be super beneficial.

Yeah, I would expect a pushback. So, documentation is an appreciated updated, more — depends.

Signature · 2 lines

With Best Regards,
Andy Shevchenko

Jonathan Cameron

maintainer

2026-06-09 13:09 UTC #18 lore raw

In reply to Nuno Sá

Quoted 46 lines · On Tue, 9 Jun 2026 09:10:14 +0100 Nuno Sá <[email protected]> wrote:

Quoted 39 lines · Maxwell Doose
Quoted 29 lines · On Mon, 08 Jun 2026 21:31:14 +0100 Nuno Sá <[email protected]> wrote:
Quoted 16 lines · Andy Shevchenko
Quoted 15 lines · Dan Carpenter
Quoted 13 lines · Andy Shevchenko
Quoted 6 lines · Dan Carpenter
Quoted 3 lines · Dan Carpenter
168 ret = sscanf(buf, "%i %i", &back->cached_reg_addr, &val); ^^^
Uninitialized variable.
s/variable/data/.
With what I asked in the previous reply and what you explained there (thanks, btw!) I still think your patches are not fully correct. They will require to atomically write all or nothing. If we want support partial writes we need to go with that differently (reset ppos when we got enough or more than enough data).
Quoted 6 lines
Quoted 2 lines
Requiring writes to syfs and debugfs be atomic is pretty normal and works well in practice. These are very small writes.
Perhaps. In any case your patch will break existing partial writes, right? I'm still considering that resetting ppos is the right thing to do. Just need to find where the best place is to do that.
I think anyone doing partial writes on a debugfs interface like this one is very unlikely but it is a fair point, yes. But can't we be more relaxed on debugfs? No userspace app should be relying on debugfs in order to work (though I know that actually happens).
Quoted 6 lines
Anyways, this is one of those interesting edge cases and easy enough to get wrong. I guess we should either:
1. Improve simple_write_to_buffer() docs;
2. Or come up with a new simple_write_once_to_buffer() helper?
Quoted 5 lines
When you say "simple_write_once_to_buffer()" do you mean a wrapper around simple_write_to_buffer() that also checks if *ppos is 0 and returns -EINVAL if *ppos isn't 0? If so I can start writing that function. Though I guess we probably want to ask Jonathan about it first since obviously he'll have his own opinions.

Quoted 5 lines

Yeah basically not allowing partial writes. Partial writes are also very tricky to handle in terms of concurrency (assuming userspace can do whatever they want). But I would not rush into writing the helper without understanding how people feel about it (at least the ones in this thread). Might not be super beneficial.

I'd be fine with such a helper. No huge rush though as I don't plan to back out the patch that kicked off this long discussion.
Hence it would be an improvement on top of that. Dan probably is
the right person to offer a considered view on whether it is worth yet another helper.

Jonathan

Quoted 7 lines

- Nuno Sá
Quoted 4 lines
Signature · 3 lines
-- 
best regards,
max  

Dan Carpenter

author

2026-06-10 07:09 UTC #19 lore raw

In reply to Nuno Sá

Quoted 29 lines · Nuno Sá

Quoted 16 lines · Andy Shevchenko
Quoted 15 lines · Dan Carpenter
Quoted 13 lines · Andy Shevchenko
Quoted 6 lines · Dan Carpenter
Quoted 3 lines · Dan Carpenter
168 ret = sscanf(buf, "%i %i", &back->cached_reg_addr, &val); ^^^
Uninitialized variable.
s/variable/data/.
With what I asked in the previous reply and what you explained there (thanks, btw!) I still think your patches are not fully correct. They will require to atomically write all or nothing. If we want support partial writes we need to go with that differently (reset ppos when we got enough or more than enough data).
Quoted 6 lines
Quoted 2 lines
Requiring writes to syfs and debugfs be atomic is pretty normal and works well in practice. These are very small writes.
Perhaps. In any case your patch will break existing partial writes, right? I'm still considering that resetting ppos is the right thing to do. Just need to find where the best place is to do that.
I think anyone doing partial writes on a debugfs interface like this one is very unlikely but it is a fair point, yes. But can't we be more relaxed on debugfs? No userspace app should be relying on debugfs in order to work (though I know that actually happens).

Quoted 5 lines

Anyways, this is one of those interesting edge cases and easy enough to get wrong. I guess we should either:
1. Improve simple_write_to_buffer() docs;
2. Or come up with a new simple_write_once_to_buffer() helper?

The right function is copy_from_user(). The point of simple_write_to_buffer() is to support partial writes and in many cases that just doesn't make sense.

regards,
dan carpenter

Dan Carpenter

author

2026-06-10 07:08 UTC #20 lore raw

In reply to Andy Shevchenko

Quoted 16 lines · Andy Shevchenko

Quoted 15 lines · Dan Carpenter
Quoted 13 lines · Andy Shevchenko
Quoted 6 lines · Dan Carpenter
Quoted 3 lines · Dan Carpenter
   168          ret = sscanf(buf, "%i %i", &back->cached_reg_addr, &val);
                             ^^^
Uninitialized variable.
s/variable/data/.
With what I asked in the previous reply and what you explained there (thanks, btw!) I still think your patches are not fully correct. They will require to atomically write all or nothing. If we want support partial writes we need to go with that differently (reset ppos when we got enough or more than enough data).

Quoted 4 lines

Quoted 2 lines
Requiring writes to syfs and debugfs be atomic is pretty normal and works well in practice. These are very small writes.
Perhaps. In any case your patch will break existing partial writes, right?

partial writes never worked so this doesn't break anything that wasn't already broken. Another option I considered was just initializing the buffer, but returning -EINVAL is the right fix.

regards,
dan carpenter

Andy Shevchenko

2026-06-10 10:41 UTC #21 lore raw

Quoted 17 lines · Dan Carpenter

Quoted 16 lines · Andy Shevchenko
Quoted 15 lines · Dan Carpenter
Quoted 13 lines · Andy Shevchenko
Quoted 6 lines · Dan Carpenter
Quoted 3 lines · Dan Carpenter
   168          ret = sscanf(buf, "%i %i", &back->cached_reg_addr, &val);
                             ^^^
Uninitialized variable.
s/variable/data/.
With what I asked in the previous reply and what you explained there (thanks, btw!) I still think your patches are not fully correct. They will require to atomically write all or nothing. If we want support partial writes we need to go with that differently (reset ppos when we got enough or more than enough data).

Quoted 6 lines

Quoted 4 lines
Quoted 2 lines
Requiring writes to syfs and debugfs be atomic is pretty normal and works well in practice. These are very small writes.
Perhaps. In any case your patch will break existing partial writes, right?
partial writes never worked

I don't get this. Does it mean it never worked at all, or in this particular case? Also "never worked" means the same issue you pointed out (uninitialised buffer) or something else?

Quoted 3 lines

so this doesn't break anything that wasn't
already broken. Another option I considered was just initializing the buffer, but returning -EINVAL is the right fix.

Signature · 2 lines

With Best Regards,
Andy Shevchenko

Dan Carpenter

author

2026-06-10 12:19 UTC #22 lore raw

Quoted 21 lines · Andy Shevchenko

Quoted 20 lines · Dan Carpenter
Quoted 19 lines · Andy Shevchenko
Quoted 17 lines · Dan Carpenter
Quoted 6 lines · Andy Shevchenko
Quoted 5 lines · Dan Carpenter
Quoted 3 lines · Dan Carpenter
   168          ret = sscanf(buf, "%i %i", &back->cached_reg_addr, &val);
                             ^^^
Uninitialized variable.
Quoted 7 lines
Quoted 1 line
s/variable/data/.
With what I asked in the previous reply and what you explained there (thanks, btw!) I still think your patches are not fully correct. They will require to atomically write all or nothing. If we want support partial writes we need to go with that differently (reset ppos when we got enough or more than enough data).
Requiring writes to syfs and debugfs be atomic is pretty normal and works well in practice. These are very small writes.

Quoted 7 lines

Quoted 3 lines
Quoted 1 line
Perhaps. In any case your patch will break existing partial writes, right?
partial writes never worked
I don't get this. Does it mean it never worked at all, or in this particular case? Also "never worked" means the same issue you pointed out (uninitialised buffer) or something else?

I mean in this case. The patch doesn't introduce a regression because partial writes were already broken.

regards,
dan carpenter

Andy Shevchenko

2026-06-10 13:44 UTC #23 lore raw

Quoted 24 lines · Dan Carpenter

Quoted 23 lines · Andy Shevchenko
Quoted 22 lines · Dan Carpenter
Quoted 20 lines · Andy Shevchenko
Quoted 9 lines · Dan Carpenter
Quoted 8 lines · Andy Shevchenko
Quoted 6 lines · Dan Carpenter
Quoted 3 lines · Dan Carpenter
   168          ret = sscanf(buf, "%i %i", &back->cached_reg_addr, &val);
                             ^^^
Uninitialized variable.
s/variable/data/.
Quoted 8 lines
Quoted 5 lines
With what I asked in the previous reply and what you explained there (thanks, btw!) I still think your patches are not fully correct. They will require to atomically write all or nothing. If we want support partial writes we need to go with that differently (reset ppos when we got enough or more than enough data).
Requiring writes to syfs and debugfs be atomic is pretty normal and works well in practice. These are very small writes.
Perhaps. In any case your patch will break existing partial writes, right?

Quoted 8 lines

Quoted 5 lines
Quoted 1 line
partial writes never worked
I don't get this. Does it mean it never worked at all, or in this particular case? Also "never worked" means the same issue you pointed out (uninitialised buffer) or something else?
I mean in this case. The patch doesn't introduce a regression because partial writes were already broken.

OK, now clear.

Signature · 2 lines

With Best Regards,
Andy Shevchenko

Andy Shevchenko

2026-06-04 14:52 UTC #24 lore raw

In reply to Dan Carpenter

Quoted 64 lines · Dan Carpenter

Quoted 24 lines · Andy Shevchenko
Quoted 23 lines · Andy Shevchenko
Quoted 21 lines · Dan Carpenter
Quoted 10 lines · Andy Shevchenko
Quoted 9 lines · Jonathan Cameron
Quoted 7 lines · On Mon, 25 May 2026 08:20:31 -0500 Maxwell Doose <[email protected]> wrote:
Quoted 5 lines · Dan Carpenter
If the *ppos value is non-zero then simple_write_to_buffer() will not initialize the start of the buf[] buffer. Non-zero ppos values aren't going to work at all. Check for that at the start of the function and return -EINVAL.
Quoted 7 lines
Quoted 2 lines
Quoted 1 line
Commit message is incorrect, it looks like you're returning -ENOSPC here...
Tweaked and applied to the fixes-togreg branch of iio.git + marked for stable.
I was stumbled over these patches by Dan. Since the file_operations for the code in question do not define .llseek, the seek is not supported and will return -ESPIPE. I'm not sure why we have these patches to begin with. Dan, can you elaborate on the case where the *ppos is not 0, please?
The simple_write_to_buffer() will update *ppos so partial writes are supported in that way.
Quoted 8 lines
Quoted 3 lines
Can you show the step-by-step scenario? I'm still fail to see how it may be happen. Is it somewhere inside the kernel loop? Which VFS function(s) is responsible for that in such a case?
Even if ppos is advanced, the simple_write_to_buffer()
https://elixir.bootlin.com/linux/v7.1-rc6/source/fs/libfs.c#L1188
won't write more than available in the buffer. Is the available also being advanced somehow?
It's not a buffer overflow, it's an uninitialized data bug.
I used Google AI to create a test case but my qemu system is arm64 and the test case is in assembly so I'm not sure how useful it is. (Also I modified the Google AI code and the test case is garbage). My test case writes 10 bytes of data at a time. I've attached the test debugfs kernel module.
Here is the code from the kernel:
drivers/iio/industrialio-backend.c
149 static ssize_t iio_backend_debugfs_write_reg(struct file *file,
150 const char __user *userbuf,
151 size_t count, loff_t *ppos)
   152  {
   153          struct iio_backend *back = file->private_data;
   154          unsigned int val;
   155          char buf[80];
buf is uninitialized. In my test code, I initialized it to all U characters which stands for uninitialized.
   156          ssize_t rc;
   157          int ret;
   158  
   159          if (*ppos != 0 || count >= sizeof(buf))
                    ^^^^^^^^^^
I added this check on *ppos but imagine it's not there and *ppos is non-zero.

Quoted 6 lines

   160                  return -ENOSPC;
   161  
   162          rc = simple_write_to_buffer(buf, sizeof(buf) - 1, ppos, userbuf, count);
The simple_write_to_buffer() function is designed to support partial writes so it leaves the first 0 to *ppos characters alone.

If you wrote as many bytes as required, *ppos left on the 'count', and next writes go after that. So, why *ppos is not 0? Maybe the issue that we need somewhere reset *ppos to 0?

Quoted 30 lines

   163          if (rc < 0)
   164                  return rc;
   165  
   166          buf[rc] = '\0';

   167  
   168          ret = sscanf(buf, "%i %i", &back->cached_reg_addr, &val);
                             ^^^

Uninitialized variable.

# ./a.out 
Enter a line of text to save: 123456789012345678901234567890
[ 4266.853201] pos=0 count=10
[ 4266.853451] rc=10 buf 1234567890
[ 4266.854774] pos=10 count=10
Writing to file character by character...
[ 4266.858582] rc=10 buf UUUUUUUUUU
[ 4266.858698] pos=20 count=10
[ 4266.858740] rc=10 buf UUUUUUUUUU
[ 4266.858798] pos=30 count=10
[ 4266.858834] rc=10 buf UUUUUUUUUU
[ 4266.858888] pos=40 count=10
[ 4266.858924] rc=10 buf UUUUUUUUUU
[ 4266.859015] pos=50 count=10

Signature · 2 lines

With Best Regards,
Andy Shevchenko