The debugfs buffers in gb_camera (data[PAGE_SIZE], length) are written with sprintf without any bounds checking. The four places in gb_camera_debugfs_capabilities, gb_camera_debugfs_configure_streams and gb_camera_debugfs_flush do:
buffer->length += sprintf(buffer->data + buffer->length, ...); buffer->length = sprintf(buffer->data, ...);
If the formatted data ever grows (e.g., more streams, larger hex dump) or if length is already close to PAGE_SIZE, this will overrun the PAGE_SIZE buffer and corrupt memory. The driver is debugfs-only so the impact is limited, but it is still a real bug and the pattern is repeated in multiple places.
Fix it by using scnprintf with the remaining size:
scnprintf(buffer->data + buffer->length, PAGE_SIZE - buffer->length, ...) scnprintf(buffer->data, PAGE_SIZE, ...)
This is the standard way to write to a fixed-size buffer in the kernel. It guarantees we never write past PAGE_SIZE and will truncate instead of overrunning, which is safe for debugfs output. The return value still accumulates in length, which matches the existing use with simple_read_from_buffer (it will just show truncated output rather than corrupting).
I checked that this exact conversion has not been proposed before: the recent greybus conversions to sysfs_emit (light.c, gbphy.c) and fbtft/vme_tsi148 scnprintf patches do not touch camera.c at all, and a search of lore for "gb_camera_debugfs" shows no prior patch for these four sprintf sites.
No functional change for normal sizes, just makes the code safe if the buffer ever fills up.
Signed-off-by: Vaibhav Agarwal contectforbusiness@proton.me --- drivers/staging/greybus/camera.c | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-)
diff --git a/drivers/staging/greybus/camera.c b/drivers/staging/greybus/camera.c index 62b55bb28..efc83ceff 100644 --- a/drivers/staging/greybus/camera.c +++ b/drivers/staging/greybus/camera.c @@ -890,7 +890,8 @@ static ssize_t gb_camera_debugfs_capabilities(struct gb_camera *gcam, for (i = 0; i < size; i += 16) { unsigned int nbytes = min_t(unsigned int, size - i, 16);
- buffer->length += sprintf(buffer->data + buffer->length, + buffer->length += scnprintf(buffer->data + buffer->length, + PAGE_SIZE - buffer->length, "%*ph\n", nbytes, caps + i); }
@@ -973,12 +974,13 @@ static ssize_t gb_camera_debugfs_configure_streams(struct gb_camera *gcam, if (ret < 0) goto done;
- buffer->length = sprintf(buffer->data, "%u;%u;", nstreams, flags); + buffer->length = scnprintf(buffer->data, PAGE_SIZE, "%u;%u;", nstreams, flags);
for (i = 0; i < nstreams; ++i) { struct gb_camera_stream_config *stream = &streams[i];
- buffer->length += sprintf(buffer->data + buffer->length, + buffer->length += scnprintf(buffer->data + buffer->length, + PAGE_SIZE - buffer->length, "%u;%u;%u;%u;%u;%u;%u;", stream->width, stream->height, stream->format, stream->vc, @@ -1046,7 +1048,7 @@ static ssize_t gb_camera_debugfs_flush(struct gb_camera *gcam, if (ret < 0) return ret;
- buffer->length = sprintf(buffer->data, "%u", req_id); + buffer->length = scnprintf(buffer->data, PAGE_SIZE, "%u", req_id);
return len; }
Your From header is wrong.
On Wed, Sep 09, 2026 at 05:15:54PM +0000, contectforbusiness@proton.me wrote:
The debugfs buffers in gb_camera (data[PAGE_SIZE], length) are written with sprintf without any bounds checking. The four places in gb_camera_debugfs_capabilities, gb_camera_debugfs_configure_streams and gb_camera_debugfs_flush do:
buffer->length += sprintf(buffer->data + buffer->length, ...); buffer->length = sprintf(buffer->data, ...);
If the formatted data ever grows (e.g., more streams, larger hex dump) or if length is already close to PAGE_SIZE, this will overrun the PAGE_SIZE buffer and corrupt memory. The driver is debugfs-only so the impact is limited, but it is still a real bug and the pattern is repeated in multiple places.
Fix it by using scnprintf with the remaining size:
scnprintf(buffer->data + buffer->length, PAGE_SIZE - buffer->length, ...) scnprintf(buffer->data, PAGE_SIZE, ...)
This is the standard way to write to a fixed-size buffer in the kernel. It guarantees we never write past PAGE_SIZE and will truncate instead of overrunning, which is safe for debugfs output. The return value still accumulates in length, which matches the existing use with simple_read_from_buffer (it will just show truncated output rather than corrupting).
I checked that this exact conversion has not been proposed before: the recent greybus conversions to sysfs_emit (light.c, gbphy.c) and fbtft/vme_tsi148 scnprintf patches do not touch camera.c at all, and a search of lore for "gb_camera_debugfs" shows no prior patch for these four sprintf sites.
This paragraph is meta commentary. It doesn't belong in the commit message. It should be put under the --- or just omitted.
No functional change for normal sizes, just makes the code safe if the buffer ever fills up.
This is a frustrating sentence because it means "no changes except the changes."
The commit message wiffle-waffles between saying that it might be possible to overflow the buffer now, or it might be able to overflow the buffer in the future... Someone needs to do this analysis. Debugfs is root only, but if this were really a buffer overflow in the current code then we would need a Fixes tag.
Signed-off-by: Vaibhav Agarwal contectforbusiness@proton.me
drivers/staging/greybus/camera.c | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-)
diff --git a/drivers/staging/greybus/camera.c b/drivers/staging/greybus/camera.c index 62b55bb28..efc83ceff 100644 --- a/drivers/staging/greybus/camera.c +++ b/drivers/staging/greybus/camera.c @@ -890,7 +890,8 @@ static ssize_t gb_camera_debugfs_capabilities(struct gb_camera *gcam, for (i = 0; i < size; i += 16) { unsigned int nbytes = min_t(unsigned int, size - i, 16);
buffer->length += sprintf(buffer->data + buffer->length,
buffer->length += scnprintf(buffer->data + buffer->length,PAGE_SIZE - buffer->length,
Better to use "sizeof(buffer->data) - buffer->length".
"%*ph\n", nbytes, caps + i);}
The size parameter of this loop is 1024 or less (I think, I didn't track this all the way, but I assume operation->response->payload_size is less than original size). We are printing 3 characters per byte so that's only 3k which is less than PAGE_SIZE.
So this code is fine as-is. Still, I do like future proofing. Remove all mentions of "potential buffer overflows" because future buffer overflows do not count and we assume that future programmers are not stupid. But using scnprintf() makes the code safer and easier to audit.
@@ -973,12 +974,13 @@ static ssize_t gb_camera_debugfs_configure_streams(struct gb_camera *gcam, if (ret < 0) goto done;
- buffer->length = sprintf(buffer->data, "%u;%u;", nstreams, flags);
- buffer->length = scnprintf(buffer->data, PAGE_SIZE, "%u;%u;", nstreams, flags);
for (i = 0; i < nstreams; ++i) {
Here nstreams is like 4. It's not a concern.
struct gb_camera_stream_config *stream = &streams[i];
buffer->length += sprintf(buffer->data + buffer->length,
buffer->length += scnprintf(buffer->data + buffer->length,PAGE_SIZE - buffer->length, "%u;%u;%u;%u;%u;%u;%u;", stream->width, stream->height, stream->format, stream->vc,@@ -1046,7 +1048,7 @@ static ssize_t gb_camera_debugfs_flush(struct gb_camera *gcam, if (ret < 0) return ret;
- buffer->length = sprintf(buffer->data, "%u", req_id);
- buffer->length = scnprintf(buffer->data, PAGE_SIZE, "%u", req_id);
And this is obviously safe as-is as well.
regards, dan carpenter
Hi Dan,
Thanks for the detailed review.
- Fixed the From header so it matches the Signed-off-by. - Dropped the review-style commentary from the commit message. - Switched every site to sizeof(buffer->data) instead of PAGE_SIZE (I'd only done this in one of the four spots in v1 - fixed the other three too). - You're right that none of these can actually overflow today: capabilities tops out around 3 bytes per input byte on a payload that's at most ~1KB, nstreams is on the order of 4, and req_id is just a u32 printed as a string. So no Fixes tag, and I reworded the commit message so it doesn't reference any current or hypothetical overflow - it's a plain style/robustness change now.
v2 below.
Thanks, Vaibhav
---
From: Vaibhav Agarwal contectforbusiness@proton.me Subject: [PATCH v2] staging: greybus: camera: use scnprintf() for debugfs buffers
The debugfs buffers in gb_camera are written with sprintf(). Use scnprintf() with sizeof(buffer->data) instead, which is the usual kernel pattern for writing into a fixed-size buffer and makes the code easier to audit.
No functional change.
Signed-off-by: Vaibhav Agarwal contectforbusiness@proton.me --- Changes in v2: - Fix From header to match Signed-off-by - Drop review-thread commentary from the commit message - Use sizeof(buffer->data) instead of PAGE_SIZE at all four sites (v1 only did this at one of the four) - Reword commit message to not reference any overflow, current or hypothetical - none of the three call sites can overflow today
drivers/staging/greybus/camera.c | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-)
diff --git a/drivers/staging/greybus/camera.c b/drivers/staging/greybus/camera.c index 62b55bb28..0e9d7642a 100644 --- a/drivers/staging/greybus/camera.c +++ b/drivers/staging/greybus/camera.c @@ -890,7 +890,8 @@ static ssize_t gb_camera_debugfs_capabilities(struct gb_camera *gcam, for (i = 0; i < size; i += 16) { unsigned int nbytes = min_t(unsigned int, size - i, 16);
- buffer->length += sprintf(buffer->data + buffer->length, + buffer->length += scnprintf(buffer->data + buffer->length, + sizeof(buffer->data) - buffer->length, "%*ph\n", nbytes, caps + i); }
@@ -973,12 +974,13 @@ static ssize_t gb_camera_debugfs_configure_streams(struct gb_camera *gcam, if (ret < 0) goto done;
- buffer->length = sprintf(buffer->data, "%u;%u;", nstreams, flags); + buffer->length = scnprintf(buffer->data, sizeof(buffer->data), "%u;%u;", nstreams, flags);
for (i = 0; i < nstreams; ++i) { struct gb_camera_stream_config *stream = &streams[i];
- buffer->length += sprintf(buffer->data + buffer->length, + buffer->length += scnprintf(buffer->data + buffer->length, + sizeof(buffer->data) - buffer->length, "%u;%u;%u;%u;%u;%u;%u;", stream->width, stream->height, stream->format, stream->vc, @@ -1046,7 +1048,7 @@ static ssize_t gb_camera_debugfs_flush(struct gb_camera *gcam, if (ret < 0) return ret;
- buffer->length = sprintf(buffer->data, "%u", req_id); + buffer->length = scnprintf(buffer->data, sizeof(buffer->data), "%u", req_id);
return len; }
-
contectforbusiness＠proton.me -
Dan Carpenter