When simple_write_to_buffer() succeeds, it returns the number of bytes
actually copied to the buffer, which may be less than the requested
'count' if the buffer size is insufficient. However, the current code
incorrectly uses 'count' as the index for null termination instead of
the actual bytes copied, leading to out-of-bound write.

Add a check for the count and use the return value as the index.

Found via static analysis. This is similar to the
commit da9374819eb3 ("iio: backend: fix out-of-bound write")

Fixes: b1c5d68ea66e ("iio: dac: ad3552r-hs: add support for internal ramp")
Cc: stable@vger.kernel.org
Signed-off-by: Miaoqian Lin <linmq006@gmail.com>
---
 drivers/iio/dac/ad3552r-hs.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/iio/dac/ad3552r-hs.c b/drivers/iio/dac/ad3552r-hs.c
index 41b96b48ba98..a9578afa7015 100644
--- a/drivers/iio/dac/ad3552r-hs.c
+++ b/drivers/iio/dac/ad3552r-hs.c
@@ -549,12 +549,15 @@ static ssize_t ad3552r_hs_write_data_source(struct file *f,
 
 	guard(mutex)(&st->lock);
 
+	if (count >= sizeof(buf))
+		return -ENOSPC;
+
 	ret = simple_write_to_buffer(buf, sizeof(buf) - 1, ppos, userbuf,
 				     count);
 	if (ret < 0)
 		return ret;
 
-	buf[count] = '\0';
+	buf[ret] = '\0';
 
 	ret = match_string(dbgfs_attr_source, ARRAY_SIZE(dbgfs_attr_source),
 			   buf);
-- 
2.39.5 (Apple Git-154)

On Mon, Oct 27, 2025 at 11:07:13PM +0800, Miaoqian Lin wrote:
> When simple_write_to_buffer() succeeds, it returns the number of bytes
> actually copied to the buffer, which may be less than the requested
> 'count' if the buffer size is insufficient. However, the current code
> incorrectly uses 'count' as the index for null termination instead of
> the actual bytes copied, leading to out-of-bound write.
> 
> Add a check for the count and use the return value as the index.

...

> +	if (count >= sizeof(buf))
> +		return -ENOSPC;

This is already done below.

>  	ret = simple_write_to_buffer(buf, sizeof(buf) - 1, ppos, userbuf,
>  				     count);
>  	if (ret < 0)
>  		return ret;

...

> -	buf[count] = '\0';
> +	buf[ret] = '\0';

Do we have an actual issue right now? Can you model it and show a real
traceback?

-- 
With Best Regards,
Andy Shevchenko

On Mon, Oct 27, 2025 at 11:07:13PM +0800, Miaoqian Lin wrote:
> When simple_write_to_buffer() succeeds, it returns the number of bytes
> actually copied to the buffer, which may be less than the requested
> 'count' if the buffer size is insufficient. However, the current code
> incorrectly uses 'count' as the index for null termination instead of
> the actual bytes copied, leading to out-of-bound write.
> 
> Add a check for the count and use the return value as the index.

...

> +	if (count >= sizeof(buf))
> +		return -ENOSPC;

But this makes the validation too strict now.

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

You definitely failed to read the code that implements the above.

>  	if (ret < 0)
>  		return ret;
>  
> -	buf[count] = '\0';
> +	buf[ret] = '\0';

NAK.

This patch is an unneeded churn.

-- 
With Best Regards,
Andy Shevchenko

On Tue, Oct 28, 2025 at 10:18:05AM +0200, Andy Shevchenko wrote:
> On Mon, Oct 27, 2025 at 11:07:13PM +0800, Miaoqian Lin wrote:

...

> > +	if (count >= sizeof(buf))
> > +		return -ENOSPC;
> 
> But this makes the validation too strict now.
> 
> >  	ret = simple_write_to_buffer(buf, sizeof(buf) - 1, ppos, userbuf,
> >  				     count);
> 
> You definitely failed to read the code that implements the above.
> 
> >  	if (ret < 0)
> >  		return ret;

> > -	buf[count] = '\0';
> > +	buf[ret] = '\0';

Maybe this line is what we might need, but I haven't checked deeper if it's a
problem.

> NAK.
> 
> This patch is an unneeded churn.

-- 
With Best Regards,
Andy Shevchenko

On Tue, Oct 28, 2025 at 10:19:27AM +0200, Andy Shevchenko wrote:
> On Tue, Oct 28, 2025 at 10:18:05AM +0200, Andy Shevchenko wrote:
> > On Mon, Oct 27, 2025 at 11:07:13PM +0800, Miaoqian Lin wrote:

+Cc: Markus Burri for the da9374819eb3

...

> > > +	if (count >= sizeof(buf))
> > > +		return -ENOSPC;
> > 
> > But this makes the validation too strict now.
> > 
> > >  	ret = simple_write_to_buffer(buf, sizeof(buf) - 1, ppos, userbuf,
> > >  				     count);
> > 
> > You definitely failed to read the code that implements the above.
> > 
> > >  	if (ret < 0)
> > >  		return ret;
> 
> > > -	buf[count] = '\0';
> > > +	buf[ret] = '\0';
> 
> Maybe this line is what we might need, but I haven't checked deeper if it's a
> problem.

So, copy_to_user() and copy_from_user() are always inlined macros.
The simple_write_to_buffer() is not. The question here is how
the __builit_object_size() will behave on the address given as a parameter to
copy_from_user() in simple_write_to_buffer().

If it may detect reliably that the buffer is the size it has. I believe it's
easy for the byte arrays on stack.

That said, without proof that compiler is unable to determine the destination
buffer size, this patch and the one by Markus are simple noise which actually
changes an error code on the overflow condition.

The only line that assigns NUL character might be useful in some cases
(definitely when buffer comes through indirect calls from a heap, etc).

> > NAK.
> > 
> > This patch is an unneeded churn.

-- 
With Best Regards,
Andy Shevchenko

On Tue, 2025-10-28 at 11:07 +0200, Andy Shevchenko wrote:
> On Tue, Oct 28, 2025 at 10:19:27AM +0200, Andy Shevchenko wrote:
> > On Tue, Oct 28, 2025 at 10:18:05AM +0200, Andy Shevchenko wrote:
> > > On Mon, Oct 27, 2025 at 11:07:13PM +0800, Miaoqian Lin wrote:
> 
> +Cc: Markus Burri for the da9374819eb3
> 
> ...
> 
> > > > +	if (count >= sizeof(buf))
> > > > +		return -ENOSPC;
> > > 
> > > But this makes the validation too strict now.
> > > 
> > > >  	ret = simple_write_to_buffer(buf, sizeof(buf) - 1, ppos,
> > > > userbuf,
> > > >  				     count);
> > > 
> > > You definitely failed to read the code that implements the above.
> > > 
> > > >  	if (ret < 0)
> > > >  		return ret;
> > 
> > > > -	buf[count] = '\0';
> > > > +	buf[ret] = '\0';
> > 
> > Maybe this line is what we might need, but I haven't checked deeper if it's
> > a
> > problem.
> 
> So, copy_to_user() and copy_from_user() are always inlined macros.
> The simple_write_to_buffer() is not. The question here is how
> the __builit_object_size() will behave on the address given as a parameter to
> copy_from_user() in simple_write_to_buffer().
> 
> If it may detect reliably that the buffer is the size it has. I believe it's
> easy for the byte arrays on stack.
> 

I think the above does not make sense (unless I'm missing your point which might
very well be). So, __builit_object_size() is for things known at compile time.
Moreover, simple_write_to_buffer() already has all of the gymnastics to make
sure copy_from_user() has the proper parameters. The thing is that it does it in
a "silent" way which means that if your buffer is not big enough you'll get a
concatenated string. Sure, you'll likely get an error down the road (due to an
invalid value) but I do see some value in returning back the root cause of the
issue.

