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

[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@vger.kernel.org # 6.2+
Assisted-by: Codex:gpt-5.6
Signed-off-by: Fan Wu <fanwu01@zju.edu.cn>
---
 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

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

[Greg KH]

On Tue, Jul 21, 2026 at 03:56:31AM +0000, Fan Wu wrote:
> 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.

Yes, but that's not needed here, right?

LLMs love to write text, please don't let it and write your own
changelog that actually makes sense :)

> This issue was found by an in-house static analysis tool.
> 
> Fixes: 7831d56b0a35 ("tty: MAX3100")
> Cc: stable@vger.kernel.org # 6.2+
> Assisted-by: Codex:gpt-5.6
> Signed-off-by: Fan Wu <fanwu01@zju.edu.cn>
> ---
>  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)

Having a bool here makes no sense at all, and it's a horrible api as now
you have to look up the documentation each time you run across it.

Also, you didn't even document it (well, the LLM didn't...)

Please be more careful.

> +{
> +	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;

It's not "requested" it's "is an interrupt registered or not", right?

thanks,

greg k-h