[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

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

[Andy Shevchenko]

On Thu, Aug 20, 2026 at 06:58:12PM +0200, Tomasz Unger wrote:
> Using sprintf has potential for buffer overflows if the formatted

sprintf()

> string exceeds the destination buffer size. Replace it with
> snprintf, passing sizeof() of the fixed-size stack buffers

snprintf()

> (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.

Then why the patch is needed at all? Do you have any compiler that warns
you about something?

...

It looks like you ignored all my comments against v1...

-- 
With Best Regards,
Andy Shevchenko

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

[Tomasz Unger]

[-- Attachment #1: Type: text/plain, Size: 1350 bytes --]

 
Correct - no compiler or static analysis tool flags this. The overflow is mathematically impossible for these argument types, so the patch doesn't fix an actual bug.

I addressed Dan's specific wording feedback but missed responding to your core objection about the patch being unnecessary. Given the above, I'll drop this patch unless you see value in keeping it as a documented safety margin.

Regards,
Tomasz Unger
    W poniedziałek, 24 sierpnia 2026 10:59:40 CEST, Andy Shevchenko <andriy.shevchenko@intel.com> napisał(-a):  
 
 On Thu, Aug 20, 2026 at 06:58:12PM +0200, Tomasz Unger wrote:
> Using sprintf has potential for buffer overflows if the formatted

sprintf()

> string exceeds the destination buffer size. Replace it with
> snprintf, passing sizeof() of the fixed-size stack buffers

snprintf()

> (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.

Then why the patch is needed at all? Do you have any compiler that warns
you about something?

...

It looks like you ignored all my comments against v1...

-- 
With Best Regards,
Andy Shevchenko


  

[-- Attachment #2: Type: text/html, Size: 2152 bytes --]