So, the preliminary check while not being a big deal, it's also not completely
useless IMO. I do not have any strong feeling though. However, I think the below
is very much needed...

> That said, without proof that compiler is unable to determine the destination
> buffer size, this patch and the one by Markus are simple noise which actually
> changes an error code on the overflow condition.
> 
> The only line that assigns NUL character might be useful in some cases
> (definitely when buffer comes through indirect calls from a heap, etc).

I think you can easily pass a string >= than 64 bytes (from userspace). AFAIR,
you don't really set a size into debugfs files. For sure you can mess things
with zero sized binary attributes so I have some confidence you have the same
with debugfs.

And even if all the above is not reproducible I'm still of the opinion that

buf[ret] = '\0';

is semantically the correct code.

- Nuno Sá

On Tue, Oct 28, 2025 at 12:31:04PM +0000, Nuno Sá wrote:
> On Tue, 2025-10-28 at 11:07 +0200, Andy Shevchenko wrote:
> > On Tue, Oct 28, 2025 at 10:19:27AM +0200, Andy Shevchenko wrote:
> > > On Tue, Oct 28, 2025 at 10:18:05AM +0200, Andy Shevchenko wrote:
> > > > On Mon, Oct 27, 2025 at 11:07:13PM +0800, Miaoqian Lin wrote:

