[PATCH] serial: 8250-dw: unregister 8250 port if clk_notifier_register() fails

[PATCH] serial: 8250-dw: unregister 8250 port if clk_notifier_register() fails

[Stepan Ionichev]

dw8250_probe() registers the 8250 port via serial8250_register_8250_port()
and then, if the device has a clock, registers a clock notifier:

	data->data.line = serial8250_register_8250_port(up);
	if (data->data.line < 0)
		return data->data.line;
	...
	if (data->clk) {
		err = clk_notifier_register(data->clk, &data->clk_notifier);
		if (err)
			return dev_err_probe(dev, err,
					     "Failed to set the clock notifier\n");
		...
	}

If clk_notifier_register() fails, probe returns the error but leaves
the 8250 port registered. The matching serial8250_unregister_port()
lives in dw8250_remove(), which is not called when probe fails, so
the port slot in the serial8250 array stays occupied until the
device is rebound or the system is rebooted. The devm-allocated
driver data is freed while the port still references it (via the
saved private_data and serial_in/serial_out callbacks), so any
access to that port slot before a rebind would be a use-after-free.

Unregister the port on the clk_notifier_register() error path.

Signed-off-by: Stepan Ionichev <sozdayvek@gmail.com>
---
 drivers/tty/serial/8250/8250_dw.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/tty/serial/8250/8250_dw.c b/drivers/tty/serial/8250/8250_dw.c
index 94beadb40..7dbd79a91 100644
--- a/drivers/tty/serial/8250/8250_dw.c
+++ b/drivers/tty/serial/8250/8250_dw.c
@@ -850,8 +850,10 @@ static int dw8250_probe(struct platform_device *pdev)
 	 */
 	if (data->clk) {
 		err = clk_notifier_register(data->clk, &data->clk_notifier);
-		if (err)
+		if (err) {
+			serial8250_unregister_port(data->data.line);
 			return dev_err_probe(dev, err, "Failed to set the clock notifier\n");
+		}
 		queue_work(system_dfl_wq, &data->clk_work);
 	}
 
-- 
2.43.0

Re: [PATCH] serial: 8250-dw: unregister 8250 port if clk_notifier_register() fails

[Andy Shevchenko]

On Wed, May 13, 2026 at 08:05:03PM +0500, Stepan Ionichev wrote:
> dw8250_probe() registers the 8250 port via serial8250_register_8250_port()
> and then, if the device has a clock, registers a clock notifier:
> 
> 	data->data.line = serial8250_register_8250_port(up);
> 	if (data->data.line < 0)
> 		return data->data.line;
> 	...
> 	if (data->clk) {
> 		err = clk_notifier_register(data->clk, &data->clk_notifier);
> 		if (err)
> 			return dev_err_probe(dev, err,
> 					     "Failed to set the clock notifier\n");
> 		...
> 	}
> 
> If clk_notifier_register() fails, probe returns the error but leaves
> the 8250 port registered. The matching serial8250_unregister_port()
> lives in dw8250_remove(), which is not called when probe fails, so
> the port slot in the serial8250 array stays occupied until the
> device is rebound or the system is rebooted. The devm-allocated
> driver data is freed while the port still references it (via the
> saved private_data and serial_in/serial_out callbacks), so any
> access to that port slot before a rebind would be a use-after-free.
> 
> Unregister the port on the clk_notifier_register() error path.

Maybe as a fix for backporting. For the current cycle can you remove that
notifier and all that crap that was brought by Baikal upstreaming which won't
ever be finished (as we actually dropped Baikal code in the kernel)?

Or maybe series of two: this one as backport and the other one as fix / remove
Baikal support entirely from this driver.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH] serial: 8250-dw: unregister 8250 port if clk_notifier_register() fails

[Andy Shevchenko]

On Wed, May 13, 2026 at 11:34:58PM +0300, Andy Shevchenko wrote:
> On Wed, May 13, 2026 at 08:05:03PM +0500, Stepan Ionichev wrote:

Also note, the Subject prefix should be "serial: 8250_dw: ...".

> > dw8250_probe() registers the 8250 port via serial8250_register_8250_port()
> > and then, if the device has a clock, registers a clock notifier:
> > 
> > 	data->data.line = serial8250_register_8250_port(up);
> > 	if (data->data.line < 0)
> > 		return data->data.line;
> > 	...
> > 	if (data->clk) {
> > 		err = clk_notifier_register(data->clk, &data->clk_notifier);
> > 		if (err)
> > 			return dev_err_probe(dev, err,
> > 					     "Failed to set the clock notifier\n");
> > 		...
> > 	}
> > 
> > If clk_notifier_register() fails, probe returns the error but leaves
> > the 8250 port registered. The matching serial8250_unregister_port()
> > lives in dw8250_remove(), which is not called when probe fails, so
> > the port slot in the serial8250 array stays occupied until the
> > device is rebound or the system is rebooted. The devm-allocated
> > driver data is freed while the port still references it (via the
> > saved private_data and serial_in/serial_out callbacks), so any
> > access to that port slot before a rebind would be a use-after-free.
> > 
> > Unregister the port on the clk_notifier_register() error path.
> 
> Maybe as a fix for backporting. For the current cycle can you remove that
> notifier and all that crap that was brought by Baikal upstreaming which won't
> ever be finished (as we actually dropped Baikal code in the kernel)?
> 
> Or maybe series of two: this one as backport and the other one as fix / remove
> Baikal support entirely from this driver.

-- 
With Best Regards,
Andy Shevchenko