[PATCH] tty: serial: samsung: Fix potential buffer overflow in clkname

[PATCH] tty: serial: samsung: Fix potential buffer overflow in clkname

[shao.mingyin]

From: Peng Jiang <jiang.peng9@zte.com.cn>

Compiling the kernel with gcc12.3 W=1 produces a warning:
/drivers/tty/serial/samsung_tty.c: In function
's3c24xx_serial_set_termios':

/drivers/tty/serial/samsung_tty.c:1392:48:
warning: '%d' directive writing between 1 and 3 bytes
into a region of size 2 [-Wformat-overflow=]
 1392 |  sprintf(clkname, "clk_uart_baud%d", cnt);
      |                    ^~

In function 's3c24xx_serial_getclk',
    inlined from 's3c24xx_serial_set_termios'
at ./drivers/tty/serial/samsung_tty.c:1493:9:

/drivers/tty/serial/samsung_tty.c:1392:34:
note: directive argument in the range [0, 254]
 1392 |  sprintf(clkname, "clk_uart_baud%d", cnt);
      |                    ^~~~~~~~~~~~~~~~~

/drivers/tty/serial/samsung_tty.c:1392:17:
note: 'sprintf' output between 15 and 17 bytes
into a destination of size 15

 1392 |  sprintf(clkname, "clk_uart_baud%d", cnt);
      |                   ^~~~~~~~~~~~~~~~~

The compiler warned about a potential buffer overflow in the
`s3c24xx_serial_set_termios` function due to the use of `sprintf` which
could write more bytes than the allocated size of the `clkname` buffer.
This could lead to undefined behavior and potential security risks.

To reproduce the issue before applying the patch:
CONFIG_SERIAL_SAMSUNG=y
make vmlinux ARCH=arm64 CROSS_COMPILE=aarch64-linux- W=1

To resolve this issue, we have increased the buffer size for `clkname`
to ensure it can accommodate the longest possible string generated by
the formatting operation. Additionally, we have replaced `sprintf` with
`snprintf` to ensure that the function does not write beyond the end of
the buffer, thus preventing any potential overflow.

Signed-off-by: Peng Jiang <jiang.peng9@zte.com.cn>
Signed-off-by: Shao Mingyin <shao.mingyin@zte.com.cn>
---
 drivers/tty/serial/samsung_tty.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/tty/serial/samsung_tty.c b/drivers/tty/serial/samsung_tty.c
index 210fff7164c1..5a0005033afa 100644
--- a/drivers/tty/serial/samsung_tty.c
+++ b/drivers/tty/serial/samsung_tty.c
@@ -1339,7 +1339,7 @@ static void s3c24xx_serial_pm(struct uart_port *port, unsigned int level,
  *
  */

-#define MAX_CLK_NAME_LENGTH 15
+#define MAX_CLK_NAME_LENGTH 18

 static inline u8 s3c24xx_serial_getsource(struct uart_port *port)
 {
@@ -1389,7 +1389,7 @@ static unsigned int s3c24xx_serial_getclk(struct s3c24xx_uart_port *ourport,
 			!(ourport->cfg->clk_sel & (1 << cnt)))
 			continue;

-		sprintf(clkname, "clk_uart_baud%d", cnt);
+		snprintf(clkname, sizeof(clkname), "clk_uart_baud%d", cnt);
 		clk = clk_get(ourport->port.dev, clkname);
 		if (IS_ERR(clk))
 			continue;
@@ -1787,7 +1787,7 @@ static int s3c24xx_serial_enable_baudclk(struct s3c24xx_uart_port *ourport)
 		if (!(clk_sel & (1 << clk_num)))
 			continue;

-		sprintf(clk_name, "clk_uart_baud%d", clk_num);
+		snprintf(clk_name, sizeof(clk_name), "clk_uart_baud%d", clk_num);
 		clk = clk_get(dev, clk_name);
 		if (IS_ERR(clk))
 			continue;
@@ -2335,7 +2335,7 @@ s3c24xx_serial_get_options(struct uart_port *port, int *baud,
 		/* now calculate the baud rate */

 		clk_sel = s3c24xx_serial_getsource(port);
-		sprintf(clk_name, "clk_uart_baud%d", clk_sel);
+		snprintf(clk_name, sizeof(clk_name), "clk_uart_baud%d", clk_sel);

 		clk = clk_get(port->dev, clk_name);
 		if (!IS_ERR(clk))
-- 
2.25.1

Re: [PATCH] tty: serial: samsung: Fix potential buffer overflow in clkname

[Krzysztof Kozlowski]

On 01/04/2025 13:24, shao.mingyin@zte.com.cn wrote:
> From: Peng Jiang <jiang.peng9@zte.com.cn>
> 
> Compiling the kernel with gcc12.3 W=1 produces a warning:
> /drivers/tty/serial/samsung_tty.c: In function
> 's3c24xx_serial_set_termios':
> 
> /drivers/tty/serial/samsung_tty.c:1392:48:
> warning: '%d' directive writing between 1 and 3 bytes
> into a region of size 2 [-Wformat-overflow=]
>  1392 |  sprintf(clkname, "clk_uart_baud%d", cnt);

Same comments as with other patches, not possible, IMO. Plus this patch
looks actually worse - commit msg is hardly readable.

Best regards,
Krzysztof

Re: [PATCH] tty: serial: samsung: Fix potential buffer overflow in clkname

[Jiri Slaby]

FWIW Your e-mail client broke threading.

On 02. 04. 25, 3:33, jiang.peng9@zte.com.cn wrote:
>> Same comments as with other patches, not possible, IMO. Plus this patch
>> looks actually worse - commit msg is hardly readable.
>>
>> Best regards,
>> Krzysztof
> 
> Hi Krzysztof,
> Thank you for your feedback. Let me briefly re-explain the change:
> The issue:
> When building with W=1, we get a format-overflow warning because "clk_uart_baud%d" could write 15-17 bytes (14 chars + 1-3 digits) into a 15-byte buffer.

Hi,

how did you come to "1-3 digits"?

thanks,
-- 
js
suse labs

Re: [PATCH] tty: serial: samsung: Fix potential buffer overflow in clkname

[Krzysztof Kozlowski]

On 02/04/2025 03:33, jiang.peng9@zte.com.cn wrote:
>> Same comments as with other patches, not possible, IMO. Plus this patch
>> looks actually worse - commit msg is hardly readable.
>>
>> Best regards,
>> Krzysztof
> 
> Hi Krzysztof,
> Thank you for your feedback. Let me briefly re-explain the change:

No need, it was already on the lists many times.

> The issue:
> When building with W=1, we get a format-overflow warning because "clk_uart_baud%d" could write 15-17 bytes (14 chars + 1-3 digits) into a 15-byte buffer.
> The fix:
> Increased clkname buffer size from 15 to 18 chars
> (original 14 chars + 3 digits + null = 18)
> Replaced sprintf() with snprintf() for safety
> This keeps the pattern consistent while eliminating the warning. Tested with CONFIG_SERIAL_SAMSUNG=y builds.

That's not a test. Test is running on a device. Compiling is not a test.

> Would you prefer any adjustments to this approach?
Same comments as for previous cases. Go search other discussions.

Best regards,
Krzysztof