...

> > > > > +	if (count >= sizeof(buf))
> > > > > +		return -ENOSPC;
> > > > 
> > > > But this makes the validation too strict now.
> > > > 
> > > > >  	ret = simple_write_to_buffer(buf, sizeof(buf) - 1, ppos,
> > > > > userbuf,
> > > > >  				     count);
> > > > 
> > > > You definitely failed to read the code that implements the above.
> > > > 
> > > > >  	if (ret < 0)
> > > > >  		return ret;
> > > 
> > > > > -	buf[count] = '\0';
> > > > > +	buf[ret] = '\0';
> > > 
> > > Maybe this line is what we might need, but I haven't checked deeper if it's
> > > a
> > > problem.
> > 
> > So, copy_to_user() and copy_from_user() are always inlined macros.
> > The simple_write_to_buffer() is not. The question here is how
> > the __builit_object_size() will behave on the address given as a parameter to
> > copy_from_user() in simple_write_to_buffer().
> > 
> > If it may detect reliably that the buffer is the size it has. I believe it's
> > easy for the byte arrays on stack.
> 
> I think the above does not make sense (unless I'm missing your point which might
> very well be).

It seems I stand corrected. I was staring too much at copy_from_user() without
retrieving the validation logic behind simple_write_to_buffer().

> So, __builit_object_size() is for things known at compile time.
> Moreover, simple_write_to_buffer() already has all of the gymnastics to make
> sure copy_from_user() has the proper parameters. The thing is that it does it in
> a "silent" way which means that if your buffer is not big enough you'll get a
> concatenated string. Sure, you'll likely get an error down the road (due to an
> invalid value) but I do see some value in returning back the root cause of the
> issue.
> 
> So, the preliminary check while not being a big deal, it's also not completely
> useless IMO. I do not have any strong feeling though. However, I think the below
> is very much needed...
> 
> > That said, without proof that compiler is unable to determine the destination
> > buffer size, this patch and the one by Markus are simple noise which actually
> > changes an error code on the overflow condition.
> > 
> > The only line that assigns NUL character might be useful in some cases
> > (definitely when buffer comes through indirect calls from a heap, etc).
> 
> I think you can easily pass a string >= than 64 bytes (from userspace). AFAIR,
> you don't really set a size into debugfs files. For sure you can mess things
> with zero sized binary attributes so I have some confidence you have the same
> with debugfs.
> 
> And even if all the above is not reproducible I'm still of the opinion that
> 
> buf[ret] = '\0';
> 
> is semantically the correct code.

Yes, but it should either be explained as just making code robust vs. real bugfix.
For the latter I want to see the real traceback and a reproducer. I also wonder why
we never had reports from syzkaller on this. It has non-zero chance to stumble over
the issue here (if there is an issue to begin with).

-- 
With Best Regards,
Andy Shevchenko

