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

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

[Karl Mehltretter]

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
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;
+		return -ENOMEM;
+	}
+	uport->tty_groups[0] = &tty_dev_attr_group;
+
 	/* Link the port to the driver state table and vice versa */
 	atomic_set(&state->refcount, 1);
 	init_waitqueue_head(&state->remove_wait);
@@ -3084,10 +3100,6 @@ static int serial_core_add_one_port(struct uart_driver *drv, struct uart_port *u
 	state->pm_state = UART_PM_STATE_UNDEFINED;
 	uart_port_set_cons(uport, drv->cons);
 	uport->minor = drv->tty_driver->minor_start + uport->line;
-	uport->name = kasprintf(GFP_KERNEL, "%s%u", drv->dev_name,
-				drv->tty_driver->name_base + uport->line);
-	if (!uport->name)
-		return -ENOMEM;
 
 	if (uport->cons && uport->dev)
 		of_console_check(uport->dev->of_node, uport->cons->name, uport->line);
@@ -3102,15 +3114,6 @@ static int serial_core_add_one_port(struct uart_driver *drv, struct uart_port *u
 
 	port->console = uart_console(uport);
 
-	num_groups = 2;
-	if (uport->attr_group)
-		num_groups++;
-
-	uport->tty_groups = kzalloc_objs(*uport->tty_groups, num_groups);
-	if (!uport->tty_groups)
-		return -ENOMEM;
-
-	uport->tty_groups[0] = &tty_dev_attr_group;
 	if (uport->attr_group)
 		uport->tty_groups[1] = uport->attr_group;
 
-- 
2.53.0

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

[Greg Kroah-Hartman]

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

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

[Karl Mehltretter]

On Thu, Jul 30, 2026 at 04:28:10PM +0100, Greg Kroah-Hartman wrote:
> > 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?

Agreed, I'll drop it.

> 
> > +	if (!uport->tty_groups) {
> > +		kfree(uport->name);
> > +		uport->name = NULL;
> 
> Why set this to NULL?
> 

It's not necessary. I will remove it.

Thanks,
Karl

[PATCH v2 2/4] serial: core: clear freed pointers on uart_register_driver() failure

[Karl Mehltretter]

uart_register_driver() leaves freed pointers behind on failure. If
tty_alloc_driver() fails, it frees drv->state without clearing it. If
tty_register_driver() fails, it also drops the tty driver reference
without clearing drv->tty_driver.

Several drivers register the uart_driver lazily and use drv->state as
an "already registered" sentinel. After a failed registration, the
next probe sees the stale pointer, skips re-registration and calls
uart_add_one_port() with freed state.

The resulting unwind can call uart_unregister_driver() with a NULL or
dangling drv->tty_driver and oops in tty_unregister_driver():

  Unhandled fault: page domain fault (0x01b) at 0x00000018
  PC is at tty_unregister_driver+0x10/0x68
  LR is at uart_unregister_driver+0x1c/0x60

Reproduced with failslab fail-nth injection on qemu's raspi1ap board:
fail the tty_alloc_driver() allocation during a sysfs bind of the
PL011 port, then bind again in the same boot.

Clear drv->state after freeing the state array and clear
drv->tty_driver after dropping the tty driver reference, as
uart_unregister_driver() already does.

The tty_register_driver() failure case predates Git history. The
tty_alloc_driver() failure case was introduced by commit 9e845abfc8a8
("serial: fix NULL pointer dereference"), which made that error path
return cleanly instead of crashing in put_tty_driver(NULL).

Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
Fixes: 9e845abfc8a8 ("serial: fix NULL pointer dereference")
Cc: stable@vger.kernel.org
Assisted-by: Claude:claude-fable-5
Signed-off-by: Karl Mehltretter <kmehltretter@gmail.com>
---
 drivers/tty/serial/serial_core.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c
index 887b1dd80ad2..ba9145c5a38a 100644
--- a/drivers/tty/serial/serial_core.c
+++ b/drivers/tty/serial/serial_core.c
@@ -2777,8 +2777,10 @@ int uart_register_driver(struct uart_driver *drv)
 	for (i = 0; i < drv->nr; i++)
 		tty_port_destroy(&drv->state[i].port);
 	tty_driver_kref_put(normal);
+	drv->tty_driver = NULL;
 out_kfree:
 	kfree(drv->state);
+	drv->state = NULL;
 out:
 	return retval;
 }
-- 
2.53.0

Re: [PATCH v2 2/4] serial: core: clear freed pointers on uart_register_driver() failure

[Greg Kroah-Hartman]

On Mon, Jul 20, 2026 at 12:10:12AM +0200, Karl Mehltretter wrote:
> uart_register_driver() leaves freed pointers behind on failure. If
> tty_alloc_driver() fails, it frees drv->state without clearing it. If
> tty_register_driver() fails, it also drops the tty driver reference
> without clearing drv->tty_driver.
> 
> Several drivers register the uart_driver lazily and use drv->state as
> an "already registered" sentinel. After a failed registration, the
> next probe sees the stale pointer, skips re-registration and calls
> uart_add_one_port() with freed state.
> 
> The resulting unwind can call uart_unregister_driver() with a NULL or
> dangling drv->tty_driver and oops in tty_unregister_driver():
> 
>   Unhandled fault: page domain fault (0x01b) at 0x00000018
>   PC is at tty_unregister_driver+0x10/0x68
>   LR is at uart_unregister_driver+0x1c/0x60
> 
> Reproduced with failslab fail-nth injection on qemu's raspi1ap board:
> fail the tty_alloc_driver() allocation during a sysfs bind of the
> PL011 port, then bind again in the same boot.
> 
> Clear drv->state after freeing the state array and clear
> drv->tty_driver after dropping the tty driver reference, as
> uart_unregister_driver() already does.
> 
> The tty_register_driver() failure case predates Git history. The
> tty_alloc_driver() failure case was introduced by commit 9e845abfc8a8
> ("serial: fix NULL pointer dereference"), which made that error path
> return cleanly instead of crashing in put_tty_driver(NULL).
> 
> Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
> Fixes: 9e845abfc8a8 ("serial: fix NULL pointer dereference")
> Cc: stable@vger.kernel.org

Same comment here, this can't ever be hit by a "normal" user, right?

thanks,

greg k-h

[PATCH v2 3/4] tty: don't oops in tty_unregister_device() when no cdev is registered

[Karl Mehltretter]

serial_core_add_one_port() keeps a uart_port when tty device registration
fails so setserial can still use it. It marks the port UPF_DEAD and
returns success. Removing the port later reaches tty_unregister_device(),
which unconditionally passes driver->cdevs[index] to cdev_del().

The slot does not always contain a live cdev. A serdev registration error
other than -ENODEV returns before tty_register_device_attr(), leaving the
slot NULL. If cdev_add() fails, tty_cdev_add() drops the cdev reference
but leaves the slot pointing at freed memory. The later cdev_del() is
therefore a NULL dereference or use-after-free.

The NULL path was reproduced with failslab during UART bind on qemu's
mcimx6ul-evk and raspi1ap boards:

  Unhandled fault: page domain fault (0x01b) at 0x00000038
  PC is at cdev_del+0x14/0x34

Clear the slot after cdev_add() fails and only call cdev_del() when it is
non-NULL. This makes a non-NULL slot mean that a live cdev is registered.

Fixes: c1a752ba2d6b ("tty: don't leak cdev in tty_cdev_add()")
Fixes: 8cde11b2baa1 ("tty/serdev: add serdev registration interface")
Cc: stable@vger.kernel.org
Assisted-by: Claude:claude-fable-5
Signed-off-by: Karl Mehltretter <kmehltretter@gmail.com>
---
 drivers/tty/tty_io.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/tty/tty_io.c b/drivers/tty/tty_io.c
index 6b283fd03ff8..4889076b975f 100644
--- a/drivers/tty/tty_io.c
+++ b/drivers/tty/tty_io.c
@@ -3167,8 +3167,10 @@ static int tty_cdev_add(struct tty_driver *driver, dev_t dev,
 	driver->cdevs[index]->ops = &tty_fops;
 	driver->cdevs[index]->owner = driver->owner;
 	err = cdev_add(driver->cdevs[index], dev, count);
-	if (err)
+	if (err) {
 		kobject_put(&driver->cdevs[index]->kobj);
+		driver->cdevs[index] = NULL;
+	}
 	return err;
 }
 
@@ -3305,7 +3307,7 @@ EXPORT_SYMBOL_GPL(tty_register_device_attr);
 void tty_unregister_device(struct tty_driver *driver, unsigned index)
 {
 	device_destroy(&tty_class, MKDEV(driver->major, driver->minor_start) + index);
-	if (!(driver->flags & TTY_DRIVER_DYNAMIC_ALLOC)) {
+	if (!(driver->flags & TTY_DRIVER_DYNAMIC_ALLOC) && driver->cdevs[index]) {
 		cdev_del(driver->cdevs[index]);
 		driver->cdevs[index] = NULL;
 	}
-- 
2.53.0

Re: [PATCH v2 3/4] tty: don't oops in tty_unregister_device() when no cdev is registered

[Greg Kroah-Hartman]

On Mon, Jul 20, 2026 at 12:10:13AM +0200, Karl Mehltretter wrote:
> serial_core_add_one_port() keeps a uart_port when tty device registration
> fails so setserial can still use it. It marks the port UPF_DEAD and
> returns success. Removing the port later reaches tty_unregister_device(),
> which unconditionally passes driver->cdevs[index] to cdev_del().
> 
> The slot does not always contain a live cdev. A serdev registration error
> other than -ENODEV returns before tty_register_device_attr(), leaving the
> slot NULL. If cdev_add() fails, tty_cdev_add() drops the cdev reference
> but leaves the slot pointing at freed memory. The later cdev_del() is
> therefore a NULL dereference or use-after-free.
> 
> The NULL path was reproduced with failslab during UART bind on qemu's
> mcimx6ul-evk and raspi1ap boards:
> 
>   Unhandled fault: page domain fault (0x01b) at 0x00000038
>   PC is at cdev_del+0x14/0x34
> 
> Clear the slot after cdev_add() fails and only call cdev_del() when it is
> non-NULL. This makes a non-NULL slot mean that a live cdev is registered.

Note, LLMs love to write a lot.  Please always rewrite the changelog
text to be sane and concise.

> 
> Fixes: c1a752ba2d6b ("tty: don't leak cdev in tty_cdev_add()")
> Fixes: 8cde11b2baa1 ("tty/serdev: add serdev registration interface")
> Cc: stable@vger.kernel.org
> Assisted-by: Claude:claude-fable-5
> Signed-off-by: Karl Mehltretter <kmehltretter@gmail.com>
> ---
>  drivers/tty/tty_io.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/tty/tty_io.c b/drivers/tty/tty_io.c
> index 6b283fd03ff8..4889076b975f 100644
> --- a/drivers/tty/tty_io.c
> +++ b/drivers/tty/tty_io.c
> @@ -3167,8 +3167,10 @@ static int tty_cdev_add(struct tty_driver *driver, dev_t dev,
>  	driver->cdevs[index]->ops = &tty_fops;
>  	driver->cdevs[index]->owner = driver->owner;
>  	err = cdev_add(driver->cdevs[index], dev, count);
> -	if (err)
> +	if (err) {
>  		kobject_put(&driver->cdevs[index]->kobj);
> +		driver->cdevs[index] = NULL;
> +	}
>  	return err;
>  }
>  
> @@ -3305,7 +3307,7 @@ EXPORT_SYMBOL_GPL(tty_register_device_attr);
>  void tty_unregister_device(struct tty_driver *driver, unsigned index)
>  {
>  	device_destroy(&tty_class, MKDEV(driver->major, driver->minor_start) + index);
> -	if (!(driver->flags & TTY_DRIVER_DYNAMIC_ALLOC)) {
> +	if (!(driver->flags & TTY_DRIVER_DYNAMIC_ALLOC) && driver->cdevs[index]) {

This looks to be doing 2 different things in one patch, or am I
confused?

thanks,

greg k-h

Re: [PATCH v2 3/4] tty: don't oops in tty_unregister_device() when no cdev is registered

[Karl Mehltretter]

On Thu, Jul 30, 2026 at 04:29:57PM +0100, Greg Kroah-Hartman wrote:
> Note, LLMs love to write a lot.  Please always rewrite the changelog
> text to be sane and concise.
> 

Understood. I'll clean it up more.

> This looks to be doing 2 different things in one patch, or am I
> confused?
> 

Good point. I'll split it into two for v3, with corresponding Fixes: tags.

Thanks,
Karl

[PATCH v2 4/4] serial: imx: serialize imx_uart_ports[] lifetime

[Karl Mehltretter]

imx_uart_probe() publishes the port in imx_uart_ports[] before
uart_add_one_port(), because console setup during that call uses the
table. The entry is not cleared if adding the port fails or after
imx_uart_remove() removes it.

The port is devm-allocated, so a failed probe or unbind leaves the table
pointing at freed memory. A later registration of the shared console can
then dereference the stale entry. Reproduced on QEMU's mcimx6ul-evk by
unbinding a sibling UART, unbinding the console UART and rebinding the
sibling:

  BUG: KASAN: slab-use-after-free in imx_uart_console_setup+0xd0/0x3d8

The entry must remain valid until uart_remove_one_port() unregisters the
console. Clearing it afterward without serialization still races a
sibling probe: the sibling can register the shared console using the
dying entry before it is cleared. The next console write then
dereferences NULL.

Use a driver-wide mutex to serialize table publication, port addition
and rollback with port removal and table clearing. Console callbacks
remain lockless because uart_add_one_port() may invoke setup while
probe holds the mutex.

The probe-failure path also relies on "serial: core: do fallible
allocations before the console can be registered", which moves the
uport->name and uport->tty_groups allocations before console
registration. Both changes should be backported together.

Fixes: dbff4e9ea2e8 ("IMX UART: remove statically initialized tables")
Fixes: 9f322ad064f9 ("imx: serial: handle initialisation failure correctly")
Reported-by: Sashiko <sashiko-bot@kernel.org>
Link: https://lore.kernel.org/all/20260719162850.043B41F000E9@smtp.kernel.org
Cc: stable@vger.kernel.org
Assisted-by: Claude:claude-fable-5
Signed-off-by: Karl Mehltretter <kmehltretter@gmail.com>
---
 drivers/tty/serial/imx.c | 21 +++++++++++++++++++--
 1 file changed, 19 insertions(+), 2 deletions(-)

diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c
index 251a50c8aa38..def874f9cd00 100644
--- a/drivers/tty/serial/imx.c
+++ b/drivers/tty/serial/imx.c
@@ -22,6 +22,7 @@
 #include <linux/clk.h>
 #include <linux/delay.h>
 #include <linux/ktime.h>
+#include <linux/mutex.h>
 #include <linux/pinctrl/consumer.h>
 #include <linux/rational.h>
 #include <linux/slab.h>
@@ -2080,6 +2081,15 @@ static const struct uart_ops imx_uart_pops = {
 
 static struct imx_port *imx_uart_ports[UART_NR];
 
+/*
+ * Store the port in imx_uart_ports[] before uart_add_one_port() and clear
+ * it only after uart_remove_one_port() returns. Console callbacks in both
+ * calls use the table, so this mutex serializes these sequences between
+ * sibling ports. Callbacks must not take it because uart_add_one_port()
+ * may invoke setup while it is held.
+ */
+static DEFINE_MUTEX(imx_uart_ports_lock);
+
 #if IS_ENABLED(CONFIG_SERIAL_IMX_CONSOLE)
 static void imx_uart_console_putchar(struct uart_port *port, unsigned char ch)
 {
@@ -2632,11 +2642,14 @@ static int imx_uart_probe(struct platform_device *pdev)
 		}
 	}
 
-	imx_uart_ports[sport->port.line] = sport;
-
 	platform_set_drvdata(pdev, sport);
 
+	mutex_lock(&imx_uart_ports_lock);
+	imx_uart_ports[sport->port.line] = sport;
 	ret = uart_add_one_port(&imx_uart_uart_driver, &sport->port);
+	if (ret)
+		imx_uart_ports[sport->port.line] = NULL;
+	mutex_unlock(&imx_uart_ports_lock);
 
 err_clk:
 	clk_disable_unprepare(sport->clk_ipg);
@@ -2647,8 +2660,12 @@ static int imx_uart_probe(struct platform_device *pdev)
 static void imx_uart_remove(struct platform_device *pdev)
 {
 	struct imx_port *sport = platform_get_drvdata(pdev);
+	unsigned int line = sport->port.line;
 
+	mutex_lock(&imx_uart_ports_lock);
 	uart_remove_one_port(&imx_uart_uart_driver, &sport->port);
+	imx_uart_ports[line] = NULL;
+	mutex_unlock(&imx_uart_ports_lock);
 }
 
 static void imx_uart_restore_context(struct imx_port *sport)
-- 
2.53.0

Re: [PATCH v2 4/4] serial: imx: serialize imx_uart_ports[] lifetime

[Greg Kroah-Hartman]

On Mon, Jul 20, 2026 at 12:10:14AM +0200, Karl Mehltretter wrote:
> imx_uart_probe() publishes the port in imx_uart_ports[] before
> uart_add_one_port(), because console setup during that call uses the
> table. The entry is not cleared if adding the port fails or after
> imx_uart_remove() removes it.
> 
> The port is devm-allocated, so a failed probe or unbind leaves the table
> pointing at freed memory. A later registration of the shared console can
> then dereference the stale entry. Reproduced on QEMU's mcimx6ul-evk by
> unbinding a sibling UART, unbinding the console UART and rebinding the
> sibling:
> 
>   BUG: KASAN: slab-use-after-free in imx_uart_console_setup+0xd0/0x3d8
> 
> The entry must remain valid until uart_remove_one_port() unregisters the
> console. Clearing it afterward without serialization still races a
> sibling probe: the sibling can register the shared console using the
> dying entry before it is cleared. The next console write then
> dereferences NULL.
> 
> Use a driver-wide mutex to serialize table publication, port addition
> and rollback with port removal and table clearing. Console callbacks
> remain lockless because uart_add_one_port() may invoke setup while
> probe holds the mutex.
> 
> The probe-failure path also relies on "serial: core: do fallible
> allocations before the console can be registered", which moves the
> uport->name and uport->tty_groups allocations before console
> registration. Both changes should be backported together.
> 
> Fixes: dbff4e9ea2e8 ("IMX UART: remove statically initialized tables")
> Fixes: 9f322ad064f9 ("imx: serial: handle initialisation failure correctly")
> Reported-by: Sashiko <sashiko-bot@kernel.org>
> Link: https://lore.kernel.org/all/20260719162850.043B41F000E9@smtp.kernel.org
> Cc: stable@vger.kernel.org
> Assisted-by: Claude:claude-fable-5
> Signed-off-by: Karl Mehltretter <kmehltretter@gmail.com>
> ---
>  drivers/tty/serial/imx.c | 21 +++++++++++++++++++--
>  1 file changed, 19 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c
> index 251a50c8aa38..def874f9cd00 100644
> --- a/drivers/tty/serial/imx.c
> +++ b/drivers/tty/serial/imx.c
> @@ -22,6 +22,7 @@
>  #include <linux/clk.h>
>  #include <linux/delay.h>
>  #include <linux/ktime.h>
> +#include <linux/mutex.h>
>  #include <linux/pinctrl/consumer.h>
>  #include <linux/rational.h>
>  #include <linux/slab.h>
> @@ -2080,6 +2081,15 @@ static const struct uart_ops imx_uart_pops = {
>  
>  static struct imx_port *imx_uart_ports[UART_NR];
>  
> +/*
> + * Store the port in imx_uart_ports[] before uart_add_one_port() and clear
> + * it only after uart_remove_one_port() returns. Console callbacks in both
> + * calls use the table, so this mutex serializes these sequences between
> + * sibling ports. Callbacks must not take it because uart_add_one_port()
> + * may invoke setup while it is held.
> + */
> +static DEFINE_MUTEX(imx_uart_ports_lock);

Again, LLMs love to write comments/text, be sane please.  This comment
has nothing to do with the storing and such.

> +
>  #if IS_ENABLED(CONFIG_SERIAL_IMX_CONSOLE)
>  static void imx_uart_console_putchar(struct uart_port *port, unsigned char ch)
>  {
> @@ -2632,11 +2642,14 @@ static int imx_uart_probe(struct platform_device *pdev)
>  		}
>  	}
>  
> -	imx_uart_ports[sport->port.line] = sport;
> -
>  	platform_set_drvdata(pdev, sport);
>  
> +	mutex_lock(&imx_uart_ports_lock);

guard()?

> +	imx_uart_ports[sport->port.line] = sport;
>  	ret = uart_add_one_port(&imx_uart_uart_driver, &sport->port);
> +	if (ret)
> +		imx_uart_ports[sport->port.line] = NULL;
> +	mutex_unlock(&imx_uart_ports_lock);
>  
>  err_clk:
>  	clk_disable_unprepare(sport->clk_ipg);
> @@ -2647,8 +2660,12 @@ static int imx_uart_probe(struct platform_device *pdev)
>  static void imx_uart_remove(struct platform_device *pdev)
>  {
>  	struct imx_port *sport = platform_get_drvdata(pdev);
> +	unsigned int line = sport->port.line;

Why are you reading this outside of the lock?

>  
> +	mutex_lock(&imx_uart_ports_lock);

guard()?

thanks,

greg k-h

Re: [PATCH v2 4/4] serial: imx: serialize imx_uart_ports[] lifetime

[Karl Mehltretter]

On Thu, Jul 30, 2026 at 04:31:51PM +0100, Greg Kroah-Hartman wrote:
> >  
> > +	mutex_lock(&imx_uart_ports_lock);
> 
> guard()?
> 

I'll use guards for v3.

> > +	unsigned int line = sport->port.line;
> 
> Why are you reading this outside of the lock?
> 

Good question. The local variable is unnecessary. I'll remove it.

Thanks,
Karl

Re: [PATCH v2 4/4] serial: imx: serialize imx_uart_ports[] lifetime

[Frank Li]

On Mon, Jul 20, 2026 at 12:10:14AM +0200, Karl Mehltretter wrote:
> [You don't often get email from kmehltretter@gmail.com. Learn why this is important at https://aka.ms/LearnAboutSenderIdentification ]
>
> imx_uart_probe() publishes the port in imx_uart_ports[] before
> uart_add_one_port(), because console setup during that call uses the
> table. The entry is not cleared if adding the port fails or after
> imx_uart_remove() removes it.
>
> The port is devm-allocated, so a failed probe or unbind leaves the table
> pointing at freed memory. A later registration of the shared console can
> then dereference the stale entry. Reproduced on QEMU's mcimx6ul-evk by
> unbinding a sibling UART, unbinding the console UART and rebinding the
> sibling:
>
>   BUG: KASAN: slab-use-after-free in imx_uart_console_setup+0xd0/0x3d8
>
> The entry must remain valid until uart_remove_one_port() unregisters the
> console. Clearing it afterward without serialization still races a
> sibling probe: the sibling can register the shared console using the
> dying entry before it is cleared. The next console write then
> dereferences NULL.
>
> Use a driver-wide mutex to serialize table publication, port addition
> and rollback with port removal and table clearing. Console callbacks
> remain lockless because uart_add_one_port() may invoke setup while
> probe holds the mutex.
>
> The probe-failure path also relies on "serial: core: do fallible
> allocations before the console can be registered", which moves the
> uport->name and uport->tty_groups allocations before console
> registration. Both changes should be backported together.
>
> Fixes: dbff4e9ea2e8 ("IMX UART: remove statically initialized tables")
> Fixes: 9f322ad064f9 ("imx: serial: handle initialisation failure correctly")
> Reported-by: Sashiko <sashiko-bot@kernel.org>
> Link: https://lore.kernel.org/all/20260719162850.043B41F000E9@smtp.kernel.org
> Cc: stable@vger.kernel.org
> Assisted-by: Claude:claude-fable-5
> Signed-off-by: Karl Mehltretter <kmehltretter@gmail.com>
> ---
>  drivers/tty/serial/imx.c | 21 +++++++++++++++++++--
>  1 file changed, 19 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c
> index 251a50c8aa38..def874f9cd00 100644
> --- a/drivers/tty/serial/imx.c
> +++ b/drivers/tty/serial/imx.c
> @@ -22,6 +22,7 @@
>  #include <linux/clk.h>
>  #include <linux/delay.h>
>  #include <linux/ktime.h>
> +#include <linux/mutex.h>
>  #include <linux/pinctrl/consumer.h>
>  #include <linux/rational.h>
>  #include <linux/slab.h>
> @@ -2080,6 +2081,15 @@ static const struct uart_ops imx_uart_pops = {
>
>  static struct imx_port *imx_uart_ports[UART_NR];
>
> +/*
> + * Store the port in imx_uart_ports[] before uart_add_one_port() and clear
> + * it only after uart_remove_one_port() returns. Console callbacks in both
> + * calls use the table, so this mutex serializes these sequences between
> + * sibling ports. Callbacks must not take it because uart_add_one_port()
> + * may invoke setup while it is held.
> + */
> +static DEFINE_MUTEX(imx_uart_ports_lock);
> +
>  #if IS_ENABLED(CONFIG_SERIAL_IMX_CONSOLE)
>  static void imx_uart_console_putchar(struct uart_port *port, unsigned char ch)
>  {
> @@ -2632,11 +2642,14 @@ static int imx_uart_probe(struct platform_device *pdev)
>                 }
>         }
>
> -       imx_uart_ports[sport->port.line] = sport;
> -
>         platform_set_drvdata(pdev, sport);
>
> +       mutex_lock(&imx_uart_ports_lock);
> +       imx_uart_ports[sport->port.line] = sport;
>         ret = uart_add_one_port(&imx_uart_uart_driver, &sport->port);
> +       if (ret)
> +               imx_uart_ports[sport->port.line] = NULL;
> +       mutex_unlock(&imx_uart_ports_lock);

ret = uart_add_one_port(&imx_uart_uart_driver, &sport->port);
if (ret)
	goto err_clk;

imx_uart_ports[sport->port.line] = sport;

return 0

and at imx_uart_remove()

imx_uart_remove()
{
	imx_uart_ports[line] = NULL;
	uart_remove_one_port(&imx_uart_uart_driver, &sport->port);
}

Is above sequence to avoid use mutex?

Frank

>
>  err_clk:
>         clk_disable_unprepare(sport->clk_ipg);
> @@ -2647,8 +2660,12 @@ static int imx_uart_probe(struct platform_device *pdev)
>  static void imx_uart_remove(struct platform_device *pdev)
>  {
>         struct imx_port *sport = platform_get_drvdata(pdev);
> +       unsigned int line = sport->port.line;
>
> +       mutex_lock(&imx_uart_ports_lock);
>         uart_remove_one_port(&imx_uart_uart_driver, &sport->port);
> +       imx_uart_ports[line] = NULL;
> +       mutex_unlock(&imx_uart_ports_lock);
>  }
>
>  static void imx_uart_restore_context(struct imx_port *sport)
> --
> 2.53.0
>

Re: [PATCH v2 4/4] serial: imx: serialize imx_uart_ports[] lifetime

[Karl Mehltretter]

On Thu, Jul 30, 2026 at 02:02:05PM +0100, Frank Li wrote:
> imx_uart_remove()
> {
> 	imx_uart_ports[line] = NULL;
> 	uart_remove_one_port(&imx_uart_uart_driver, &sport->port);
> }
> 
> Is above sequence to avoid use mutex?
> 

I now tried this, but it doesn't work as-is:

uart_add_one_port() may call imx_uart_console_setup(), so the entry
must be present before the call.

uart_remove_one_port() unregisters the console. Clearing the entry first
causes its callbacks to dereference NULL.

The mutex protects those sequences against sibling probes.

Thanks,
Karl

Re: [PATCH v2 4/4] serial: imx: serialize imx_uart_ports[] lifetime

[Frank Li]

On Fri, Jul 31, 2026 at 04:40:21PM +0200, Karl Mehltretter wrote:
> On Thu, Jul 30, 2026 at 02:02:05PM +0100, Frank Li wrote:
> > imx_uart_remove()
> > {
> > 	imx_uart_ports[line] = NULL;
> > 	uart_remove_one_port(&imx_uart_uart_driver, &sport->port);
> > }
> >
> > Is above sequence to avoid use mutex?
> >
>
> I now tried this, but it doesn't work as-is:
>
> uart_add_one_port() may call imx_uart_console_setup(), so the entry
> must be present before the call.
>
> uart_remove_one_port() unregisters the console. Clearing the entry first
> causes its callbacks to dereference NULL.
>
> The mutex protects those sequences against sibling probes.

Okay

Frank
>
> Thanks,
> Karl