* [PATCH] iio: dac: ad3552r-hs: fix out-of-bound write in ad3552r_hs_write_data_source
@ 2025-10-27 15:07 Miaoqian Lin
2025-10-27 15:19 ` David Lechner
` (3 more replies)
0 siblings, 4 replies; 13+ messages in thread
From: Miaoqian Lin @ 2025-10-27 15:07 UTC (permalink / raw)
To: Lars-Peter Clausen, Michael Hennerich, Jonathan Cameron,
David Lechner, Nuno Sá, Andy Shevchenko, Angelo Dureghello,
linux-iio, linux-kernel
Cc: linmq006, stable
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)
^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH] iio: dac: ad3552r-hs: fix out-of-bound write in ad3552r_hs_write_data_source
2025-10-27 15:07 [PATCH] iio: dac: ad3552r-hs: fix out-of-bound write in ad3552r_hs_write_data_source Miaoqian Lin
@ 2025-10-27 15:19 ` David Lechner
2025-10-27 16:05 ` Nuno Sá
` (2 subsequent siblings)
3 siblings, 0 replies; 13+ messages in thread
From: David Lechner @ 2025-10-27 15:19 UTC (permalink / raw)
To: Miaoqian Lin, Lars-Peter Clausen, Michael Hennerich,
Jonathan Cameron, Nuno Sá, Andy Shevchenko,
Angelo Dureghello, linux-iio, linux-kernel
Cc: stable
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>
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] iio: dac: ad3552r-hs: fix out-of-bound write in ad3552r_hs_write_data_source
2025-10-27 15:07 [PATCH] iio: dac: ad3552r-hs: fix out-of-bound write in ad3552r_hs_write_data_source Miaoqian Lin
2025-10-27 15:19 ` David Lechner
@ 2025-10-27 16:05 ` Nuno Sá
2025-10-28 8:18 ` Andy Shevchenko
2025-10-28 11:40 ` Andy Shevchenko
3 siblings, 0 replies; 13+ messages in thread
From: Nuno Sá @ 2025-10-27 16:05 UTC (permalink / raw)
To: Miaoqian Lin, Lars-Peter Clausen, Michael Hennerich,
Jonathan Cameron, David Lechner, Nuno Sá, Andy Shevchenko,
Angelo Dureghello, linux-iio, linux-kernel
Cc: stable
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);
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] iio: dac: ad3552r-hs: fix out-of-bound write in ad3552r_hs_write_data_source
2025-10-27 15:07 [PATCH] iio: dac: ad3552r-hs: fix out-of-bound write in ad3552r_hs_write_data_source Miaoqian Lin
2025-10-27 15:19 ` David Lechner
2025-10-27 16:05 ` Nuno Sá
@ 2025-10-28 8:18 ` Andy Shevchenko
2025-10-28 8:19 ` Andy Shevchenko
2025-10-28 11:40 ` Andy Shevchenko
3 siblings, 1 reply; 13+ messages in thread
From: Andy Shevchenko @ 2025-10-28 8:18 UTC (permalink / raw)
To: Miaoqian Lin
Cc: Lars-Peter Clausen, Michael Hennerich, Jonathan Cameron,
David Lechner, Nuno Sá, Andy Shevchenko, Angelo Dureghello,
linux-iio, linux-kernel, stable
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
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] iio: dac: ad3552r-hs: fix out-of-bound write in ad3552r_hs_write_data_source
2025-10-28 8:18 ` Andy Shevchenko
@ 2025-10-28 8:19 ` Andy Shevchenko
2025-10-28 9:07 ` Andy Shevchenko
0 siblings, 1 reply; 13+ messages in thread
From: Andy Shevchenko @ 2025-10-28 8:19 UTC (permalink / raw)
To: Miaoqian Lin
Cc: Lars-Peter Clausen, Michael Hennerich, Jonathan Cameron,
David Lechner, Nuno Sá, Andy Shevchenko, Angelo Dureghello,
linux-iio, linux-kernel, stable
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
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] iio: dac: ad3552r-hs: fix out-of-bound write in ad3552r_hs_write_data_source
2025-10-28 8:19 ` Andy Shevchenko
@ 2025-10-28 9:07 ` Andy Shevchenko
2025-10-28 9:46 ` 林妙倩
2025-10-28 12:31 ` Nuno Sá
0 siblings, 2 replies; 13+ messages in thread
From: Andy Shevchenko @ 2025-10-28 9:07 UTC (permalink / raw)
To: Miaoqian Lin, Markus Burri
Cc: Lars-Peter Clausen, Michael Hennerich, Jonathan Cameron,
David Lechner, Nuno Sá, Andy Shevchenko, Angelo Dureghello,
linux-iio, linux-kernel, stable
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
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] iio: dac: ad3552r-hs: fix out-of-bound write in ad3552r_hs_write_data_source
2025-10-28 9:07 ` Andy Shevchenko
@ 2025-10-28 9:46 ` 林妙倩
2025-10-28 9:58 ` Andy Shevchenko
2025-10-28 12:31 ` Nuno Sá
1 sibling, 1 reply; 13+ messages in thread
From: 林妙倩 @ 2025-10-28 9:46 UTC (permalink / raw)
To: Andy Shevchenko
Cc: Markus Burri, Lars-Peter Clausen, Michael Hennerich,
Jonathan Cameron, David Lechner, Nuno Sá, Andy Shevchenko,
Angelo Dureghello, linux-iio, linux-kernel, stable
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
>
>
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] iio: dac: ad3552r-hs: fix out-of-bound write in ad3552r_hs_write_data_source
2025-10-28 9:46 ` 林妙倩
@ 2025-10-28 9:58 ` Andy Shevchenko
0 siblings, 0 replies; 13+ messages in thread
From: Andy Shevchenko @ 2025-10-28 9:58 UTC (permalink / raw)
To: 林妙倩
Cc: Markus Burri, Lars-Peter Clausen, Michael Hennerich,
Jonathan Cameron, David Lechner, Nuno Sá, Andy Shevchenko,
Angelo Dureghello, linux-iio, linux-kernel, stable
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
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] iio: dac: ad3552r-hs: fix out-of-bound write in ad3552r_hs_write_data_source
2025-10-27 15:07 [PATCH] iio: dac: ad3552r-hs: fix out-of-bound write in ad3552r_hs_write_data_source Miaoqian Lin
` (2 preceding siblings ...)
2025-10-28 8:18 ` Andy Shevchenko
@ 2025-10-28 11:40 ` Andy Shevchenko
3 siblings, 0 replies; 13+ messages in thread
From: Andy Shevchenko @ 2025-10-28 11:40 UTC (permalink / raw)
To: Miaoqian Lin
Cc: Lars-Peter Clausen, Michael Hennerich, Jonathan Cameron,
David Lechner, Nuno Sá, Andy Shevchenko, Angelo Dureghello,
linux-iio, linux-kernel, stable
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
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] iio: dac: ad3552r-hs: fix out-of-bound write in ad3552r_hs_write_data_source
2025-10-28 9:07 ` Andy Shevchenko
2025-10-28 9:46 ` 林妙倩
@ 2025-10-28 12:31 ` Nuno Sá
2025-10-28 14:45 ` Andy Shevchenko
1 sibling, 1 reply; 13+ messages in thread
From: Nuno Sá @ 2025-10-28 12:31 UTC (permalink / raw)
To: Andy Shevchenko, Miaoqian Lin, Markus Burri
Cc: Lars-Peter Clausen, Michael Hennerich, Jonathan Cameron,
David Lechner, Nuno Sá, Andy Shevchenko, Angelo Dureghello,
linux-iio, linux-kernel, stable
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á
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] iio: dac: ad3552r-hs: fix out-of-bound write in ad3552r_hs_write_data_source
2025-10-28 12:31 ` Nuno Sá
@ 2025-10-28 14:45 ` Andy Shevchenko
2025-10-28 15:12 ` Nuno Sá
0 siblings, 1 reply; 13+ messages in thread
From: Andy Shevchenko @ 2025-10-28 14:45 UTC (permalink / raw)
To: Nuno Sá
Cc: Miaoqian Lin, Markus Burri, Lars-Peter Clausen, Michael Hennerich,
Jonathan Cameron, David Lechner, Nuno Sá, Andy Shevchenko,
Angelo Dureghello, linux-iio, linux-kernel, stable
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
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] iio: dac: ad3552r-hs: fix out-of-bound write in ad3552r_hs_write_data_source
2025-10-28 14:45 ` Andy Shevchenko
@ 2025-10-28 15:12 ` Nuno Sá
2025-10-28 15:19 ` Andy Shevchenko
0 siblings, 1 reply; 13+ messages in thread
From: Nuno Sá @ 2025-10-28 15:12 UTC (permalink / raw)
To: Andy Shevchenko
Cc: Miaoqian Lin, Markus Burri, Lars-Peter Clausen, Michael Hennerich,
Jonathan Cameron, David Lechner, Nuno Sá, Andy Shevchenko,
Angelo Dureghello, linux-iio, linux-kernel, stable
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á
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] iio: dac: ad3552r-hs: fix out-of-bound write in ad3552r_hs_write_data_source
2025-10-28 15:12 ` Nuno Sá
@ 2025-10-28 15:19 ` Andy Shevchenko
0 siblings, 0 replies; 13+ messages in thread
From: Andy Shevchenko @ 2025-10-28 15:19 UTC (permalink / raw)
To: Nuno Sá
Cc: Miaoqian Lin, Markus Burri, Lars-Peter Clausen, Michael Hennerich,
Jonathan Cameron, David Lechner, Nuno Sá, Andy Shevchenko,
Angelo Dureghello, linux-iio, linux-kernel, stable
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
^ permalink raw reply [flat|nested] 13+ messages in thread
end of thread, other threads:[~2025-10-28 15:19 UTC | newest]
Thread overview: 13+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-10-27 15:07 [PATCH] iio: dac: ad3552r-hs: fix out-of-bound write in ad3552r_hs_write_data_source Miaoqian Lin
2025-10-27 15:19 ` David Lechner
2025-10-27 16:05 ` Nuno Sá
2025-10-28 8:18 ` Andy Shevchenko
2025-10-28 8:19 ` Andy Shevchenko
2025-10-28 9:07 ` Andy Shevchenko
2025-10-28 9:46 ` 林妙倩
2025-10-28 9:58 ` Andy Shevchenko
2025-10-28 12:31 ` Nuno Sá
2025-10-28 14:45 ` Andy Shevchenko
2025-10-28 15:12 ` Nuno Sá
2025-10-28 15:19 ` Andy Shevchenko
2025-10-28 11:40 ` Andy Shevchenko
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).