All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] iio: backend: fix uninitialized data in debugfs
@ 2026-05-25  7:16 Dan Carpenter
  2026-05-25 13:20 ` Maxwell Doose
  0 siblings, 1 reply; 15+ messages in thread
From: Dan Carpenter @ 2026-05-25  7:16 UTC (permalink / raw)
  To: Nuno Sa
  Cc: Olivier Moysan, Jonathan Cameron, David Lechner, Andy Shevchenko,
	linux-iio, linux-kernel, kernel-janitors

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 <error27@gmail.com>
---
 drivers/iio/industrialio-backend.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/iio/industrialio-backend.c b/drivers/iio/industrialio-backend.c
index 138ebebc9c0d..4763e224ebc6 100644
--- a/drivers/iio/industrialio-backend.c
+++ b/drivers/iio/industrialio-backend.c
@@ -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);
-- 
2.53.0


^ permalink raw reply related	[flat|nested] 15+ messages in thread

* Re: [PATCH] iio: backend: fix uninitialized data in debugfs
  2026-05-25  7:16 [PATCH] iio: backend: fix uninitialized data in debugfs Dan Carpenter
@ 2026-05-25 13:20 ` Maxwell Doose
  2026-05-26 18:19   ` Jonathan Cameron
  0 siblings, 1 reply; 15+ messages in thread
From: Maxwell Doose @ 2026-05-25 13:20 UTC (permalink / raw)
  To: Dan Carpenter
  Cc: Nuno Sa, Olivier Moysan, Jonathan Cameron, David Lechner,
	Andy Shevchenko, linux-iio, linux-kernel, kernel-janitors

On Mon, May 25, 2026 at 2:17 AM Dan Carpenter <error27@gmail.com> wrote:
>
> 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...

>
> Fixes: cdf01e0809a4 ("iio: backend: add debugFs interface")
> Signed-off-by: Dan Carpenter <error27@gmail.com>
> ---
>  drivers/iio/industrialio-backend.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/iio/industrialio-backend.c b/drivers/iio/industrialio-backend.c
> index 138ebebc9c0d..4763e224ebc6 100644
> --- a/drivers/iio/industrialio-backend.c
> +++ b/drivers/iio/industrialio-backend.c
> @@ -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


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

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH] iio: backend: fix uninitialized data in debugfs
  2026-05-25 13:20 ` Maxwell Doose
@ 2026-05-26 18:19   ` Jonathan Cameron
  2026-06-04  7:30     ` Andy Shevchenko
  0 siblings, 1 reply; 15+ messages in thread
From: Jonathan Cameron @ 2026-05-26 18:19 UTC (permalink / raw)
  To: Maxwell Doose
  Cc: Dan Carpenter, Nuno Sa, Olivier Moysan, David Lechner,
	Andy Shevchenko, linux-iio, linux-kernel, kernel-janitors

On Mon, 25 May 2026 08:20:31 -0500
Maxwell Doose <m32285159@gmail.com> wrote:

> On Mon, May 25, 2026 at 2:17 AM Dan Carpenter <error27@gmail.com> wrote:
> >
> > 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,

J
> 
> >
> > Fixes: cdf01e0809a4 ("iio: backend: add debugFs interface")
> > Signed-off-by: Dan Carpenter <error27@gmail.com>
> > ---
> >  drivers/iio/industrialio-backend.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/iio/industrialio-backend.c b/drivers/iio/industrialio-backend.c
> > index 138ebebc9c0d..4763e224ebc6 100644
> > --- a/drivers/iio/industrialio-backend.c
> > +++ b/drivers/iio/industrialio-backend.c
> > @@ -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
> 
> 
> >
> >         rc = simple_write_to_buffer(buf, sizeof(buf) - 1, ppos, userbuf, count);
> > --
> > 2.53.0
> >
> >  


^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH] iio: backend: fix uninitialized data in debugfs
  2026-05-26 18:19   ` Jonathan Cameron
@ 2026-06-04  7:30     ` Andy Shevchenko
  2026-06-04  7:38       ` Andy Shevchenko
  2026-06-04  7:45       ` Dan Carpenter
  0 siblings, 2 replies; 15+ messages in thread
