| Message ID | 20230116141658.GC8107@altlinux.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | serial: samsung: fix buffer size for clk_name | expand |
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
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.
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
On 01/02/2023 03:44, Alexey V. Vissarionov wrote: > serial: samsung: fix buffer size for clk_name This does not belong here. > > 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 ALT Linux Team (altlinux.org) and Linux Verification > Center (linuxtesting.org) using SVACE. > > Signed-off-by: Alexey V. Vissarionov <gremlin@altlinux.org> > Fixes: 5f5a7a5578c5 ("serial: samsung: switch to clkdev based clock lookup") How did you address my comments? the clk_num cannot be beig as 2^32-1. It's not possible, if I understand correctly. > > --- > > On 2023-01-19 15:52:38 +0100, Greg Kroah-Hartman wrote: > > >> 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. > > Done. Also added the comment to the source regarding the buffer size. > > > 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" */ NAK. It's just waste of space. Best regards, Krzysztof
On Wed, Feb 01, 2023 at 05:44:57AM +0300, Alexey V. Vissarionov wrote: > serial: samsung: fix buffer size for clk_name > > 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 ALT Linux Team (altlinux.org) and Linux Verification > Center (linuxtesting.org) using SVACE. > > Signed-off-by: Alexey V. Vissarionov <gremlin@altlinux.org> > Fixes: 5f5a7a5578c5 ("serial: samsung: switch to clkdev based clock lookup") > > --- > > On 2023-01-19 15:52:38 +0100, Greg Kroah-Hartman wrote: > > >> 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. > > Done. Also added the comment to the source regarding the buffer size. > > > 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); So you bump the size of the buffer and continue to use an "unsafe" call that could overflow the buffer? Is this a plan to submit a series of patches all "fixing" something based on the last change? :) As Krzysztof said, this whole thing is not needed at all. Please fix your tool to generate valid changes. thanks, greg k-h
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))
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 <gremlin@altlinux.org>