Re: [PATCH] printk: fix out-of-bounds access in try_enable_preferred_console()

[Petr Mladek]

(Once again with corrected LKML address. I am sorry for noice.)

Adding other printk subsystem reviewers into Cc.

There is get_maintainer.pl script for this purpose. For example:

$> ./scripts/get_maintainer.pl kernel/printk/printk.c
Petr Mladek <pmladek@xxxxxxxx> (maintainer:PRINTK)
Steven Rostedt <rostedt@xxxxxxxxxxx> (reviewer:PRINTK)
John Ogness <john.ogness@xxxxxxxxxxxxx> (reviewer:PRINTK)
Sergey Senozhatsky <senozhatsky@xxxxxxxxxxxx> (reviewer:PRINTK)
linux-kernel@xxxxxxxxxxxxxxx (open list)

On Sat 2026-05-30 10:18:24, Naveen Kumar Chaudhary wrote:
> When all MAX_CMDLINECONSOLES (8) slots in console_cmdline[] are occupied
> and none match the newly registered console, the for loop exits with
> i == MAX_CMDLINECONSOLES and c pointing past the end of the array. The
> subsequent access to c->user_specified is then an out-of-bounds read.

Great catch!

> This can occur when a self-enabling console (one with CON_ENABLED already
> set), such as netconsole or pstore, calls register_console() on a system
> where the console_cmdline[] array has been filled by a combination of
> command-line console= parameters, ACPI SPCR, device tree stdout-path,
> and/or arch-specific add_preferred_console() calls.
> 
> Add a bounds check to ensure c is only dereferenced when the loop exited
> due to finding an empty slot (i.e., c still points within the array).
> Also add parentheses around the bitwise-AND to silence compiler warnings
> about its use in a boolean context.

But the fix is is not correct, see below.

> --- a/kernel/printk/printk.c
> +++ b/kernel/printk/printk.c
> @@ -3938,7 +3938,8 @@ static int try_enable_preferred_console(struct console *newcon,
>  	 * without matching. Accept the pre-enabled consoles only when match()
>  	 * and setup() had a chance to be called.
>  	 */
> -	if (newcon->flags & CON_ENABLED && c->user_specified ==	user_specified)
> +	if (i < MAX_CMDLINECONSOLES && (newcon->flags & CON_ENABLED) &&
> +	    c->user_specified == user_specified)

This would prevent the out-of-bound access to c->user_specified.

But the check of c->user_specified does _not_ make sense in the first
place.

Background:
-----------

The idea was that we would allow to match a preferred console
and run newcon->setup(). By other words, this code should
be called in register_console() after

	/* See if this console matches one we selected on the command line */
	err = try_enable_preferred_console(newcon, true);

	/* If not, try to match against the platform default(s) */
	if (err == -ENOENT)
		err = try_enable_preferred_console(newcon, false);

, when try_enable_preferred_console() did not return a real error.
The real error is an error from newcon->setup(). Note that -ENOENT
is not meant as a real error here.

Solution:
---------

IMHO, the right approach is to move this code out of
try_enable_preferred_console(). Like it is done in my clean up of
the registration code. I have just sent v3 earlier today, see
https://lore.kernel.org/all/20260602085312.228251-10-pmladek@xxxxxxxx/


Impact:
-------

Now, the question is how serious this bug is.
Do we need to fix it now?
Or is it enough to wait for the clean up?

IMHO, the only danger is that the kernel might trigger an invalid
access and panic(). But is this possible?

console_cmdline[] is stored in the initialized data segment. I guess
that there are always another "valid" static data right after it.
So, it should "never" trigger an invalid access.

Reading invalid data should not cause big problems.
In the worst case, the console will get enabled in
try_enable_preferred_console(newcon, true) instead
of try_enable_preferred_console(newcon, true) and
newcon->setup() won't get caller. But I think that pre-enabled
console drivers should never rely on calling this, anyway.

Finally, it is hard to imagine that anyone would use all 8 slots
for preferred console entries. So, this should be almost impossible
to hit in practice.

That said, I think about backporting the 10th patch from the
above mentioned clean up and fixing this ASAP. Better be
safe than sorry...

Best Regards,
Petr

>  		return 0;
>  
>  	return -ENOENT;
> -- 
> 2.43.0