[PATCH 0/2 5.10] serial: 8250_dw: fix port leak on clock notifier failure

Denis Arefev posted 2 patches 2 weeks, 3 days ago
drivers/tty/serial/8250/8250_dw.c | 32 ++++++++++++++-----------------
1 file changed, 14 insertions(+), 18 deletions(-)
[PATCH 0/2 5.10] serial: 8250_dw: fix port leak on clock notifier failure
Posted by Denis Arefev 2 weeks, 3 days ago
This series cleans up the error handling in dw8250_probe() and fixes a
port leak. When clk_notifier_register() fails, probe() returns an error
but the 8250 port registered just before it stays registered: the
matching serial8250_unregister_port() lives in dw8250_remove(), which is
not called on probe failure. The port slot then stays occupied until a
rebind, and the devm-allocated driver data is freed while the port still
references it (via private_data and the serial_in/serial_out callbacks),
a use-after-free hazard.

The first patch converts the probe() error paths to dev_err_probe().
The second patch unregisters the 8250 port on the
clk_notifier_register() error path.

Andy Shevchenko (1):
  serial: 8250_dw: Use dev_err_probe()

Stepan Ionichev (1):
  serial: 8250_dw: unregister 8250 port if clk_notifier_register() fails

 drivers/tty/serial/8250/8250_dw.c | 32 ++++++++++++++-----------------
 1 file changed, 14 insertions(+), 18 deletions(-)
-- 
2.43.0
Re: [PATCH 0/2 5.10] serial: 8250_dw: fix port leak on clock notifier failure
Posted by Sasha Levin 2 weeks, 2 days ago
> This series cleans up the error handling in dw8250_probe() and fixes a
> port leak. When clk_notifier_register() fails, probe() returns an error
> but the 8250 port registered just before it stays registered: the
> matching serial8250_unregister_port() lives in dw8250_remove(), which is
> not called on probe failure.

Queued for 5.10, thanks.

-- 
Thanks,
Sasha
Re: [PATCH 0/2 5.10] serial: 8250_dw: fix port leak on clock notifier failure
Posted by Sasha Levin 2 weeks, 2 days ago
> Queued for 5.10, thanks.

I'm dropping these again...

The bug being fixed isn't reachable on 5.10. dw8250_probe() there ends the
clock notifier block with:

	if (data->clk) {
		err = clk_notifier_register(data->clk, &data->clk_notifier);
		if (err)
			dev_warn(p->dev, "Failed to set the clock notifier\n");
		else
			queue_work(system_unbound_wq, &data->clk_work);
	}

	platform_set_drvdata(pdev, data);

so a notifier registration failure warns and probe still returns 0. There is no
error return anywhere after serial8250_register_8250_port(), which is the
precondition 10fc708b4de7 ("serial: 8250_dw: unregister 8250 port if
clk_notifier_register() fails") describes when it says probe "returns the error
but leaves the 8250 port registered". No port is left behind on 5.10, so
there's nothing to unregister and no use after free.

What creates that path is patch 1/2, 57f83e5dd6a3 ("serial: 8250_dw: Use
dev_err_probe()"), which turns the warn into a return. That's a cleanup from
May 2022 with no Fixes tag and no stable Cc. It's in the base of 6.1 and newer,
which is why the fix is correct there, but 5.10 and 5.15 predate it and never
had the failing path. Taking the pair here would import the bug and its fix
together, and would also make a notifier registration failure fatal to probe on
a tree where it currently isn't, for hardware that works today.

Same reasoning applies to 5.15, so please don't send it there either.

-- 
Thanks,
Sasha