| Message ID | 20250401192420169tLRsDis5R0RrVmdFnFuS9@zte.com.cn (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | tty: serial: samsung: Fix potential buffer overflow in clkname | expand |
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
> 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
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,
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
>>> 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
> 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
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))