[PATCH v2] staging: fbtft: prefer snprintf over sprintf in fbtft-core.c

[PATCH v2] staging: fbtft: prefer snprintf over sprintf in fbtft-core.c

[Tomasz Unger]

Using sprintf has potential for buffer overflows if the formatted
string exceeds the destination buffer size. Replace it with
snprintf, passing sizeof() of the fixed-size stack buffers
(text1[50] and text2[50]) so the write is always bounded.

An overflow is impossible here: even the worst case for the
argument types (size_t for text1, three ints for text2) still
fits within 50 bytes. Use snprintf() rather than scnprintf()
since the return value is not used here.

Signed-off-by: Tomasz Unger <tomasz.unger@yahoo.pl>
---
Changes in v2 (per Dan Carpenter's review):
- Switched from scnprintf() to snprintf(), since the return
  value is unused.
- Reworded the commit message: overflow is impossible, not
  merely unlikely, and dropped detail about where the values
  come from.

Verified with checkpatch.pl - no errors or warnings.
Compiled the fbtft module successfully with CONFIG_FB_TFT=m.
fb.ko, syscopyarea.ko, sysimgblt.ko, sysfillrect.ko,
fb_sys_fops.ko and fbtft.ko all load without errors in a QEMU
environment (verified via insmod and lsmod, dmesg shows no
errors).
---
 drivers/staging/fbtft/fbtft-core.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/staging/fbtft/fbtft-core.c b/drivers/staging/fbtft/fbtft-core.c
index ca0c38221c16..9ea0442a337e 100644
--- a/drivers/staging/fbtft/fbtft-core.c
+++ b/drivers/staging/fbtft/fbtft-core.c
@@ -784,10 +784,10 @@ int fbtft_register_framebuffer(struct fb_info *fb_info)
 	fbtft_sysfs_init(par);
 
 	if (par->txbuf.buf && par->txbuf.len >= 1024)
-		sprintf(text1, ", %zu KiB buffer memory", par->txbuf.len >> 10);
+		snprintf(text1, sizeof(text1), ", %zu KiB buffer memory", par->txbuf.len >> 10);
 	if (spi)
-		sprintf(text2, ", spi%d.%d at %d MHz", spi->controller->bus_num,
-			spi_get_chipselect(spi, 0), spi->max_speed_hz / 1000000);
+		snprintf(text2, sizeof(text2), ", spi%d.%d at %d MHz", spi->controller->bus_num,
+			 spi_get_chipselect(spi, 0), spi->max_speed_hz / 1000000);
 	fb_dbg(fb_info,
 	       "%s frame buffer, %dx%d, %d KiB video memory%s, fps=%lu%s\n",
 	       fb_info->fix.id, fb_info->var.xres, fb_info->var.yres,

---
base-commit: 8d3ae59288f1e7d58d76558a6ee96d533bc5019f
change-id: 20260820-fbtft-v2-final-7891edd1d87e

Best regards,
--  
Tomasz Unger <tomasz.unger@yahoo.pl>

Re: [PATCH v2] staging: fbtft: prefer snprintf over sprintf in fbtft-core.c

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [High] Integer overflow in `vmem_size` calculation leads to a small allocation, causing subsequent out-of-bounds reads/writes.
--

--- Patch [1]: [PATCH v2] staging: fbtft: prefer snprintf over sprintf in fbtft-core.c ---
commit c1e6190003fe313b23e0964b72df871f88f45da7
Author: Tomasz Unger <tomasz.unger@yahoo.pl>

staging: fbtft: prefer snprintf over sprintf in fbtft-core.c

This commit replaces sprintf with snprintf for formatting stack buffers 
text1 and text2 in fbtft-core.c. This prevents potential buffer overflows 
even though the arguments theoretically fit within the 50-byte limits.

> diff --git a/drivers/staging/fbtft/fbtft-core.c b/drivers/staging/fbtft/fbtft-core.c
> index ca0c38221c16b..9ea0442a337e5 100644
> --- a/drivers/staging/fbtft/fbtft-core.c
> +++ b/drivers/staging/fbtft/fbtft-core.c
> @@ -784,10 +784,10 @@ int fbtft_register_framebuffer(struct fb_info *fb_info)
>  	fbtft_sysfs_init(par);
>  
>  	if (par->txbuf.buf && par->txbuf.len >= 1024)
> -		sprintf(text1, ", %zu KiB buffer memory", par->txbuf.len >> 10);
> +		snprintf(text1, sizeof(text1), ", %zu KiB buffer memory", par->txbuf.len >> 10);

[Severity: High]
This isn't a bug introduced by this patch, but while reviewing the memory
safety improvements in this file, I noticed a potential integer overflow in
fbtft_framebuffer_alloc() prior to memory allocation:

drivers/staging/fbtft/fbtft-core.c:fbtft_framebuffer_alloc() {
    ...
    vmem_size = display->width * display->height * bpp / 8;
    vmem = vzalloc(vmem_size);
    if (!vmem)
        goto release_framebuf;
    ...
}

Could this calculation wrap around 32-bit arithmetic if sufficiently large
dimensions (e.g., width=65544, height=4096, bpp=16) are provided from the
device tree?

If display->width * display->height * bpp overflows, vmem_size could end up
as a small positive integer, leading to a small buffer allocation by vzalloc().

Later in fbtft_update_display(), the line length is calculated as
width * bpp / 8, which easily fits in 32 bits without overflowing. Does this
mean operations might attempt to read or write lengths larger than the
allocated vmem buffer, causing an out-of-bounds access?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260820-fbtft-v2-final-v2-1-44d107b04634@yahoo.pl?part=1