[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

[jiang.peng9]

> 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.
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.
Would you prefer any adjustments to this approach?

Best regards

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

[jiang.peng9]

>>> 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"?

Hi jirislaby,

Thanks for the follow-up! Let me clarify:

Since num_clks is an unsigned char, it could technically go up to 255. The compiler’s analysis seems to align with this.

While I’m not sure if real-world usage ever needs 3-digit values (like 100+), addressing the warning proactively seems worthwhile.

The change eliminates the compiler warning while adding minimal overhead. Better safe than sorry!

Happy to refine this further if you’d prefer a different approach.

Best regards
Peng Jiang

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

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

[jiang.peng9]

> No need, it was already on the lists many times.
> Same comments as for previous cases. Go search other discussions.

Hi,

Understood. I'll drop this patch.

Apologies for the inconvenience, and thank you and the other maintainers for your time and patience.

Best regards
Peng Jiang