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

[PATCH] staging: fbtft: prefer scnprintf 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
scnprintf, passing sizeof() of the fixed-size stack buffers
(text1[50] and text2[50]) so the write is always bounded.

In practice an overflow is very unlikely here: text1 only needs
room for a size_t value from an SPI TX buffer length, and text2
formats three small integers (bus number, chip select, and
frequency in MHz) that always come from real hardware ranges far
below the theoretical worst case for their types. This is
therefore a defense-in-depth hardening rather than a fix for an
observed or easily triggered issue.

Signed-off-by: Tomasz Unger <tomasz.unger@yahoo.pl>
---
Verified with checkpatch.pl - no errors or warnings.
Compiled the fbtft module successfully with CONFIG_FB_TFT=m
(also required enabling CONFIG_FB=m, CONFIG_SPI=y and
CONFIG_GPIOLIB=y, all previously disabled).
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). This confirms the
modules load cleanly but does not exercise the changed code
path, which would require an actual SPI-connected TFT display.
---
 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..95c92ad55e98 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);
+		scnprintf(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);
+		scnprintf(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: 20260819-fbtft-core-scnprintf-1fcf0426c834

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

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

[Andy Shevchenko]

On Wed, Aug 19, 2026 at 7:45 PM Tomasz Unger <tomasz.unger@yahoo.pl> wrote:
>
> Using sprintf has potential for buffer overflows if the formatted

sprintf()

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

scnprintf()

> (text1[50] and text2[50]) so the write is always bounded.

> In practice an overflow is very unlikely here: text1 only needs
> room for a size_t value from an SPI TX buffer length, and text2
> formats three small integers (bus number, chip select, and
> frequency in MHz) that always come from real hardware ranges far
> below the theoretical worst case for their types. This is
> therefore a defense-in-depth hardening rather than a fix for an
> observed or easily triggered issue.

This paragraph is not for the commit message, rather for the comment.

...

>         if (par->txbuf.buf && par->txbuf.len >= 1024)
> -               sprintf(text1, ", %zu KiB buffer memory", par->txbuf.len >> 10);

The 64-bit number takes up to 20 decimal digits, the rest here is 20
and one for NUL terminator. 50 is more than enough.

> +               scnprintf(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,

For this it might be worse, 3 32-bit integers that may take up to 10
decimal digits + sign, so 33 altogether and the rest is 13 and one for
NUL terminator, so still below 50 in the longest case.

> -                       spi_get_chipselect(spi, 0), spi->max_speed_hz / 1000000);
> +               scnprintf(text2, sizeof(text2), ", spi%d.%d at %d MHz", spi->controller->bus_num,
> +                         spi_get_chipselect(spi, 0), spi->max_speed_hz / 1000000);

Taking the above (and if there are no compiler warnings currently
present) this change is an unneeded churn.

-- 
With Best Regards,
Andy Shevchenko

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

[Dan Carpenter]

On Wed, Aug 19, 2026 at 08:06:39PM +0300, Andy Shevchenko wrote:
> > (text1[50] and text2[50]) so the write is always bounded.
> 
> > In practice an overflow is very unlikely here: text1 only needs
> > room for a size_t value from an SPI TX buffer length, and text2
> > formats three small integers (bus number, chip select, and
> > frequency in MHz) that always come from real hardware ranges far
> > below the theoretical worst case for their types. This is
> > therefore a defense-in-depth hardening rather than a fix for an
> > observed or easily triggered issue.
> 
> This paragraph is not for the commit message, rather for the comment.
> 

The paragraph describes the expected impact of the patch and it
belongs in the commit message.  My problem is with it is that it's
1) Wrong because it says unlikely instead of impossible. And 2) goes
into too much unnecessary detail. We don't need to know where the
values come from since even the worst case for the type could fit.

regards,
dan carpenter

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

[Dan Carpenter]

On Wed, Aug 19, 2026 at 06:44:44PM +0200, Tomasz Unger wrote:
> Using sprintf has potential for buffer overflows if the formatted
> string exceeds the destination buffer size. Replace it with
> scnprintf, passing sizeof() of the fixed-size stack buffers
> (text1[50] and text2[50]) so the write is always bounded.
> 
> In practice an overflow is very unlikely here: text1 only needs

I don't have a problem with the patch, because I think making the
code easy to audit is helpful.  However, change "unlikely" to
"impossible".

18446744073709551616  KiB buffer memory
123456789 123456789 123456789 1234567890

spi-4294967296.-4294967296 at -4294967296 MHz
123456789 123456789 123456789 123456789 1234567890

The other thing is that scnprintf() is only for if we care about
the return.  Otherwise default to snprintf().  It's just a more
traditional choice.

regards,
dan carpenter