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

[Dan Carpenter <error27@gmail.com>]

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