From: Andy Shevchenko @ 2026-06-04  7:30 UTC (permalink / raw)
  To: Jonathan Cameron
  Cc: Maxwell Doose, Dan Carpenter, Nuno Sa, Olivier Moysan,
	David Lechner, Andy Shevchenko, linux-iio, linux-kernel,
	kernel-janitors

On Tue, May 26, 2026 at 07:19:46PM +0100, Jonathan Cameron wrote:
> On Mon, 25 May 2026 08:20:31 -0500
> Maxwell Doose <m32285159@gmail.com> wrote:
> > On Mon, May 25, 2026 at 2:17 AM Dan Carpenter <error27@gmail.com> wrote:
> > >
> > > 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?

-- 
With Best Regards,
Andy Shevchenko



^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH] iio: backend: fix uninitialized data in debugfs
  2026-06-04  7:30     ` Andy Shevchenko
@ 2026-06-04  7:38       ` Andy Shevchenko
  2026-06-04  7:45       ` Dan Carpenter
  1 sibling, 0 replies; 15+ messages in thread
From: Andy Shevchenko @ 2026-06-04  7:38 UTC (permalink / raw)
  To: Jonathan Cameron
  Cc: Maxwell Doose, Dan Carpenter, Nuno Sa, Olivier Moysan,
	David Lechner, Andy Shevchenko, linux-iio, linux-kernel,
	kernel-janitors

On Thu, Jun 04, 2026 at 10:30:40AM +0300, Andy Shevchenko wrote:
> On Tue, May 26, 2026 at 07:19:46PM +0100, Jonathan Cameron wrote:
> > On Mon, 25 May 2026 08:20:31 -0500
> > Maxwell Doose <m32285159@gmail.com> wrote:
> > > On Mon, May 25, 2026 at 2:17 AM Dan Carpenter <error27@gmail.com> wrote:
> > > >
> > > > 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?

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

-- 
With Best Regards,
Andy Shevchenko



^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH] iio: backend: fix uninitialized data in debugfs
  2026-06-04  7:30     ` Andy Shevchenko
  2026-06-04  7:38       ` Andy Shevchenko
@ 2026-06-04  7:45       ` Dan Carpenter
  2026-06-04  8:03         ` Andy Shevchenko
  1 sibling, 1 reply; 15+ messages in thread
From: Dan Carpenter @ 2026-06-04  7:45 UTC (permalink / raw)
  To: Andy Shevchenko
  Cc: Jonathan Cameron, Maxwell Doose, Nuno Sa, Olivier Moysan,
	David Lechner, Andy Shevchenko, linux-iio, linux-kernel,
	kernel-janitors

On Thu, Jun 04, 2026 at 10:30:34AM +0300, Andy Shevchenko wrote:
> On Tue, May 26, 2026 at 07:19:46PM +0100, Jonathan Cameron wrote:
> > On Mon, 25 May 2026 08:20:31 -0500
> > Maxwell Doose <m32285159@gmail.com> wrote:
> > > On Mon, May 25, 2026 at 2:17 AM Dan Carpenter <error27@gmail.com> wrote:
> > > >
> > > > 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?

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

regards,
dan carpenter


^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH] iio: backend: fix uninitialized data in debugfs
  2026-06-04  7:45       ` Dan Carpenter
@ 2026-06-04  8:03         ` Andy Shevchenko
  2026-06-04  8:28           ` Andy Shevchenko
  0 siblings, 1 reply; 15+ messages in thread
From: Andy Shevchenko @ 2026-06-04  8:03 UTC (permalink / raw)
  To: Dan Carpenter
  Cc: Jonathan Cameron, Maxwell Doose, Nuno Sa, Olivier Moysan,
	David Lechner, Andy Shevchenko, linux-iio, linux-kernel,
	kernel-janitors