On Tue, 2025-10-28 at 16:45 +0200, Andy Shevchenko wrote:
> On Tue, Oct 28, 2025 at 12:31:04PM +0000, Nuno Sá wrote:
> > On Tue, 2025-10-28 at 11:07 +0200, Andy Shevchenko wrote:
> > > On Tue, Oct 28, 2025 at 10:19:27AM +0200, Andy Shevchenko wrote:
> > > > On Tue, Oct 28, 2025 at 10:18:05AM +0200, Andy Shevchenko wrote:
> > > > > On Mon, Oct 27, 2025 at 11:07:13PM +0800, Miaoqian Lin wrote:
> 
> ...
> 
> > > > > > +	if (count >= sizeof(buf))
> > > > > > +		return -ENOSPC;
> > > > > 
> > > > > But this makes the validation too strict now.
> > > > > 
> > > > > >  	ret = simple_write_to_buffer(buf, sizeof(buf) - 1, ppos,
> > > > > > userbuf,
> > > > > >  				     count);
> > > > > 
> > > > > You definitely failed to read the code that implements the above.
> > > > > 
> > > > > >  	if (ret < 0)
> > > > > >  		return ret;
> > > > 
> > > > > > -	buf[count] = '\0';
> > > > > > +	buf[ret] = '\0';
> > > > 
> > > > Maybe this line is what we might need, but I haven't checked deeper if
> > > > it's
> > > > a
> > > > problem.
> > > 
> > > So, copy_to_user() and copy_from_user() are always inlined macros.
> > > The simple_write_to_buffer() is not. The question here is how
> > > the __builit_object_size() will behave on the address given as a parameter
> > > to
> > > copy_from_user() in simple_write_to_buffer().
> > > 
> > > If it may detect reliably that the buffer is the size it has. I believe
> > > it's
> > > easy for the byte arrays on stack.
> > 
> > I think the above does not make sense (unless I'm missing your point which
> > might
> > very well be).
> 
> It seems I stand corrected. I was staring too much at copy_from_user() without
> retrieving the validation logic behind simple_write_to_buffer().

:)

...

> > 
> > I think you can easily pass a string >= than 64 bytes (from userspace).
> > AFAIR,
> > you don't really set a size into debugfs files. For sure you can mess things
> > with zero sized binary attributes so I have some confidence you have the
> > same
> > with debugfs.
> > 
> > And even if all the above is not reproducible I'm still of the opinion that
> > 
> > buf[ret] = '\0';
> > 
> > is semantically the correct code.
> 
> Yes, but it should either be explained as just making code robust vs. real
> bugfix.

Agreed. If we find it's the former, the commit message should be updated.

> For the latter I want to see the real traceback and a reproducer. I also
> wonder why
> we never had reports from syzkaller on this. It has non-zero chance to stumble
> over
> the issue here (if there is an issue to begin with).

If I have the time, I might do it. If my suspicious are correct, it should be
fairly easy to reproduce.

- Nuno Sá

On Tue, Oct 28, 2025 at 03:12:29PM +0000, Nuno Sá wrote:
> On Tue, 2025-10-28 at 16:45 +0200, Andy Shevchenko wrote:
> > On Tue, Oct 28, 2025 at 12:31:04PM +0000, Nuno Sá wrote:

...

> > For the latter I want to see the real traceback and a reproducer. I also
> > wonder why
> > we never had reports from syzkaller on this. It has non-zero chance to stumble
> > over
> > the issue here (if there is an issue to begin with).
> 
> If I have the time, I might do it. If my suspicious are correct, it should be
> fairly easy to reproduce.

My suspicious is also like this, if you have a working setup for one of such
a user (like this chip) already, it's ~15 minutes to get it done without
writing an additional code.

-- 
With Best Regards,
Andy Shevchenko

Hi,

I don’t have the actual hardware, so I built a similar demo module to
mirror the bug and ran it in QEMU.
With KASAN enabled, the PoC triggers BUG: KASAN: stack-out-of-bounds.

Pattern of the bug:
- A fixed 64-byte stack buffer is filled using count.
- If count > 64, the code still does buf[count] = '\0', causing an
out-of-bounds write on the stack.

PoC (what it does):
- Opens the device node.
- Writes 128 bytes of A to it.
- This overflows the 64-byte stack buffer and KASAN reports the stack OOB.

If you have the real device, you may run the similar PoC on your driver
to validate—just ensure KASAN is enabled to see the report.
I also tested the straightforward fix buf[ret] = '\0'; with that
change, the issue no longer reproduces.
Below are the trace, the demo module, and the PoC for reference.


[   11.824318] overflow_demo: copied 63 bytes (stack buf 64, user count 128)
[   11.825125] ==================================================================
[   11.825806] BUG: KASAN: stack-out-of-bounds in mas_walk+0x466/0x510
[   11.825806] Write of size 8 at addr ffff888011017e48 by task
overflow_demo_p/75
[   11.825806]
[   11.825806] CPU: 0 PID: 75 Comm: overflow_demo_p Tainted: G
  O       6.6.9 #4
