[PATCH] staging: greybus: camera: fix potential overflow in debugfs buffers

[PATCH] staging: greybus: camera: fix potential overflow in debugfs buffers

[contectforbusiness]

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;
 }

Re: [PATCH] staging: greybus: camera: fix potential overflow in debugfs buffers

[Dan Carpenter]

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

Re: [PATCH] staging: greybus: camera: fix potential overflow in debugfs buffers

[contectforbusiness]

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;
 }