[PATCH] serial: core: fix NULL pointer dereference in serial_core_unregister_port()

[PATCH] serial: core: fix NULL pointer dereference in serial_core_unregister_port()

[Ruslan Valiyev]

serial_core_unregister_port() dereferences port->port_dev before it has
been checked:

	struct serial_port_device *port_dev = port->port_dev;
	struct serial_ctrl_device *ctrl_dev = serial_core_get_ctrl_dev(port_dev);

serial_core_get_ctrl_dev() takes &port_dev->dev and reads dev->parent
straight away, so a NULL port_dev faults at offset 0x40.

port_dev is NULL whenever no port device is installed:
serial_core_remove_one_port() clears it on teardown, and it is never
set if registration failed before serial_core_port_device_add().

serial8250_unregister_port() reaches that state. It calls
uart_remove_one_port(), which clears port_dev, and then re-adds the
port with uart_add_one_port() without checking the return value. When
that re-add fails, port_dev stays NULL while port.dev still points at
the ISA platform device, so unbinding that device once more calls
serial8250_unregister_port() again and oopses:

  Oops: general protection fault, probably for non-canonical address
  KASAN: null-ptr-deref in range [0x0000000000000040-0x0000000000000047]
  RIP: 0010:serial_core_unregister_port+0xef/0x990
  Call Trace:
   serial8250_unregister_port+0x1e4/0x8a0
   serial8250_remove+0x8c/0xb0
   platform_remove+0x5f/0x80
   device_release_driver_internal+0x46b/0x640
   unbind_store+0xf8/0x110
   sysfs_kf_write+0xf2/0x150
   vfs_write+0x6ac/0x1050

Return early when there is no port device to remove, and read
port->port_dev under port_mutex, since every other update of that
field is serialised by it.

Also clear port->port_dev on the serial_core_register_port() error
path. serial_base_port_device_remove() frees the port device but left
the pointer behind, so unregistering after a failed registration read
freed memory instead. That is the use-after-free variant of the same
crash, and matches the title syzbot first reported this under.

Fixes: 84a9582fd203 ("serial: core: Start managing serial controllers to enable runtime PM")
Reported-by: syzbot+9f57c1b2792029198fcf@syzkaller.appspotmail.com
Closes: https://syzkaller.appspot.com/bug?extid=9f57c1b2792029198fcf
Cc: stable@vger.kernel.org
Signed-off-by: Ruslan Valiyev <linuxoid@gmail.com>
---
Reproduced and verified on 8d3ae59288f1 (Linux 7.2) with syzbot's config,
under QEMU/KVM x86_64. Over six runs of the reproducer:

  stock:   6/6 oops at serial_core_unregister_port+0xef, with the same
           Code: bytes and RDI=0x40 as the syzbot report
  patched: 0/6 oops at serial_core_unregister_port

checkpatch.pl clean, W=1 build of serial_core.o produces no new warnings,
and the patch applies cleanly to current mainline.

Please note the reproducer does not run to completion on a patched kernel.
It goes on to hit two further problems. Both look pre-existing and neither
is addressed here; I am describing them so the remaining crashes are not
mistaken for this patch failing.

1) tty_cdev_add() drops the last reference to the cdev when cdev_add()
   fails, but leaves driver->cdevs[index] pointing at it, and
   tty_unregister_device() then calls cdev_del() on the freed object:

     WARNING: lib/refcount.c:28 at refcount_warn_saturate
     Call Trace:
      kobject_put+0x26f/0x6f0
      tty_unregister_device+0x118/0x1c0
      tty_port_unregister_device+0x60/0x70
      serial_core_unregister_port+0x333/0x9a0

   tty_unregister_device() also calls cdev_del(driver->cdevs[index])
   unconditionally, and that entry is NULL when registration failed
   before tty_cdev_add() ran:

     KASAN: null-ptr-deref in range [0x60-0x67]
     RIP: 0010:cdev_del+0x26/0xa0

   serial_core_add_one_port() reaches both: it treats a failed tty
   registration as non-fatal, flagging the port dead and returning
   success, so the port is still unregistered later.