[   11.825806] Hardware name: QEMU Standard PC (Q35 + ICH9, 2009),
BIOS 1.16.3-debian-1.16.3-2 04/01/2014
[   11.825806] Call Trace:
[   11.825806]  <TASK>
[   11.825806]  dump_stack_lvl+0x36/0x50
[   11.825806]  print_report+0xcf/0x670
[   11.825806]  ? __pfx__raw_spin_lock_irqsave+0x10/0x10
[   11.825806]  kasan_report+0xc7/0x100
[   11.825806]  ? mas_walk+0x466/0x510
[   11.825806]  ? mas_walk+0x466/0x510
[   11.825806]  mas_walk+0x466/0x510
[   11.825806]  ? __pfx_rebalance_domains+0x10/0x10
[   11.825806]  lock_vma_under_rcu+0xbe/0x420
[   11.825806]  ? __pfx_lock_vma_under_rcu+0x10/0x10
[   11.825806]  ? __do_softirq+0x1bf/0x5b2
[   11.825806]  ? hrtimer_interrupt+0x313/0x7c0
[   11.825806]  do_user_addr_fault+0x1c6/0x940
[   11.825806]  exc_page_fault+0x5d/0xd0
[   11.825806]  asm_exc_page_fault+0x26/0x30
[   11.825806] RIP: 0033:0x43f430
[   11.825806] Code: Unable to access opcode bytes at 0x43f406.
[   11.825806] RSP: 002b:00007ffe2f3b53b8 EFLAGS: 00000202
[   11.825806] RAX: 00007ffe2f3b53e0 RBX: 0000000000000003 RCX: 0000000000000002
[   11.825806] RDX: 00007ffe2f3b53c0 RSI: 0000000000486029 RDI: 00000000004b2320
[   11.825806] RBP: 00007ffe2f3b5490 R08: 00000000004b2820 R09: 0000000000000110
[   11.825806] R10: 0000000000000000 R11: 0000000000000202 R12: 00007ffe2f3b54a0
[   11.825806] R13: 00007ffe2f3b5668 R14: 00000000004ad868 R15: 0000000000000001
[   11.825806]  </TASK>
[   11.825806]
[   11.825806] The buggy address belongs to stack of task overflow_demo_p/75
[   11.825806]  and is located at offset 56 in frame:
[   11.825806]  lock_vma_under_rcu+0x0/0x420
[   11.825806]
[   11.825806] This frame has 1 object:
[   11.825806]  [32, 96) 'mas'
[   11.825806]
[   11.825806] The buggy address belongs to the physical page:
[   11.825806] page:(____ptrval____) refcount:0 mapcount:0
mapping:0000000000000000 index:0x0 pfn:0x11017
[   11.825806] flags: 0x100000000000000(node=0|zone=1)
[   11.825806] page_type: 0xffffffff()
[   11.825806] raw: 0100000000000000 dead000000000100 dead000000000122
0000000000000000
[   11.825806] raw: 0000000000000000 0000000000000000 00000000ffffffff
0000000000000000
[   11.825806] page dumped because: kasan: bad access detected
[   11.825806]
[   11.825806] Memory state around the buggy address:
[   11.825806]  ffff888011017d00: 00 00 00 00 00 00 00 00 00 00 00 00
00 00 00 00
[   11.825806]  ffff888011017d80: 00 00 00 00 00 00 00 00 00 00 00 00
00 00 00 00
[   11.825806] >ffff888011017e00: 00 00 f1 f1 f1 f1 00 00 00 f3 00 00
00 00 f3 f3
[   11.825806]                                               ^
[   11.825806]  ffff888011017e80: f3 f3 00 00 00 00 00 00 00 00 00 00
00 00 00 00
[   11.825806]  ffff888011017f00: 00 00 00 00 00 00 00 00 00 00 00 00
00 00 00 00
[   11.825806] ==================================================================
[   11.858618] Disabling lock debugging due to kernel taint
[   11.892349] overflow_demo_p (75) used greatest stack depth: 27192 bytes left