On Thu, Jun 04, 2026 at 10:45:36AM +0300, Dan Carpenter wrote:
> On Thu, Jun 04, 2026 at 10:30:34AM +0300, Andy Shevchenko wrote:
> > On Tue, May 26, 2026 at 07:19:46PM +0100, Jonathan Cameron wrote:
> > > On Mon, 25 May 2026 08:20:31 -0500
> > > Maxwell Doose <m32285159@gmail.com> wrote:
> > > > On Mon, May 25, 2026 at 2:17 AM Dan Carpenter <error27@gmail.com> wrote:
> > > > >
> > > > > 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?

> 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?

-- 
With Best Regards,
Andy Shevchenko



^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH] iio: backend: fix uninitialized data in debugfs
  2026-06-04  8:03         ` Andy Shevchenko
@ 2026-06-04  8:28           ` Andy Shevchenko
  2026-06-04 10:38             ` Dan Carpenter
  0 siblings, 1 reply; 15+ messages in thread
From: Andy Shevchenko @ 2026-06-04  8:28 UTC (permalink / raw)
  To: Dan Carpenter
  Cc: Jonathan Cameron, Maxwell Doose, Nuno Sa, Olivier Moysan,
	David Lechner, Andy Shevchenko, linux-iio, linux-kernel,
	kernel-janitors

On Thu, Jun 04, 2026 at 11:03:15AM +0300, Andy Shevchenko wrote:
> On Thu, Jun 04, 2026 at 10:45:36AM +0300, Dan Carpenter wrote:
> > On Thu, Jun 04, 2026 at 10:30:34AM +0300, Andy Shevchenko wrote:
> > > On Tue, May 26, 2026 at 07:19:46PM +0100, Jonathan Cameron wrote:
> > > > On Mon, 25 May 2026 08:20:31 -0500
> > > > Maxwell Doose <m32285159@gmail.com> wrote:
> > > > > On Mon, May 25, 2026 at 2:17 AM Dan Carpenter <error27@gmail.com> wrote:
> > > > > >
> > > > > > 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?
> 
> > 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?

-- 
With Best Regards,
Andy Shevchenko



^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH] iio: backend: fix uninitialized data in debugfs
  2026-06-04  8:28           ` Andy Shevchenko
@ 2026-06-04 10:38             ` Dan Carpenter
  2026-06-04 10:42               ` Dan Carpenter
  2026-06-04 14:52               ` Andy Shevchenko
  0 siblings, 2 replies; 15+ messages in thread
From: Dan Carpenter @ 2026-06-04 10:38 UTC (permalink / raw)
  To: Andy Shevchenko
  Cc: Jonathan Cameron, Maxwell Doose, Nuno Sa, Olivier Moysan,
	David Lechner, Andy Shevchenko, linux-iio, linux-kernel,
	kernel-janitors

[-- Attachment #1: Type: text/plain, Size: 4309 bytes --]

On Thu, Jun 04, 2026 at 11:28:39AM +0300, Andy Shevchenko wrote:
> On Thu, Jun 04, 2026 at 11:03:15AM +0300, Andy Shevchenko wrote:
> > On Thu, Jun 04, 2026 at 10:45:36AM +0300, Dan Carpenter wrote:
> > > On Thu, Jun 04, 2026 at 10:30:34AM +0300, Andy Shevchenko wrote:
> > > > On Tue, May 26, 2026 at 07:19:46PM +0100, Jonathan Cameron wrote:
> > > > > On Mon, 25 May 2026 08:20:31 -0500
> > > > > Maxwell Doose <m32285159@gmail.com> wrote:
> > > > > > On Mon, May 25, 2026 at 2:17 AM Dan Carpenter <error27@gmail.com> wrote:
> > > > > > >
> > > > > > > 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?
> > 
> > > 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?

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

[-- Attachment #2: hello_debugfs.c --]
[-- Type: text/x-csrc, Size: 1989 bytes --]

#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);

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH] iio: backend: fix uninitialized data in debugfs
  2026-06-04 10:38             ` Dan Carpenter