2) Registration is not failure-atomic. serial_core_add_one_port() links
   state->uart_port before the kasprintf() and tty_groups allocations, so
   a failure there leaves the port half registered. The state is never
   released, and because serial_core_add_one_port() starts with

           if (state->uart_port)
                   return -EINVAL;

   that line can then never be registered again. Unwinding it properly
   means undoing uart_configure_port(), which claims resources and can
   register a console, so it did not look like something to bolt onto a
   crash fix.

While here I also noticed serial8250_unregister_port() ignores the return
value of the uart_add_one_port() call that re-adds the port to the ISA
device, which is what produces the NULL port_dev this patch guards
against.
 drivers/tty/serial/serial_core.c | 17 +++++++++++++++--
 1 file changed, 15 insertions(+), 2 deletions(-)

diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c
index a530ad372b434..5bf71d7bbd223 100644
--- a/drivers/tty/serial/serial_core.c
+++ b/drivers/tty/serial/serial_core.c
@@ -3327,6 +3327,7 @@ int serial_core_register_port(struct uart_driver *drv, struct uart_port *port)
 
 err_unregister_port_dev:
 	serial_base_port_device_remove(port->port_dev);
+	port->port_dev = NULL;
 
 err_unregister_ctrl_dev:
 	serial_base_ctrl_device_remove(new_ctrl_dev);
