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