The refill_buf function uses snprintf to append to a fixed-size buffer.
snprintf returns the length that would have been written, which can
exceed the remaining buffer size. If this happens, ptr advances beyond
the buffer and rem becomes negative. In the 2nd iteration, rem is
treated as a large unsigned integer, causing snprintf to write oob.

While this behavior is technically mitigated by num_perfcntrs being
locked at 5, it's still unsafe if num_perfcntrs were ever to change/a
second source was added.

Signed-off-by: veygax <veyga@veygax.dev>
---
 drivers/gpu/drm/msm/msm_perf.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/msm/msm_perf.c b/drivers/gpu/drm/msm/msm_perf.c
index d3c7889aaf26..c369d4acc378 100644
--- a/drivers/gpu/drm/msm/msm_perf.c
+++ b/drivers/gpu/drm/msm/msm_perf.c
@@ -65,13 +65,13 @@ static int refill_buf(struct msm_perf_state *perf)
 
 	if ((perf->cnt++ % 32) == 0) {
 		/* Header line: */
-		n = snprintf(ptr, rem, "%%BUSY");
+		n = scnprintf(ptr, rem, "%%BUSY");
 		ptr += n;
 		rem -= n;
 
 		for (i = 0; i < gpu->num_perfcntrs; i++) {
 			const struct msm_gpu_perfcntr *perfcntr = &gpu->perfcntrs[i];
-			n = snprintf(ptr, rem, "\t%s", perfcntr->name);
+			n = scnprintf(ptr, rem, "\t%s", perfcntr->name);
 			ptr += n;
 			rem -= n;
 		}
@@ -93,21 +93,21 @@ static int refill_buf(struct msm_perf_state *perf)
 			return ret;
 
 		val = totaltime ? 1000 * activetime / totaltime : 0;
-		n = snprintf(ptr, rem, "%3d.%d%%", val / 10, val % 10);
+		n = scnprintf(ptr, rem, "%3d.%d%%", val / 10, val % 10);
 		ptr += n;
 		rem -= n;
 
 		for (i = 0; i < ret; i++) {
 			/* cycle counters (I think).. convert to MHz.. */
 			val = cntrs[i] / 10000;
-			n = snprintf(ptr, rem, "\t%5d.%02d",
+			n = scnprintf(ptr, rem, "\t%5d.%02d",
 					val / 100, val % 100);
 			ptr += n;
 			rem -= n;
 		}
 	}
 
-	n = snprintf(ptr, rem, "\n");
+	n = scnprintf(ptr, rem, "\n");
 	ptr += n;
 	rem -= n;
 
-- 
2.52.0

On Sun, 30 Nov 2025 01:28:54 +0000, veygax wrote:
> The refill_buf function uses snprintf to append to a fixed-size buffer.
> snprintf returns the length that would have been written, which can
> exceed the remaining buffer size. If this happens, ptr advances beyond
> the buffer and rem becomes negative. In the 2nd iteration, rem is
> treated as a large unsigned integer, causing snprintf to write oob.
> 
> While this behavior is technically mitigated by num_perfcntrs being
> locked at 5, it's still unsafe if num_perfcntrs were ever to change/a
> second source was added.
> 
> [...]

Applied to msm-fixes, thanks!

[1/1] drm/msm: Replace unsafe snprintf usage with scnprintf
      https://gitlab.freedesktop.org/lumag/msm/-/commit/093cbd754382

Best regards,
-- 
With best wishes
Dmitry

On 24/12/2025 09:27, Dmitry Baryshkov wrote:
> [1/1] drm/msm: Replace unsafe snprintf usage with scnprintf
>       https://gitlab.freedesktop.org/lumag/msm/-/commit/093cbd754382

Hi Dmitry,

Apologies but I've just noticed that I signed-off with my screen name
instead of my real name. Would you like me to submit a patch V2 fixing this?

-- 
- Evan Lambert / veygax

On Wed, Dec 24, 2025 at 12:07:29PM +0000, veygax wrote:
> On 24/12/2025 09:27, Dmitry Baryshkov wrote:
> > [1/1] drm/msm: Replace unsafe snprintf usage with scnprintf
> >       https://gitlab.freedesktop.org/lumag/msm/-/commit/093cbd754382
> 
> Hi Dmitry,
> 
> Apologies but I've just noticed that I signed-off with my screen name
> instead of my real name. Would you like me to submit a patch V2 fixing this?

And I didn't notice it... Yes, please, send v2.

-- 
With best wishes
Dmitry

On Sun, Nov 30, 2025 at 01:28:54AM +0000, veygax wrote:
> The refill_buf function uses snprintf to append to a fixed-size buffer.
> snprintf returns the length that would have been written, which can
> exceed the remaining buffer size. If this happens, ptr advances beyond
> the buffer and rem becomes negative. In the 2nd iteration, rem is
> treated as a large unsigned integer, causing snprintf to write oob.
> 
> While this behavior is technically mitigated by num_perfcntrs being
> locked at 5, it's still unsafe if num_perfcntrs were ever to change/a
> second source was added.
> 
> Signed-off-by: veygax <veyga@veygax.dev>
> ---
>  drivers/gpu/drm/msm/msm_perf.c | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)
> 

Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@oss.qualcomm.com>


-- 
With best wishes
Dmitry