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

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

[Tony Lindgren]

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(-)

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);
 }
-- 
2.42.0

[PATCH v2 2/2] printk: Constify name for add_preferred_console()

[Tony Lindgren]

While adding a preferred console handling for serial_core for serial port
hardware based device addressing, Jiri suggested we constify name for
add_preferred_console(). The name gets copied anyways. This allows serial
core to add a preferred console using serial drv->dev_name without copying
it.

Note that constifying options causes changes all over the place because of
struct console for match().

Suggested-by: Jiri Slaby <jirislaby@kernel.org>
Reviewed-by: Petr Mladek <pmladek@suse.com>
Signed-off-by: Tony Lindgren <tony@atomide.com>
---

Changes since v1:

- Updated to apply on the const short idx change

- Separated out of the serial core related patches, the serial core
  changes still need some more changes for preferred console usage

- Added Reviewed-by from Petr from the serial core thread

---
 include/linux/console.h | 2 +-
 kernel/printk/printk.c  | 2 +-
 2 files changed, 2 insertions(+), 2 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, const short idx, char *options);
+extern int add_preferred_console(const 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
@@ -2520,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, const short idx, char *options)
+int add_preferred_console(const char *name, const short idx, char *options)
 {
 	return __add_preferred_console(name, idx, options, NULL, false);
 }
-- 
2.42.0

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

[Greg Kroah-Hartman]

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

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

[Tony Lindgren]

* 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

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

[Petr Mladek]

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

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

[Tony Lindgren]

* 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

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

[Jiri Slaby]

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,
-- 
js
suse labs

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

[Tony Lindgren]

* 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