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);
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
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 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.
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.
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.
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.
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).
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.
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?
linux-stable-mirror@lists.linaro.org
-
Andy Shevchenko -
David Lechner -
Miaoqian Lin -
Nuno Sá -
林妙倩