@ 2026-06-04 10:42               ` Dan Carpenter
  2026-06-04 14:55                 ` Andy Shevchenko
  2026-06-04 14:52               ` Andy Shevchenko
  1 sibling, 1 reply; 15+ messages in thread
From: Dan Carpenter @ 2026-06-04 10:42 UTC (permalink / raw)
  To: Andy Shevchenko
  Cc: Jonathan Cameron, Maxwell Doose, Nuno Sa, Olivier Moysan,
	David Lechner, Andy Shevchenko, linux-iio, linux-kernel,
	kernel-janitors

On Thu, Jun 04, 2026 at 01:38:50PM +0300, Dan Carpenter wrote:
>    168          ret = sscanf(buf, "%i %i", &back->cached_reg_addr, &val);
>                              ^^^
> Uninitialized variable.

s/variable/data/.

regards,
dan carpenter


^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH] iio: backend: fix uninitialized data in debugfs
  2026-06-04 10:38             ` Dan Carpenter
  2026-06-04 10:42               ` Dan Carpenter
@ 2026-06-04 14:52               ` Andy Shevchenko
  1 sibling, 0 replies; 15+ messages in thread
From: Andy Shevchenko @ 2026-06-04 14:52 UTC (permalink / raw)
  To: Dan Carpenter
  Cc: Jonathan Cameron, Maxwell Doose, Nuno Sa, Olivier Moysan,
	David Lechner, Andy Shevchenko, linux-iio, linux-kernel,
	kernel-janitors

On Thu, Jun 04, 2026 at 01:38:50PM +0300, Dan Carpenter wrote:
> On Thu, Jun 04, 2026 at 11:28:39AM +0300, Andy Shevchenko wrote:
> > On Thu, Jun 04, 2026 at 11:03:15AM +0300, Andy Shevchenko wrote:
> > > On Thu, Jun 04, 2026 at 10:45:36AM +0300, Dan Carpenter wrote:
> > > > On Thu, Jun 04, 2026 at 10:30:34AM +0300, Andy Shevchenko wrote:
> > > > > On Tue, May 26, 2026 at 07:19:46PM +0100, Jonathan Cameron wrote:
> > > > > > On Mon, 25 May 2026 08:20:31 -0500
> > > > > > Maxwell Doose <m32285159@gmail.com> wrote:
> > > > > > > On Mon, May 25, 2026 at 2:17 AM Dan Carpenter <error27@gmail.com> wrote:
> > > > > > > >
> > > > > > > > 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?
> > > 
> > > > 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?
> 
> 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.

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?

>    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  

> # ./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

-- 
With Best Regards,
Andy Shevchenko



^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH] iio: backend: fix uninitialized data in debugfs
  2026-06-04 10:42               ` Dan Carpenter
@ 2026-06-04 14:55                 ` Andy Shevchenko
  2026-06-05  6:12                   ` Dan Carpenter
  0 siblings, 1 reply; 15+ messages in thread
From: Andy Shevchenko @ 2026-06-04 14:55 UTC (permalink / raw)
  To: Dan Carpenter
  Cc: Jonathan Cameron, Maxwell Doose, Nuno Sa, Olivier Moysan,
	David Lechner, Andy Shevchenko, linux-iio, linux-kernel,
	kernel-janitors

On Thu, Jun 04, 2026 at 01:42:11PM +0300, Dan Carpenter wrote:
> On Thu, Jun 04, 2026 at 01:38:50PM +0300, Dan Carpenter wrote:
> >    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).

-- 
With Best Regards,
Andy Shevchenko



^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH] iio: backend: fix uninitialized data in debugfs
  2026-06-04 14:55                 ` Andy Shevchenko
@ 2026-06-05  6:12                   ` Dan Carpenter
  2026-06-05  8:28                     ` Andy Shevchenko
  0 siblings, 1 reply; 15+ messages in thread
