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

[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