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á