@@ -3341,12 +3342,24 @@ int serial_core_register_port(struct uart_driver *drv, struct uart_port *port)
 void serial_core_unregister_port(struct uart_driver *drv, struct uart_port *port)
 {
 	struct device *phys_dev = port->dev;
-	struct serial_port_device *port_dev = port->port_dev;
-	struct serial_ctrl_device *ctrl_dev = serial_core_get_ctrl_dev(port_dev);
+	struct serial_port_device *port_dev;
+	struct serial_ctrl_device *ctrl_dev;
 	int ctrl_id = port->ctrl_id;
 
 	guard(mutex)(&port_mutex);
 
+	/*
+	 * A NULL port device means there is no registered port device to
+	 * remove: serial_core_remove_one_port() clears port_dev on
+	 * teardown, and it is never set if registration failed before
+	 * serial_core_port_device_add().
+	 */
+	port_dev = port->port_dev;
+	if (!port_dev)
+		return;
+
+	ctrl_dev = serial_core_get_ctrl_dev(port_dev);
+
 	port->flags |= UPF_DEAD;
 
 	serial_core_remove_one_port(drv, port);

base-commit: 8d3ae59288f1e7d58d76558a6ee96d533bc5019f
-- 
2.43.0

Re: [PATCH] serial: core: fix NULL pointer dereference in serial_core_unregister_port()

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:

Pre-existing issues:
- [High] `tty_unregister_device()` unconditionally deregisters a `cdev`, causing UAF or NULL pointer dereference if `tty_cdev_add()` previously failed.
- [High] `uart_resume_port()` dereferences a NULL `tty_dev` in `device_may_wakeup()` if port registration failed and the driver kept the port active.
- [High] Concurrent read-modify-write of `port->flags` without proper locking causes `UPF_DEAD` to be lost, undermining port teardown safety.
--

commit a9b5deb58407eb5cb3253b4b3646215bfcb86ab0
Author: Ruslan Valiyev <linuxoid@gmail.com>

serial: core: fix NULL pointer dereference in serial_core_unregister_port()

This commit fixes a NULL pointer dereference in serial_core_unregister_port()
that occurs when unregistering a port with no port device installed. It adds
an early return if port_dev is NULL and explicitly clears port->port_dev
on the error path of serial_core_register_port().

> diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c
> index a530ad372b434..5bf71d7bbd223 100644
> --- a/drivers/tty/serial/serial_core.c
> +++ b/drivers/tty/serial/serial_core.c
> @@ -3327,6 +3327,7 @@ int serial_core_register_port(struct uart_driver *drv, struct uart_port *port)
>  
>  err_unregister_port_dev:
>  	serial_base_port_device_remove(port->port_dev);
> +	port->port_dev = NULL;

[Severity: High]
This is a pre-existing issue, but does uart_resume_port() dereference a
NULL tty_dev in device_may_wakeup() if port registration failed and the
driver kept the port active?

If serial_core_register_port() fails, port_dev is set to NULL. If this port
is configured as a console, during system suspend uart_suspend_port() will
return early (leaving uport->suspended = 0).

On system resume, uart_resume_port() calls device_find_child(NULL, ...):

drivers/tty/serial/serial_core.c:uart_resume_port() {
	tty_dev = device_find_child(&uport->port_dev->dev, &match, serial_match_port);
	if (!uport->suspended && device_may_wakeup(tty_dev)) {
		...
	}
}

It safely returns NULL for tty_dev. Since uport->suspended is 0, the
short-circuit && does not prevent device_may_wakeup(tty_dev) from being
called.

device_may_wakeup(NULL) unconditionally dereferences NULL->power.can_wakeup:

include/linux/pm_wakeup.h:device_may_wakeup() {
	return dev->power.can_wakeup && !!dev->power.wakeup;
}

>  
>  err_unregister_ctrl_dev:
>  	serial_base_ctrl_device_remove(new_ctrl_dev);

[ ... ]

> @@ -3341,12 +3342,24 @@ int serial_core_register_port(struct uart_driver *drv, struct uart_port *port)
>  void serial_core_unregister_port(struct uart_driver *drv, struct uart_port *port)
>  {
>  	struct device *phys_dev = port->dev;
> -	struct serial_port_device *port_dev = port->port_dev;
> -	struct serial_ctrl_device *ctrl_dev = serial_core_get_ctrl_dev(port_dev);
> +	struct serial_port_device *port_dev;
> +	struct serial_ctrl_device *ctrl_dev;
>  	int ctrl_id = port->ctrl_id;
>  
>  	guard(mutex)(&port_mutex);
>  
> +	/*
> +	 * A NULL port device means there is no registered port device to
> +	 * remove: serial_core_remove_one_port() clears port_dev on
> +	 * teardown, and it is never set if registration failed before
> +	 * serial_core_port_device_add().
> +	 */
> +	port_dev = port->port_dev;
> +	if (!port_dev)
> +		return;
> +
> +	ctrl_dev = serial_core_get_ctrl_dev(port_dev);
> +
>  	port->flags |= UPF_DEAD;

[Severity: High]
This is a pre-existing issue, but can a concurrent read-modify-write of
port->flags without proper locking cause UPF_DEAD to be lost, undermining
port teardown safety?

serial_core_unregister_port() sets the UPF_DEAD bit in port->flags while
holding the global port_mutex, but without holding the TTY &port->mutex.

Concurrently, uart_set_info() modifies port->flags using a read-modify-write
operation while holding the TTY &port->mutex:

drivers/tty/serial/serial_core.c:uart_set_info() {
	uport->flags = (uport->flags & ~UPF_CHANGE_MASK) |
				 (new_flags & UPF_CHANGE_MASK);
}

If the threads interleave, uart_set_info() can overwrite and clear the
UPF_DEAD bit set by the teardown path. If UPF_DEAD is lost,
uart_port_activate() will incorrectly allow an unbound/removed port to be
opened and activated.

>  
>  	serial_core_remove_one_port(drv, port);

[Severity: High]
This is a pre-existing issue, but does tty_unregister_device() unconditionally
deregister a cdev, causing UAF or NULL pointer dereference if tty_cdev_add()
previously failed?

If cdev_alloc() or cdev_add() fails in tty_cdev_add(), it leaves
driver->cdevs[index] pointing to NULL or freed memory:

drivers/tty/tty_io.c:tty_cdev_add() {
	err = cdev_add(driver->cdevs[index], dev, count);
	if (err)
		kobject_put(&driver->cdevs[index]->kobj);
	return err;
}

Because serial_core_add_one_port() ignores this error, treating TTY
registration failure as non-fatal and returning success, the port gets
registered:

drivers/tty/serial/serial_core.c:serial_core_add_one_port() {
	if (!IS_ERR(tty_dev)) {
		device_set_wakeup_capable(tty_dev, 1);
	} else {
		uport->flags |= UPF_DEAD;
		dev_err(uport->dev, "Cannot register tty device on line %u\n",
		       uport->line);
	}
	return 0;
}

On teardown, serial_core_remove_one_port() calls
tty_port_unregister_device() which invokes tty_unregister_device().
tty_unregister_device() calls cdev_del(driver->cdevs[index]) unconditionally
since serial_core does not use the TTY_DRIVER_DYNAMIC_ALLOC flag:

drivers/tty/tty_io.c:tty_unregister_device() {
	if (!(driver->flags & TTY_DRIVER_DYNAMIC_ALLOC)) {
		cdev_del(driver->cdevs[index]);
		driver->cdevs[index] = NULL;
	}
}

cdev_del() does not check for NULL and dereferences the pointer, causing a
direct kernel panic.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260826073237.1377668-1-linuxoid@gmail.com?part=1

Re: [PATCH] serial: core: fix NULL pointer dereference in serial_core_unregister_port()

[Greg Kroah-Hartman]

On Wed, Aug 26, 2026 at 09:32:36AM +0200, Ruslan Valiyev wrote:
> serial_core_unregister_port() dereferences port->port_dev before it has
> been checked:
> 
> 	struct serial_port_device *port_dev = port->port_dev;
> 	struct serial_ctrl_device *ctrl_dev = serial_core_get_ctrl_dev(port_dev);
> 
> serial_core_get_ctrl_dev() takes &port_dev->dev and reads dev->parent
> straight away, so a NULL port_dev faults at offset 0x40.
> 
> port_dev is NULL whenever no port device is installed:
> serial_core_remove_one_port() clears it on teardown, and it is never
> set if registration failed before serial_core_port_device_add().
> 
> serial8250_unregister_port() reaches that state. It calls
> uart_remove_one_port(), which clears port_dev, and then re-adds the
> port with uart_add_one_port() without checking the return value. When
> that re-add fails, port_dev stays NULL while port.dev still points at
> the ISA platform device, so unbinding that device once more calls
> serial8250_unregister_port() again and oopses:
> 
>   Oops: general protection fault, probably for non-canonical address
>   KASAN: null-ptr-deref in range [0x0000000000000040-0x0000000000000047]
>   RIP: 0010:serial_core_unregister_port+0xef/0x990
>   Call Trace:
>    serial8250_unregister_port+0x1e4/0x8a0
>    serial8250_remove+0x8c/0xb0
>    platform_remove+0x5f/0x80
>    device_release_driver_internal+0x46b/0x640
>    unbind_store+0xf8/0x110
>    sysfs_kf_write+0xf2/0x150
>    vfs_write+0x6ac/0x1050
> 
> Return early when there is no port device to remove, and read
> port->port_dev under port_mutex, since every other update of that
> field is serialised by it.
> 
> Also clear port->port_dev on the serial_core_register_port() error
> path. serial_base_port_device_remove() frees the port device but left
> the pointer behind, so unregistering after a failed registration read
> freed memory instead. That is the use-after-free variant of the same
> crash, and matches the title syzbot first reported this under.

It's an invalid syzbot reproducer, if root tells the kernel to unbind
from a device when it is being used, it gets to keep the pieces when
things break :(

Let me go polish off my "taint the kernel if bind/unbind runs" patch to
keep this from happening...

> Fixes: 84a9582fd203 ("serial: core: Start managing serial controllers to enable runtime PM")
> Reported-by: syzbot+9f57c1b2792029198fcf@syzkaller.appspotmail.com
> Closes: https://syzkaller.appspot.com/bug?extid=9f57c1b2792029198fcf
> Cc: stable@vger.kernel.org
> Signed-off-by: Ruslan Valiyev <linuxoid@gmail.com>

Did you forget an Assisted-by: tag?

thanks,

greg k-h

Re: [PATCH] serial: core: fix NULL pointer dereference in serial_core_unregister_port()

[Ruslan Valiyev]

On Wed, Aug 26, 2026 at 09:53:34AM +0200, Greg Kroah-Hartman wrote:
> It's an invalid syzbot reproducer, if root tells the kernel to unbind
> from a device when it is being used, it gets to keep the pieces when
> things break :(
>
> Let me go polish off my "taint the kernel if bind/unbind runs" patch to
> keep this from happening...

Understood, thanks for looking at it so quickly. I'll drop the patch.

> Did you forget an Assisted-by: tag?

Yes. The patch was AI-assisted and should have included

  Assisted-by: Claude:claude-opus-5
  Assisted-by: Codex:gpt-5.6-sol

I worked from submitting-patches.rst for the tag order and did not read
coding-assistants.rst. My mistake, and it will be on anything I send in future.

Thanks,
Ruslan

Re: [PATCH] serial: core: fix NULL pointer dereference in serial_core_unregister_port()

[Andy Shevchenko]

On Wed, Aug 26, 2026 at 09:32:36AM +0200, Ruslan Valiyev wrote:
> serial_core_unregister_port() dereferences port->port_dev before it has
> been checked:
> 
> 	struct serial_port_device *port_dev = port->port_dev;
> 	struct serial_ctrl_device *ctrl_dev = serial_core_get_ctrl_dev(port_dev);
> 
> serial_core_get_ctrl_dev() takes &port_dev->dev and reads dev->parent
> straight away, so a NULL port_dev faults at offset 0x40.
> 
> port_dev is NULL whenever no port device is installed:
> serial_core_remove_one_port() clears it on teardown, and it is never
> set if registration failed before serial_core_port_device_add().
> 
> serial8250_unregister_port() reaches that state. It calls
> uart_remove_one_port(), which clears port_dev, and then re-adds the
> port with uart_add_one_port() without checking the return value. When
> that re-add fails, port_dev stays NULL while port.dev still points at
> the ISA platform device, so unbinding that device once more calls
> serial8250_unregister_port() again and oopses:
> 
>   Oops: general protection fault, probably for non-canonical address
>   KASAN: null-ptr-deref in range [0x0000000000000040-0x0000000000000047]
>   RIP: 0010:serial_core_unregister_port+0xef/0x990
>   Call Trace:
>    serial8250_unregister_port+0x1e4/0x8a0
>    serial8250_remove+0x8c/0xb0
>    platform_remove+0x5f/0x80
>    device_release_driver_internal+0x46b/0x640
>    unbind_store+0xf8/0x110

>    sysfs_kf_write+0xf2/0x150
>    vfs_write+0x6ac/0x1050

At least these two lines are noise in the backtrace in the commit message.
Submitting Patches recommends to leave only significantly important lines.

> Return early when there is no port device to remove, and read
> port->port_dev under port_mutex, since every other update of that
> field is serialised by it.
> 
> Also clear port->port_dev on the serial_core_register_port() error
> path. serial_base_port_device_remove() frees the port device but left
> the pointer behind, so unregistering after a failed registration read
> freed memory instead. That is the use-after-free variant of the same
> crash, and matches the title syzbot first reported this under.

Overall, try to re-read and simplify the text. This looks like an AI puke.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH] serial: core: fix NULL pointer dereference in serial_core_unregister_port()

[Greg Kroah-Hartman]

On Wed, Aug 26, 2026 at 10:06:26AM +0200, Ruslan Valiyev wrote:
> On Wed, Aug 26, 2026 at 09:53:34AM +0200, Greg Kroah-Hartman wrote:
> > It's an invalid syzbot reproducer, if root tells the kernel to unbind
> > from a device when it is being used, it gets to keep the pieces when
> > things break :(
> >
> > Let me go polish off my "taint the kernel if bind/unbind runs" patch to
> > keep this from happening...
> 
> Understood, thanks for looking at it so quickly. I'll drop the patch.

It's not an invalid change, just fix it up to be sane and we can take
it.

> > Did you forget an Assisted-by: tag?
> 
> Yes. The patch was AI-assisted and should have included
> 
>   Assisted-by: Claude:claude-opus-5
>   Assisted-by: Codex:gpt-5.6-sol
> 
> I worked from submitting-patches.rst for the tag order and did not read
> coding-assistants.rst. My mistake, and it will be on anything I send in future.

Your LLM should have read that file and added it automatically.  If not,
you're "holding it wrong" when using those tools as there's lots of text
in there that it should be reading.

thanks,

greg k-h