[PATCH] tty: serial: max3100: drain async producers in remove() to fix timer UAF

[Fan Wu]

All teardown of the driver's async producers (polling timer, SPI
workqueue, IRQ) lives in the max3100_shutdown() uart_ops callback.
On the suspend-then-remove path, max3100_shutdown() returns without
draining these producers, so max3100_remove() can call kfree() while
the polling timer remains armed.

max3100_startup() arms the polling timer via max3100_enable_ms().  The
timer callback max3100_timeout() unconditionally re-arms itself with
mod_timer(), and on each tick dereferences the owning struct
max3100_port (recovered via container_of(), then uart_poll_timeout on
s->port).

max3100_shutdown() opens with "if (s->suspending) return;", which
short-circuits the whole drain block.  max3100_suspend() sets
s->suspending before calling uart_suspend_port().
uart_suspend_port() (serial_core.c) clears tty_port_initialized() and
then invokes ops->shutdown = max3100_shutdown, which takes the
s->suspending early return above and runs none of the drain.  The
timer stays armed.

On a later SPI unbind, max3100_remove() calls uart_remove_one_port(),
which (serial_core_remove_one_port) invokes tty_port_tty_vhangup() but
does not invoke ops->shutdown() directly.  ops->shutdown() is reachable
only through tty_port_shutdown() and uart_port_shutdown(), but
tty_port_shutdown() is gated on tty_port_initialized(), which suspend
clears.  The removal path therefore does not invoke max3100_shutdown(),
and max3100_remove() proceeds directly to kfree(max3100s[i]) with the
timer still armed.

A port that was opened before system suspend can retain its polling
timer.  If the SPI device is then unbound before resume, max3100_remove()
frees the port while the timer can still run and re-arm itself.

Factor a drain helper with a final-teardown argument.  Both variants
stop the timer, IRQ, and workqueue in that order.  Normal shutdown uses
timer_delete_sync(), which preserves the timer for a later open.  Final
remove uses timer_shutdown_sync(), because max3100_timeout() re-arms via
mod_timer() and the object is about to be freed; shutdown makes a racing
re-arm a no-op.

Track whether request_irq() succeeded separately from port->irq.  The
latter is the hardware resource number and must survive a normal close
for the next startup; irq_requested makes the drain idempotent without
changing it.

Call the non-final form from max3100_shutdown() and the final form
after uart_remove_one_port() in max3100_remove().  The s->suspending
early return in max3100_shutdown() remains intact, while remove()
drains unconditionally.

timer_shutdown_sync() is available since v6.2.

This issue was found by an in-house static analysis tool.

Fixes: 7831d56b0a35 ("tty: MAX3100")
Cc: stable@xxxxxxxxxxxxxxx # 6.2+
Assisted-by: Codex:gpt-5.6
Signed-off-by: Fan Wu <fanwu01@xxxxxxxxxx>
---
 drivers/tty/serial/max3100.c | 37 ++++++++++++++++++++++++++----------
 1 file changed, 27 insertions(+), 10 deletions(-)

diff --git a/drivers/tty/serial/max3100.c b/drivers/tty/serial/max3100.c
index 44b745fa26c6..9dac733a9b81 100644
--- a/drivers/tty/serial/max3100.c
+++ b/drivers/tty/serial/max3100.c
@@ -107,6 +107,7 @@ struct max3100_port {
 	int  force_end_work;
 	/* need to know we are suspending to avoid deadlock on workqueue */
 	int suspending;
+	bool irq_requested;
 
 	struct timer_list	timer;
 };
@@ -306,6 +307,29 @@ static void max3100_dowork(struct max3100_port *s)
 		queue_work(s->workqueue, &s->work);
 }
 
+/*
+ * Stop async producers before tearing down the workqueue.  A normal
+ * shutdown must leave the timer re-armable for the next open, while final
+ * removal uses timer_shutdown_sync() to prevent max3100_timeout() from
+ * re-arming a timer embedded in an object about to be freed.
+ */
+static void max3100_drain_async(struct max3100_port *s, bool final)
+{
+	s->force_end_work = 1;
+	if (final)
+		timer_shutdown_sync(&s->timer);
+	else
+		timer_delete_sync(&s->timer);
+	if (s->irq_requested) {
+		free_irq(s->port.irq, s);
+		s->irq_requested = false;
+	}
+	if (s->workqueue) {
+		destroy_workqueue(s->workqueue);
+		s->workqueue = NULL;
+	}
+}
+
 static void max3100_timeout(struct timer_list *t)
 {
 	struct max3100_port *s = timer_container_of(s, t, timer);
@@ -530,16 +554,7 @@ static void max3100_shutdown(struct uart_port *port)
 	if (s->suspending)
 		return;
 
-	s->force_end_work = 1;
-
-	timer_delete_sync(&s->timer);
-
-	if (s->workqueue) {
-		destroy_workqueue(s->workqueue);
-		s->workqueue = NULL;
-	}
-	if (port->irq)
-		free_irq(port->irq, s);
+	max3100_drain_async(s, false);
 
 	/* set shutdown mode to save power */
 	max3100_sr(s, MAX3100_WC | MAX3100_SHDN, &rx);
@@ -581,6 +596,7 @@ static int max3100_startup(struct uart_port *port)
 		return -EBUSY;
 	}
 
+	s->irq_requested = true;
 	s->conf_commit = 1;
 	max3100_dowork(s);
 	/* wait for clock to settle */
@@ -752,6 +768,7 @@ static void max3100_remove(struct spi_device *spi)
 		if (max3100s[i] == s) {
 			dev_dbg(&spi->dev, "%s: removing port %d\n", __func__, i);
 			uart_remove_one_port(&max3100_uart_driver, &max3100s[i]->port);
+			max3100_drain_async(max3100s[i], true);
 			kfree(max3100s[i]);
 			max3100s[i] = NULL;
 			break;
-- 
2.34.1