diff mbox series

[v2,1/2] printk: Check valid console index for preferred console

Message ID 20231011074330.14487-1-tony@atomide.com
State New
Headers
Series [v2,1/2] printk: Check valid console index for preferred console |

Commit Message

Tony Lindgren Oct. 11, 2023, 7:43 a.m. UTC 
  Let's check for valid console index values to avoid bogus console index
numbers from kernel command line. While struct console uses short for
index, and negative index values are used by some device drivers, we do
not want to allow negative values for preferred console.

Let's change the idx to short to match struct console, and return an error
on negative values. And let's also constify idx while at it.

Signed-off-by: Tony Lindgren <tony@atomide.com>
---

Changes since v1:

- Use const short idx and return an error on negative values

---
 include/linux/console.h |  2 +-
 kernel/printk/printk.c  | 11 +++++++++--
 2 files changed, 10 insertions(+), 3 deletions(-)

Comments

Greg KH Oct. 11, 2023, 7:53 a.m. UTC | #1 
On Wed, Oct 11, 2023 at 10:43:25AM +0300, Tony Lindgren wrote:
> Let's check for valid console index values to avoid bogus console index
> numbers from kernel command line. While struct console uses short for
> index, and negative index values are used by some device drivers, we do
> not want to allow negative values for preferred console.

What drivers use a negative index for the console?