From: Dan Carpenter @ 2026-06-05  6:12 UTC (permalink / raw)
  To: Andy Shevchenko
  Cc: Jonathan Cameron, Maxwell Doose, Nuno Sa, Olivier Moysan,
	David Lechner, Andy Shevchenko, linux-iio, linux-kernel,
	kernel-janitors

On Thu, Jun 04, 2026 at 05:55:08PM +0300, Andy Shevchenko wrote:
> On Thu, Jun 04, 2026 at 01:42:11PM +0300, Dan Carpenter wrote:
> > On Thu, Jun 04, 2026 at 01:38:50PM +0300, Dan Carpenter wrote:
> > >    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).

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

regards,
dan carpenter


^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH] iio: backend: fix uninitialized data in debugfs
  2026-06-05  6:12                   ` Dan Carpenter
@ 2026-06-05  8:28                     ` Andy Shevchenko
  2026-06-05 14:38                       ` Maxwell Doose
  0 siblings, 1 reply; 15+ messages in thread
From: Andy Shevchenko @ 2026-06-05  8:28 UTC (permalink / raw)
  To: Dan Carpenter
  Cc: Jonathan Cameron, Maxwell Doose, Nuno Sa, Olivier Moysan,
	David Lechner, Andy Shevchenko, linux-iio, linux-kernel,
	kernel-janitors

On Fri, Jun 05, 2026 at 09:12:38AM +0300, Dan Carpenter wrote:
> On Thu, Jun 04, 2026 at 05:55:08PM +0300, Andy Shevchenko wrote:
> > On Thu, Jun 04, 2026 at 01:42:11PM +0300, Dan Carpenter wrote:
> > > On Thu, Jun 04, 2026 at 01:38:50PM +0300, Dan Carpenter wrote:
> > > >    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).
> 
> 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.

-- 
With Best Regards,
Andy Shevchenko



^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH] iio: backend: fix uninitialized data in debugfs
  2026-06-05  8:28                     ` Andy Shevchenko
@ 2026-06-05 14:38                       ` Maxwell Doose
  0 siblings, 0 replies; 15+ messages in thread
From: Maxwell Doose @ 2026-06-05 14:38 UTC (permalink / raw)
  To: Andy Shevchenko
  Cc: Dan Carpenter, Jonathan Cameron, Nuno Sa, Olivier Moysan,
	David Lechner, Andy Shevchenko, linux-iio, linux-kernel,
	kernel-janitors

On Fri, Jun 5, 2026 at 3:28 AM Andy Shevchenko
<andriy.shevchenko@intel.com> wrote:
>
> On Fri, Jun 05, 2026 at 09:12:38AM +0300, Dan Carpenter wrote:
> > On Thu, Jun 04, 2026 at 05:55:08PM +0300, Andy Shevchenko wrote:
> > > On Thu, Jun 04, 2026 at 01:42:11PM +0300, Dan Carpenter wrote:
> > > > On Thu, Jun 04, 2026 at 01:38:50PM +0300, Dan Carpenter wrote:
> > > > >    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).
> >
> > 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
:)

^ permalink raw reply	[flat|nested] 15+ messages in thread

end of thread, other threads:[~2026-06-05 14:40 UTC | newest]

Thread overview: 15+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2026-05-25  7:16 [PATCH] iio: backend: fix uninitialized data in debugfs Dan Carpenter
2026-05-25 13:20 ` Maxwell Doose
2026-05-26 18:19   ` Jonathan Cameron
2026-06-04  7:30     ` Andy Shevchenko
2026-06-04  7:38       ` Andy Shevchenko
2026-06-04  7:45       ` Dan Carpenter
2026-06-04  8:03         ` Andy Shevchenko
2026-06-04  8:28           ` Andy Shevchenko
2026-06-04 10:38             ` Dan Carpenter
2026-06-04 10:42               ` Dan Carpenter
2026-06-04 14:55                 ` Andy Shevchenko
2026-06-05  6:12                   ` Dan Carpenter
2026-06-05  8:28                     ` Andy Shevchenko
2026-06-05 14:38                       ` Maxwell Doose
2026-06-04 14:52               ` Andy Shevchenko

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.