Including a nul byte in the otherwise human-readable ascii output from this debugfs file is probably not intended.
Signed-off-by: Rasmus Villemoes linux@rasmusvillemoes.dk --- drivers/greybus/es2.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/greybus/es2.c b/drivers/greybus/es2.c index 48ad154df3a7..86a7fbc7fe13 100644 --- a/drivers/greybus/es2.c +++ b/drivers/greybus/es2.c @@ -1171,7 +1171,7 @@ static ssize_t apb_log_enable_read(struct file *f, char __user *buf, char tmp_buf[3];
sprintf(tmp_buf, "%d\n", enable); - return simple_read_from_buffer(buf, count, ppos, tmp_buf, 3); + return simple_read_from_buffer(buf, count, ppos, tmp_buf, 2); }
static ssize_t apb_log_enable_write(struct file *f, const char __user *buf,
On 3/26/21 10:22 AM, Rasmus Villemoes wrote:
Including a nul byte in the otherwise human-readable ascii output from this debugfs file is probably not intended.
Looking only at the comments above simple_read_from_buffer(), the last argument is the size of the buffer (tmp_buf in this case). So "3" is proper.
But looking at callers, it seems that the trailing NUL is often excluded this way.
I don't really have a problem with your patch, but could you explain why having the NUL byte included is an actual problem? A short statement about that would provide better context than just "probably not intended."
Thanks.
-Alex
Signed-off-by: Rasmus Villemoes linux@rasmusvillemoes.dk
drivers/greybus/es2.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/greybus/es2.c b/drivers/greybus/es2.c index 48ad154df3a7..86a7fbc7fe13 100644 --- a/drivers/greybus/es2.c +++ b/drivers/greybus/es2.c @@ -1171,7 +1171,7 @@ static ssize_t apb_log_enable_read(struct file *f, char __user *buf, char tmp_buf[3]; sprintf(tmp_buf, "%d\n", enable);
- return simple_read_from_buffer(buf, count, ppos, tmp_buf, 3);
- return simple_read_from_buffer(buf, count, ppos, tmp_buf, 2); }
static ssize_t apb_log_enable_write(struct file *f, const char __user *buf,
On 26/03/2021 17.31, Alex Elder wrote:
On 3/26/21 10:22 AM, Rasmus Villemoes wrote:
Including a nul byte in the otherwise human-readable ascii output from this debugfs file is probably not intended.
Looking only at the comments above simple_read_from_buffer(), the last argument is the size of the buffer (tmp_buf in this case). So "3" is proper.
The size of the buffer is 3 because that's what sprintf() is gonna need to print one digit, '\n' and a nul byte. That doesn't necessarily imply that the entire buffer is meant to be sent to userspace.
But looking at callers, it seems that the trailing NUL is often excluded this way.
I don't really have a problem with your patch, but could you explain why having the NUL byte included is an actual problem? A short statement about that would provide better context than just "probably not intended."
debugfs files are AFAIK intended to be used with simple "cat foo", "echo 1 > foo" in shell (scripts and interactive). Having non-printable characters returned from that "cat foo" is odd (and can sometimes break scripts that e.g. "grep 1 foo/*/*/bar" when grep stops because it thinks one of the files is binary, or when the output of that is further piped somewhere).
At the very least, it's inconsistent for this one, those in greybus/svc.c do just return the ascii digits and the newline (and if one followed your argument above and let those pass 16 instead of desc one would leak a few bytes of uninitialized kernel stack to userspace).
I said "probably not intended" because for all I know, it might be intentional. I just doubt it.
Rasmus
On 3/26/21 12:05 PM, Rasmus Villemoes wrote:
On 26/03/2021 17.31, Alex Elder wrote:
On 3/26/21 10:22 AM, Rasmus Villemoes wrote:
Including a nul byte in the otherwise human-readable ascii output from this debugfs file is probably not intended.
Looking only at the comments above simple_read_from_buffer(), the last argument is the size of the buffer (tmp_buf in this case). So "3" is proper.
The size of the buffer is 3 because that's what sprintf() is gonna need to print one digit, '\n' and a nul byte. That doesn't necessarily imply that the entire buffer is meant to be sent to userspace.
But looking at callers, it seems that the trailing NUL is often excluded this way.
I don't really have a problem with your patch, but could you explain why having the NUL byte included is an actual problem? A short statement about that would provide better context than just "probably not intended."
My point was really that you should have provided a better explanation in your description.
At this point it's been discussed enough so I won't ask you to post version 2.
Acked-by: Alex Elder elder@linaro.org
debugfs files are AFAIK intended to be used with simple "cat foo", "echo 1 > foo" in shell (scripts and interactive). Having non-printable characters returned from that "cat foo" is odd (and can sometimes break scripts that e.g. "grep 1 foo/*/*/bar" when grep stops because it thinks one of the files is binary, or when the output of that is further piped somewhere).
At the very least, it's inconsistent for this one, those in greybus/svc.c do just return the ascii digits and the newline (and if one followed your argument above and let those pass 16 instead of desc one would leak a few bytes of uninitialized kernel stack to userspace).
I said "probably not intended" because for all I know, it might be intentional. I just doubt it.
Rasmus
-
Alex Elder -
Rasmus Villemoes