Andy Shevchenko <andriy.shevchenko@intel.com> 于2025年10月28日周二 23:19写道：
>
> On Tue, Oct 28, 2025 at 03:12:29PM +0000, Nuno Sá wrote:
> > On Tue, 2025-10-28 at 16:45 +0200, Andy Shevchenko wrote:
> > > On Tue, Oct 28, 2025 at 12:31:04PM +0000, Nuno Sá wrote:
>
> ...
>
> > > For the latter I want to see the real traceback and a reproducer. I also
> > > wonder why
> > > we never had reports from syzkaller on this. It has non-zero chance to stumble
> > > over
> > > the issue here (if there is an issue to begin with).
> >
> > If I have the time, I might do it. If my suspicious are correct, it should be
> > fairly easy to reproduce.
>
> My suspicious is also like this, if you have a working setup for one of such
> a user (like this chip) already, it's ~15 minutes to get it done without
> writing an additional code.
>
> --
> With Best Regards,
> Andy Shevchenko
>
>
// SPDX-License-Identifier: GPL-2.0-only
/*
 * Simple PoC for the overflow_demo kernel module.
 * Build with: gcc -Wall -O2 -o overflow_poc samples/overflow_demo_poc.c
 */

#include <errno.h>
#include <fcntl.h>
#include <stdio.h>
#include <stdlib.h>
#include <string.h>
#include <unistd.h>

#ifndef TARGET_DEVICE
#define TARGET_DEVICE "/dev/overflow_demo"
#endif
#define PAYLOAD_SIZE 128

int main(void)
{
	char payload[PAYLOAD_SIZE];
	int fd, ret;

	memset(payload, 'A', sizeof(payload));

	fd = open(TARGET_DEVICE, O_WRONLY);
	if (fd < 0) {
		perror("open");
		return EXIT_FAILURE;
	}

	ret = write(fd, payload, sizeof(payload));
	if (ret < 0) {
		perror("write");
		close(fd);
		return EXIT_FAILURE;
	}

	printf("wrote %d bytes to %s\n", ret, TARGET_DEVICE);
	close(fd);
	return EXIT_SUCCESS;
}
// SPDX-License-Identifier: GPL-2.0-only
/*
 * Stack overflow demo module
 *
 * This sample mimics a real-world bug pattern: a fixed-size stack buffer is
 * filled via simple_write_to_buffer(), but the code uses the user-supplied
 * count to place a terminator, so a large count causes a write past the end
 * of the stack buffer even if the copy was truncated. Device: /dev/overflow_demo.
 */

#define pr_fmt(fmt) "overflow_demo: " fmt

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

#define DEMO_STACK_BUF_SIZE 64

static noinline ssize_t overflow_demo_write(struct file *file,
					    const char __user *buf, size_t len,
					    loff_t *ppos)
{
	char local[DEMO_STACK_BUF_SIZE];
	ssize_t written;

	if (!len)
		return 0;

	/* Vulnerable pattern: copy is bounded, but terminator uses user count. */
	written = simple_write_to_buffer(local, sizeof(local) - 1, ppos, buf,
					 len);
	if (written < 0)
		return written;

	/* Diagnostic: show the stack buffer base and the terminator write target. */
	if (len >= sizeof(local))
		pr_err("len=%zu >= buf=%zu: local=%px, terminator_target=%px\n",
		       len, sizeof(local), local, local + len);
	else
		pr_info("len=%zu < buf=%zu: local=%px, terminator_target=%px\n",
		       len, sizeof(local), local, local + len);

	/* BUG: len may exceed local size; this OOB write mimics the real issue. */
	local[len] = '\0';

	pr_info("copied %zd bytes (stack buf %zu, user count %zu)\n", written,
		sizeof(local), len);

	return written;
}

static const struct file_operations overflow_demo_fops = {
	.owner = THIS_MODULE,
	.write = overflow_demo_write,
};

static struct miscdevice overflow_demo_miscdev = {
	.minor = MISC_DYNAMIC_MINOR,
	.name = "overflow_demo",
	.fops = &overflow_demo_fops,
};

static int __init overflow_demo_init(void)
{
	int ret;

	ret = misc_register(&overflow_demo_miscdev);
	if (ret)
		return ret;

	pr_warn("loaded: writing > %d bytes will corrupt the stack (count-based OOB)\n",
		DEMO_STACK_BUF_SIZE);
	return 0;
}

static void __exit overflow_demo_exit(void)
{
	misc_deregister(&overflow_demo_miscdev);
	pr_info("unloaded\n");
}

module_init(overflow_demo_init);
module_exit(overflow_demo_exit);

MODULE_DESCRIPTION("Intentional stack overflow demo module");
MODULE_AUTHOR("Your Name <devnull@example.com>");
MODULE_LICENSE("GPL");

