serial: samsung: fix buffer size for clk_name

Commit Message

Alexey V. Vissarionov Jan. 16, 2023, 2:16 p.m. UTC

Although very unlikely, the 'clk_num' value may be as big as
2**32 - 1 (uint32_max), so the buffer should have enough
space for storing "clk_uart_baud4294967295\0".
Also, the numbers in clk_name are expected to be unsigned.

Found by Linux Verification Center (linuxtesting.org) with SVACE.

Fixes: 5f5a7a5578c58852 ("serial: samsung: switch to clkdev based clock lookup")
Signed-off-by: Alexey V. Vissarionov <[e-mail]>

Comments

Krzysztof Kozlowski Jan. 16, 2023, 4:16 p.m. UTC | #1

On 16/01/2023 15:16, Alexey V. Vissarionov wrote:
> Although very unlikely, the 'clk_num' value may be as big as
> 2**32 - 1 (uint32_max), 

How it could be that high? Code has num_clks defined from 1 to 4 and it
is used as strict boundary for the loop so how it could end up here with
higher value?

s3c24xx_serial_getsource() also returns value & with mask, so up to 4
max. This does not look like real issue but some change to satisfy
static code analyzers, so I don't think it's correct approach.

Best regards,
Krzysztof

Alexey V. Vissarionov Jan. 16, 2023, 6:26 p.m. UTC | #2

On 2023-01-16 17:16:59 +0100, Krzysztof Kozlowski wrote:

 >> Although very unlikely, the 'clk_num' value may be as big
 >> as 2**32 - 1 (uint32_max),
 > How it could be that high? Code has num_clks defined from 1
 > to 4 and it is used as strict boundary for the loop so how
 > it could end up here with higher value?

That's why I called this "very unlikely". However, nobody can
definitely exclude the possibility of extending this limit in
the future.

 > s3c24xx_serial_getsource() also returns value & with mask,
 > so up to 4 max.

Possibly, clk_num should be uint8_t then - so the buffer size
could be extended up to only 17 bytes ("clk_uart_baud255\0")
with format specifiers changed to "%u", or 18 bytes for "%d"
(clk_uart_baud-128\0).

 > This does not look like real issue but some change to satisfy
 > static code analyzers, so I don't think it's correct approach.

Although I agree this is probably only a theoretical issue, it's
always easier to spend several bytes than to prove that we don't
need to.

But, anyway, the final decision is up to you.

Greg KH Jan. 19, 2023, 2:52 p.m. UTC | #3

On Mon, Jan 16, 2023 at 05:16:58PM +0300, Alexey V. Vissarionov wrote:
> Although very unlikely, the 'clk_num' value may be as big as
> 2**32 - 1 (uint32_max), so the buffer should have enough
> space for storing "clk_uart_baud4294967295\0".
> Also, the numbers in clk_name are expected to be unsigned.
> 
> Found by Linux Verification Center (linuxtesting.org) with SVACE.
> 
> Fixes: 5f5a7a5578c58852 ("serial: samsung: switch to clkdev based clock lookup")

Please fix your scripts to use the proper number of SHA1 digits in a
Fixes: line as the documentation asks for.

thanks,

greg k-h

Patch

diff --git a/drivers/tty/serial/samsung_tty.c b/drivers/tty/serial/samsung_tty.c
index 0fce856434dafd80..2c701dc7c6a37191 100644
--- a/drivers/tty/serial/samsung_tty.c
+++ b/drivers/tty/serial/samsung_tty.c
@@ -1407,7 +1407,7 @@  static void s3c24xx_serial_pm(struct uart_port *port, unsigned int level,
  *
  */
 
-#define MAX_CLK_NAME_LENGTH 15
+#define MAX_CLK_NAME_LENGTH 24		/* "clk_uart_baud4294967295\0" */
 
 static inline int s3c24xx_serial_getsource(struct uart_port *port)
 {
@@ -1457,7 +1457,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);
+		sprintf(clkname, "clk_uart_baud%u", cnt);
 		clk = clk_get(ourport->port.dev, clkname);
 		if (IS_ERR(clk))
 			continue;
@@ -1957,7 +1957,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);
+		sprintf(clk_name, "clk_uart_baud%u", clk_num);
 		clk = clk_get(dev, clk_name);
 		if (IS_ERR(clk))
 			continue;
@@ -2522,7 +2522,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);
+		sprintf(clk_name, "clk_uart_baud%u", clk_sel);
 
 		clk = clk_get(port->dev, clk_name);
 		if (!IS_ERR(clk))