Re: [PATCH v2 1/4] serial: core: do fallible allocations before the console can be registered

[Greg Kroah-Hartman <gregkh@linuxfoundation.org>]

On Mon, Jul 20, 2026 at 12:10:11AM +0200, Karl Mehltretter wrote:
> serial_core_add_one_port() allocates uport->tty_groups after
> uart_configure_port() has already registered the port's console. If
> that allocation fails, the function returns -ENOMEM with the console
> still registered, and the driver's probe error path then tears down
> the port state the console callbacks depend on.
> 
> Reproduced with fault injection on qemu's raspi1ap board. Failing the
> tty_groups allocation during a PL011 sysfs bind makes
> uart_add_one_port() return -ENOMEM. pl011_register_port() then clears
> amba_ports[0], but ttyAMA0 remains registered as a console. The nbcon
> printer thread dereferences the NULL entry and oopses:
> 
>   Unhandled fault: page domain fault (0x01b) at 0x00000178
>   CPU: 0 UID: 0 PID: 43 Comm: pr/ttyAMA0 Not tainted 7.2.0-rc3+ #1
>   PC is at pl011_console_write_thread+0x2c/0x168
> 
> This is not PL011-specific: the failing allocation is in serial core,
> after uart_configure_port() has registered the console, so any console
> UART driver is exposed. On i.MX the retained console references a
> devm-allocated port that the failed probe frees, causing a
> use-after-free. Reproduced on qemu's mcimx6ul-evk using the same
> fail-nth harness under KASAN:
> 
>   BUG: KASAN: slab-use-after-free in imx_uart_console_write_thread+0x50/0x278
>   Read of size 4 at addr c5246048 by task pr/ttymxc0/63
>    imx_uart_console_write_thread from nbcon_emit_next_record+0x360/0x50c
>    nbcon_emit_next_record from nbcon_emit_one+0x140/0x184
>   Allocated by task 1:
>    devm_kmalloc from imx_uart_probe+0x90/0xa5c
>   Freed by task 1:
>    devres_release_all from device_unbind_cleanup+0x38/0xdc
>    device_unbind_cleanup from really_probe+0x2b4/0x388
> 
> The pre-existing kasprintf() failure path has a related problem: it
> returns with state->uart_port already pointing at a port whose probe
> is about to unwind and free it.
> 
> Reorder the function so the uport->name and uport->tty_groups
> allocations both happen before the port is linked into the driver
> state table and before uart_configure_port() registers the console:
> 
>  1. Allocate uport->name.
>  2. Allocate the tty_groups array with room for three entries
>     unconditionally (serial core group, optional driver group, NULL
>     terminator). The optional group cannot be examined at this point:
>     config_port() may only supply uport->attr_group during
>     uart_configure_port(), e.g. 8250 sets it after autodetection.
>  3. Only then link the port into the driver state table and run
>     uart_configure_port().
>  4. Fill in the optional attr_group slot afterwards.
> 
> A fail-nth sweep over the whole bind path on both boards left the
> console unregistered after every failed bind and did not reproduce the
> i.MX use-after-free.
> 
> Fixes: 266dcff03eed ("Serial: allow port drivers to have a default attribute group")
> Fixes: f7048b15900f ("tty: serial_core: Add name field to uart_port struct")
> Cc: stable@vger.kernel.org

As this can only be duplicated with fault-injection, it really doesn't
need to go to any stable kernels, right?



> Assisted-by: Claude:claude-fable-5
> Signed-off-by: Karl Mehltretter <kmehltretter@gmail.com>
> ---
>  drivers/tty/serial/serial_core.c | 31 +++++++++++++++++--------------
>  1 file changed, 17 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c
> index a530ad372b43..887b1dd80ad2 100644
> --- a/drivers/tty/serial/serial_core.c
> +++ b/drivers/tty/serial/serial_core.c
> @@ -3056,7 +3056,6 @@ static int serial_core_add_one_port(struct uart_driver *drv, struct uart_port *u
>  	struct uart_state *state;
>  	struct tty_port *port;
>  	struct device *tty_dev;
> -	int num_groups;
>  
>  	if (uport->line >= drv->nr)
>  		return -EINVAL;
> @@ -3068,6 +3067,23 @@ static int serial_core_add_one_port(struct uart_driver *drv, struct uart_port *u
>  	if (state->uart_port)
>  		return -EINVAL;
>  
> +	uport->name = kasprintf(GFP_KERNEL, "%s%u", drv->dev_name,
> +				drv->tty_driver->name_base + uport->line);
> +	if (!uport->name)
> +		return -ENOMEM;
> +
> +	/*
> +	 * uart_configure_port() may set uport->attr_group and register the
> +	 * console. Allocate room for both groups and a NULL terminator first.
> +	 */
> +	uport->tty_groups = kzalloc_objs(*uport->tty_groups, 3);
> +	if (!uport->tty_groups) {
> +		kfree(uport->name);
> +		uport->name = NULL;

Why set this to NULL?

thanks,

greg k-h