On Wed, Dec 17, 2025 at 02:47:17PM +0800, 林妙倩 wrote:
> Hi,
> 
> I don’t have the actual hardware, so I built a similar demo module to
> mirror the bug and ran it in QEMU.
> With KASAN enabled, the PoC triggers BUG: KASAN: stack-out-of-bounds.
> 
> Pattern of the bug:
> - A fixed 64-byte stack buffer is filled using count.
> - If count > 64, the code still does buf[count] = '\0', causing an
> out-of-bounds write on the stack.
> 
> PoC (what it does):
> - Opens the device node.
> - Writes 128 bytes of A to it.
> - This overflows the 64-byte stack buffer and KASAN reports the stack OOB.
> 
> If you have the real device, you may run the similar PoC on your driver
> to validate—just ensure KASAN is enabled to see the report.
> I also tested the straightforward fix buf[ret] = '\0'; with that
> change, the issue no longer reproduces.
> Below are the trace, the demo module, and the PoC for reference.

Thanks for the additional information, I think it would be good to have a
summary of it in the commit message of the fix.

-- 
With Best Regards,
Andy Shevchenko

Hi, Andy

Thanks for you review.

Andy Shevchenko <andriy.shevchenko@intel.com> 于2025年10月28日周二 17:07写道：
>
> On Tue, Oct 28, 2025 at 10:19:27AM +0200, Andy Shevchenko wrote:
> > On Tue, Oct 28, 2025 at 10:18:05AM +0200, Andy Shevchenko wrote:
> > > On Mon, Oct 27, 2025 at 11:07:13PM +0800, Miaoqian Lin wrote:
>
> +Cc: Markus Burri for the da9374819eb3
>
> ...
>
> > > > + if (count >= sizeof(buf))
> > > > +         return -ENOSPC;
> > >
> > > But this makes the validation too strict now.
> > >
> > > >   ret = simple_write_to_buffer(buf, sizeof(buf) - 1, ppos, userbuf,
> > > >                                count);
> > >
> > > You definitely failed to read the code that implements the above.
> > >

I previously read the simple_write_to_buffer(), but add the check and
think it can help to catch error eariler. My mistake.

> > > >   if (ret < 0)
> > > >           return ret;
> >
> > > > - buf[count] = '\0';
> > > > + buf[ret] = '\0';
> >
> > Maybe this line is what we might need, but I haven't checked deeper if it's a
> > problem.
>
> So, copy_to_user() and copy_from_user() are always inlined macros.
> The simple_write_to_buffer() is not. The question here is how
> the __builit_object_size() will behave on the address given as a parameter to
> copy_from_user() in simple_write_to_buffer().
>
> If it may detect reliably that the buffer is the size it has. I believe it's
> easy for the byte arrays on stack.
>
> That said, without proof that compiler is unable to determine the destination
> buffer size, this patch and the one by Markus are simple noise which actually
> changes an error code on the overflow condition.
>
> The only line that assigns NUL character might be useful in some cases
> (definitely when buffer comes through indirect calls from a heap, etc).
>

I believe it is still necessray to use buf[ret] = '\0'; intead of
buf[count] = '\0';
If you argee with this, I send a v2 with just this fix. Thanks.

> > > NAK.
> > >
> > > This patch is an unneeded churn.
>
> --
> With Best Regards,
> Andy Shevchenko
>
>

On Tue, Oct 28, 2025 at 05:46:53PM +0800, 林妙倩 wrote:
> Andy Shevchenko <andriy.shevchenko@intel.com> 于2025年10月28日周二 17:07写道：
> > On Tue, Oct 28, 2025 at 10:19:27AM +0200, Andy Shevchenko wrote:
> > > On Tue, Oct 28, 2025 at 10:18:05AM +0200, Andy Shevchenko wrote:
> > > > On Mon, Oct 27, 2025 at 11:07:13PM +0800, Miaoqian Lin wrote:

...