> Let's change the idx to short to match struct console, and return an error
> on negative values. And let's also constify idx while at it.
> 
> Signed-off-by: Tony Lindgren <tony@atomide.com>
> ---
> 
> Changes since v1:
> 
> - Use const short idx and return an error on negative values
> 
> ---
>  include/linux/console.h |  2 +-
>  kernel/printk/printk.c  | 11 +++++++++--
>  2 files changed, 10 insertions(+), 3 deletions(-)
> 
> diff --git a/include/linux/console.h b/include/linux/console.h
> --- a/include/linux/console.h
> +++ b/include/linux/console.h
> @@ -340,7 +340,7 @@ enum con_flush_mode {
>  	CONSOLE_REPLAY_ALL,
>  };
>  
> -extern int add_preferred_console(char *name, int idx, char *options);
> +extern int add_preferred_console(char *name, const short idx, char *options);
>  extern void console_force_preferred_locked(struct console *con);
>  extern void register_console(struct console *);
>  extern int unregister_console(struct console *);
> diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
> --- a/kernel/printk/printk.c
> +++ b/kernel/printk/printk.c
> @@ -2404,12 +2404,19 @@ static void set_user_specified(struct console_cmdline *c, bool user_specified)
>  	console_set_on_cmdline = 1;
>  }
>  
> -static int __add_preferred_console(char *name, int idx, char *options,
> +static int __add_preferred_console(const char *name, const short idx, char *options,
>  				   char *brl_options, bool user_specified)
>  {
>  	struct console_cmdline *c;
>  	int i;
>  
> +	/*
> +	 * Negative struct console index may be valid for drivers in some cases,
> +	 * but negative index is not valid for a preferred console.
> +	 */
> +	if (idx < 0)
> +		return -EINVAL;

Looks good to me, I'll take this through my tty tree if no one objects.

thanks,

greg k-h
Tony Lindgren Oct. 11, 2023, 9:18 a.m. UTC | #2 
* Greg Kroah-Hartman <gregkh@linuxfoundation.org> [231011 07:53]:
> On Wed, Oct 11, 2023 at 10:43:25AM +0300, Tony Lindgren wrote:
> > Let's check for valid console index values to avoid bogus console index
> > numbers from kernel command line. While struct console uses short for
> > index, and negative index values are used by some device drivers, we do
> > not want to allow negative values for preferred console.
> 
> What drivers use a negative index for the console?

This is based on grepping with $ git grep "co->index =" drivers/tty/

Not sure what all might be stopping making struct console index unsigned.

Regards,

Tony
Petr Mladek Oct. 11, 2023, 3:26 p.m. UTC | #3 
On Wed 2023-10-11 12:18:03, Tony Lindgren wrote:
> * Greg Kroah-Hartman <gregkh@linuxfoundation.org> [231011 07:53]:
> > On Wed, Oct 11, 2023 at 10:43:25AM +0300, Tony Lindgren wrote:
> > > Let's check for valid console index values to avoid bogus console index
> > > numbers from kernel command line. While struct console uses short for
> > > index, and negative index values are used by some device drivers, we do
> > > not want to allow negative values for preferred console.
> > 
> > What drivers use a negative index for the console?
> 
> This is based on grepping with $ git grep "co->index =" drivers/tty/
> 
> Not sure what all might be stopping making struct console index unsigned.

The value -1 is used for initializing struct console, see:

$> git grep -A10 "struct console.*=" | \
   grep -e "struct console" -e index | \
   grep -B1 index
[...]
drivers/tty/serial/8250/8250_core.c:static struct console univ8250_console = {
drivers/tty/serial/8250/8250_core.c-    .index          = -1,
[...]
drivers/tty/serial/imx.c:static struct console imx_uart_console = {
drivers/tty/serial/imx.c-       .index          = -1,
drivers/tty/serial/ip22zilog.c:static struct console ip22zilog_console = {
drivers/tty/serial/ip22zilog.c- .index  =       -1,
drivers/tty/serial/kgdb_nmi.c:static struct console kgdb_nmi_console = {
drivers/tty/serial/kgdb_nmi.c-  .index  = -1,
drivers/tty/serial/lantiq.c:static struct console lqasc_console = {
drivers/tty/serial/lantiq.c-    .index =        -1,
drivers/tty/serial/liteuart.c:static struct console liteuart_console = {
drivers/tty/serial/liteuart.c-  .index = -1,
drivers/tty/serial/lpc32xx_hs.c:static struct console lpc32xx_hsuart_console = {
drivers/tty/serial/lpc32xx_hs.c-        .index          = -1,
drivers/tty/serial/ma35d1_serial.c:static struct console ma35d1serial_console = {
drivers/tty/serial/ma35d1_serial.c-     .index   = -1,
drivers/tty/serial/mcf.c:static struct console mcf_console = {
drivers/tty/serial/mcf.c-       .index          = -1,
[...]

It means that the index still has be get assigned. For example, it is
used here:

static int try_enable_preferred_console(struct console *newcon,
					bool user_specified)
{
[...]
			if (newcon->index < 0)
				newcon->index = c->index;
[...]


Resume:

1. We must either keep signed short in struct console or
   use another check for non-yet assigned index.

2. We should fix the commit message and the comment. We should
   explain that negative value is used in struct console
   to distinguish a non-yet-registered/assigned index/port.

Best Regards,
Petr
Tony Lindgren Oct. 11, 2023, 4:17 p.m. UTC | #4 
* Petr Mladek <pmladek@suse.com> [231011 15:26]:
> On Wed 2023-10-11 12:18:03, Tony Lindgren wrote:
> > * Greg Kroah-Hartman <gregkh@linuxfoundation.org> [231011 07:53]:
> > > On Wed, Oct 11, 2023 at 10:43:25AM +0300, Tony Lindgren wrote:
> > > > Let's check for valid console index values to avoid bogus console index
> > > > numbers from kernel command line. While struct console uses short for
> > > > index, and negative index values are used by some device drivers, we do
> > > > not want to allow negative values for preferred console.
> > > 
> > > What drivers use a negative index for the console?
> > 
> > This is based on grepping with $ git grep "co->index =" drivers/tty/
> > 
> > Not sure what all might be stopping making struct console index unsigned.
> 
> The value -1 is used for initializing struct console, see:
> 
> $> git grep -A10 "struct console.*=" | \
>    grep -e "struct console" -e index | \
>    grep -B1 index
> [...]
> drivers/tty/serial/8250/8250_core.c:static struct console univ8250_console = {
> drivers/tty/serial/8250/8250_core.c-    .index          = -1,
> [...]
> drivers/tty/serial/imx.c:static struct console imx_uart_console = {
> drivers/tty/serial/imx.c-       .index          = -1,
> drivers/tty/serial/ip22zilog.c:static struct console ip22zilog_console = {
> drivers/tty/serial/ip22zilog.c- .index  =       -1,
> drivers/tty/serial/kgdb_nmi.c:static struct console kgdb_nmi_console = {
> drivers/tty/serial/kgdb_nmi.c-  .index  = -1,
> drivers/tty/serial/lantiq.c:static struct console lqasc_console = {
> drivers/tty/serial/lantiq.c-    .index =        -1,
> drivers/tty/serial/liteuart.c:static struct console liteuart_console = {
> drivers/tty/serial/liteuart.c-  .index = -1,
> drivers/tty/serial/lpc32xx_hs.c:static struct console lpc32xx_hsuart_console = {
> drivers/tty/serial/lpc32xx_hs.c-        .index          = -1,
> drivers/tty/serial/ma35d1_serial.c:static struct console ma35d1serial_console = {
> drivers/tty/serial/ma35d1_serial.c-     .index   = -1,
> drivers/tty/serial/mcf.c:static struct console mcf_console = {
> drivers/tty/serial/mcf.c-       .index          = -1,
> [...]
> 
> It means that the index still has be get assigned. For example, it is
> used here:
> 
> static int try_enable_preferred_console(struct console *newcon,
> 					bool user_specified)
> {
> [...]
> 			if (newcon->index < 0)
> 				newcon->index = c->index;
> [...]

OK

> Resume:
> 
> 1. We must either keep signed short in struct console or
>    use another check for non-yet assigned index.

OK thanks for clarifying the usage for it.

> 2. We should fix the commit message and the comment. We should
>    explain that negative value is used in struct console
>    to distinguish a non-yet-registered/assigned index/port.

I'll send v3 patches tomorrow with updated commit message and comments.

Thanks,

Tony
Jiri Slaby Oct. 12, 2023, 5:53 a.m. UTC | #5 
On 11. 10. 23, 9:43, Tony Lindgren wrote:
> Let's check for valid console index values to avoid bogus console index
> numbers from kernel command line. While struct console uses short for
> index, and negative index values are used by some device drivers, we do
> not want to allow negative values for preferred console.
> 
> Let's change the idx to short to match struct console, and return an error
> on negative values. And let's also constify idx while at it.
> 
> Signed-off-by: Tony Lindgren <tony@atomide.com>
...
> --- a/kernel/printk/printk.c
> +++ b/kernel/printk/printk.c
> @@ -2404,12 +2404,19 @@ static void set_user_specified(struct console_cmdline *c, bool user_specified)
>   	console_set_on_cmdline = 1;
>   }
>   
> -static int __add_preferred_console(char *name, int idx, char *options,
> +static int __add_preferred_console(const char *name, const short idx, char *options,

This "const char *name" change is somehow not related to 1/2, but to 
2/2, I suppose.


thanks,
Tony Lindgren Oct. 12, 2023, 6:24 a.m. UTC | #6 
* Jiri Slaby <jirislaby@kernel.org> [231012 05:54]:
> On 11. 10. 23, 9:43, Tony Lindgren wrote:
> > Let's check for valid console index values to avoid bogus console index
> > numbers from kernel command line. While struct console uses short for
> > index, and negative index values are used by some device drivers, we do
> > not want to allow negative values for preferred console.
> > 
> > Let's change the idx to short to match struct console, and return an error
> > on negative values. And let's also constify idx while at it.
> > 
> > Signed-off-by: Tony Lindgren <tony@atomide.com>
> ...
> > --- a/kernel/printk/printk.c
> > +++ b/kernel/printk/printk.c
> > @@ -2404,12 +2404,19 @@ static void set_user_specified(struct console_cmdline *c, bool user_specified)
> >   	console_set_on_cmdline = 1;
> >   }
> > -static int __add_preferred_console(char *name, int idx, char *options,
> > +static int __add_preferred_console(const char *name, const short idx, char *options,
> 
> This "const char *name" change is somehow not related to 1/2, but to 2/2, I
> suppose.

Thanks I just noticed that too, that sneaked in while I was resolving a
merge conflict while changing the order of the two patches.

Regards,

Tony
diff mbox series

Patch

diff --git a/include/linux/console.h b/include/linux/console.h
--- a/include/linux/console.h
+++ b/include/linux/console.h
@@ -340,7 +340,7 @@  enum con_flush_mode {
 	CONSOLE_REPLAY_ALL,
 };
 
-extern int add_preferred_console(char *name, int idx, char *options);
+extern int add_preferred_console(char *name, const short idx, char *options);
 extern void console_force_preferred_locked(struct console *con);
 extern void register_console(struct console *);
 extern int unregister_console(struct console *);
diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
--- a/kernel/printk/printk.c
+++ b/kernel/printk/printk.c
@@ -2404,12 +2404,19 @@  static void set_user_specified(struct console_cmdline *c, bool user_specified)
 	console_set_on_cmdline = 1;
 }
 
-static int __add_preferred_console(char *name, int idx, char *options,
+static int __add_preferred_console(const char *name, const short idx, char *options,
 				   char *brl_options, bool user_specified)
 {
 	struct console_cmdline *c;
 	int i;
 
+	/*
+	 * Negative struct console index may be valid for drivers in some cases,
+	 * but negative index is not valid for a preferred console.
+	 */
+	if (idx < 0)
+		return -EINVAL;
+
 	/*
 	 *	See if this tty is not yet registered, and
 	 *	if we have a slot free.
@@ -2513,7 +2520,7 @@  __setup("console=", console_setup);
  * commonly to provide a default console (ie from PROM variables) when
  * the user has not supplied one.
  */
-int add_preferred_console(char *name, int idx, char *options)
+int add_preferred_console(char *name, const short idx, char *options)
 {
 	return __add_preferred_console(name, idx, options, NULL, false);
 }