> > > > > + if (count >= sizeof(buf))
> > > > > +         return -ENOSPC;
> > > >
> > > > But this makes the validation too strict now.
> > > >
> > > > >   ret = simple_write_to_buffer(buf, sizeof(buf) - 1, ppos, userbuf,
> > > > >                                count);
> > > >
> > > > You definitely failed to read the code that implements the above.
> > > >
> 
> I previously read the simple_write_to_buffer(), but add the check and
> think it can help to catch error eariler. My mistake.
> 
> > > > >   if (ret < 0)
> > > > >           return ret;
> > >
> > > > > - buf[count] = '\0';
> > > > > + buf[ret] = '\0';
> > >
> > > Maybe this line is what we might need, but I haven't checked deeper if it's a
> > > problem.
> >
> > So, copy_to_user() and copy_from_user() are always inlined macros.
> > The simple_write_to_buffer() is not. The question here is how
> > the __builit_object_size() will behave on the address given as a parameter to
> > copy_from_user() in simple_write_to_buffer().
> >
> > If it may detect reliably that the buffer is the size it has. I believe it's
> > easy for the byte arrays on stack.
> >
> > That said, without proof that compiler is unable to determine the destination
> > buffer size, this patch and the one by Markus are simple noise which actually
> > changes an error code on the overflow condition.
> >
> > The only line that assigns NUL character might be useful in some cases
> > (definitely when buffer comes through indirect calls from a heap, etc).
> >
> 
> I believe it is still necessray to use buf[ret] = '\0'; intead of
> buf[count] = '\0';
> If you argee with this, I send a v2 with just this fix. Thanks.

As explained above, please try to model the situation and see if current code
is buggy, i.e. provide a step-by-step test case and show a traceback that
points to a out-of-boundary access in this function. (Note, you don't need to
have a HW for that, you might need to create a dummy IIO or other module with
the similar interface and run it, in such a case, share also link to the source
code of that module.) When you prove the problem exists, I will happily ACK
all similar patches, including yours.

> > > > NAK.
> > > >
> > > > This patch is an unneeded churn.

-- 
With Best Regards,
Andy Shevchenko

On Mon, 2025-10-27 at 23:07 +0800, Miaoqian Lin wrote:
> When simple_write_to_buffer() succeeds, it returns the number of bytes
> actually copied to the buffer, which may be less than the requested
> 'count' if the buffer size is insufficient. However, the current code
> incorrectly uses 'count' as the index for null termination instead of
> the actual bytes copied, leading to out-of-bound write.
> 
> Add a check for the count and use the return value as the index.
> 
> Found via static analysis. This is similar to the
> commit da9374819eb3 ("iio: backend: fix out-of-bound write")
> 
> Fixes: b1c5d68ea66e ("iio: dac: ad3552r-hs: add support for internal ramp")
> Cc: stable@vger.kernel.org
> Signed-off-by: Miaoqian Lin <linmq006@gmail.com>
> ---

Reviewed-by: Nuno Sá <nuno.sa@analog.com>

>  drivers/iio/dac/ad3552r-hs.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/iio/dac/ad3552r-hs.c b/drivers/iio/dac/ad3552r-hs.c
> index 41b96b48ba98..a9578afa7015 100644
> --- a/drivers/iio/dac/ad3552r-hs.c
> +++ b/drivers/iio/dac/ad3552r-hs.c
> @@ -549,12 +549,15 @@ static ssize_t ad3552r_hs_write_data_source(struct file
> *f,
>  
>  	guard(mutex)(&st->lock);
>  
> +	if (count >= sizeof(buf))
> +		return -ENOSPC;
> +
>  	ret = simple_write_to_buffer(buf, sizeof(buf) - 1, ppos, userbuf,
>  				     count);
>  	if (ret < 0)
>  		return ret;
>  
> -	buf[count] = '\0';
> +	buf[ret] = '\0';
>  
>  	ret = match_string(dbgfs_attr_source, ARRAY_SIZE(dbgfs_attr_source),
>  			   buf);

On 10/27/25 10:07 AM, Miaoqian Lin wrote:
> When simple_write_to_buffer() succeeds, it returns the number of bytes
> actually copied to the buffer, which may be less than the requested
> 'count' if the buffer size is insufficient. However, the current code
> incorrectly uses 'count' as the index for null termination instead of
> the actual bytes copied, leading to out-of-bound write.
> 
> Add a check for the count and use the return value as the index.
> 
> Found via static analysis. This is similar to the
> commit da9374819eb3 ("iio: backend: fix out-of-bound write")
> 
> Fixes: b1c5d68ea66e ("iio: dac: ad3552r-hs: add support for internal ramp")
> Cc: stable@vger.kernel.org
> Signed-off-by: Miaoqian Lin <linmq006@gmail.com>
> ---
Reviewed-by: David Lechner <dlechner@baylibre.com>