[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

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

[Fan Wu]

Hi Greg,

Thanks for the review. You're right. I'll respin with a concise changelog,
remove the boolean helper, and rename the state to irq_registered.

I'll send v2 shortly.

Thanks,
Fan

> On Jul 30, 2026, at 22:39, Greg KH <gregkh@linuxfoundation.org> wrote:
> 
> 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 :)
> 
>> 
>> +/*
>> + * 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

[PATCH v2] tty: serial: max3100: shut down timer before freeing port

[Fan Wu]

max3100_shutdown() stops the polling timer but returns early during
system suspend. If the SPI device is unbound before resume, the serial
core does not call max3100_shutdown() again, so max3100_remove() frees
the port while the timer remains armed. max3100_timeout() can then
access the freed port and re-arm the timer.

Add final timer teardown to max3100_remove() and use
timer_shutdown_sync() to prevent a racing callback from re-arming it.
Also drain the IRQ and workqueue before freeing the port.

Keep timer_delete_sync() in max3100_shutdown() so that a subsequent
open() can re-arm the timer.

Introduce an irq_registered flag to track whether the IRQ is registered,
independently of port->irq, so a failed request_irq() can be retried on
the next open().

Found by static analysis.

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>
---
Changes since v1:
  - Drop the shared drain helper; call timer_shutdown_sync() only in
    max3100_remove(), keeping timer_delete_sync() in max3100_shutdown()
    so a later open() can re-arm the timer.
  - Track IRQ registration with a flag instead of clearing port->irq,
    so a failed request_irq() can be retried on the next open().

v1: https://lore.kernel.org/all/20260721035631.3186613-1-fanwu01@zju.edu.cn/
---
 drivers/tty/serial/max3100.c | 18 ++++++++++++++++--
 1 file changed, 16 insertions(+), 2 deletions(-)

diff --git a/drivers/tty/serial/max3100.c b/drivers/tty/serial/max3100.c
index 44b745fa26c6..7bc3c5cfe886 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_registered;
 
 	struct timer_list	timer;
 };
@@ -538,8 +539,10 @@ static void max3100_shutdown(struct uart_port *port)
 		destroy_workqueue(s->workqueue);
 		s->workqueue = NULL;
 	}
-	if (port->irq)
+	if (s->irq_registered) {
 		free_irq(port->irq, s);
+		s->irq_registered = false;
+	}
 
 	/* set shutdown mode to save power */
 	max3100_sr(s, MAX3100_WC | MAX3100_SHDN, &rx);
@@ -575,12 +578,12 @@ static int max3100_startup(struct uart_port *port)
 	ret = request_irq(port->irq, max3100_irq, IRQF_TRIGGER_FALLING, "max3100", s);
 	if (ret < 0) {
 		dev_warn(&s->spi->dev, "cannot allocate irq %d\n", port->irq);
-		port->irq = 0;
 		destroy_workqueue(s->workqueue);
 		s->workqueue = NULL;
 		return -EBUSY;
 	}
 
+	s->irq_registered = true;
 	s->conf_commit = 1;
 	max3100_dowork(s);
 	/* wait for clock to settle */
@@ -752,6 +755,17 @@ 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);
+
+			s->force_end_work = 1;
+			timer_shutdown_sync(&s->timer);
+			if (s->irq_registered) {
+				free_irq(s->port.irq, s);
+				s->irq_registered = false;
+			}
+			if (s->workqueue) {
+				destroy_workqueue(s->workqueue);
+				s->workqueue = NULL;
+			}
 			kfree(max3100s[i]);
 			max3100s[i] = NULL;
 			break;
-- 
2.34.1

Re: [PATCH v2] tty: serial: max3100: shut down timer before freeing port

[Greg KH]

On Sat, Aug 01, 2026 at 06:12:08AM +0000, Fan Wu wrote:
> max3100_shutdown() stops the polling timer but returns early during
> system suspend. If the SPI device is unbound before resume, the serial
> core does not call max3100_shutdown() again, so max3100_remove() frees
> the port while the timer remains armed. max3100_timeout() can then
> access the freed port and re-arm the timer.
> 
> Add final timer teardown to max3100_remove() and use
> timer_shutdown_sync() to prevent a racing callback from re-arming it.
> Also drain the IRQ and workqueue before freeing the port.
> 
> Keep timer_delete_sync() in max3100_shutdown() so that a subsequent
> open() can re-arm the timer.
> 
> Introduce an irq_registered flag to track whether the IRQ is registered,
> independently of port->irq, so a failed request_irq() can be retried on
> the next open().
> 
> Found by static analysis.
> 
> 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>
> ---
> Changes since v1:
>   - Drop the shared drain helper; call timer_shutdown_sync() only in
>     max3100_remove(), keeping timer_delete_sync() in max3100_shutdown()
>     so a later open() can re-arm the timer.
>   - Track IRQ registration with a flag instead of clearing port->irq,
>     so a failed request_irq() can be retried on the next open().
> 
> v1: https://lore.kernel.org/all/20260721035631.3186613-1-fanwu01@zju.edu.cn/
> ---
>  drivers/tty/serial/max3100.c | 18 ++++++++++++++++--
>  1 file changed, 16 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/tty/serial/max3100.c b/drivers/tty/serial/max3100.c
> index 44b745fa26c6..7bc3c5cfe886 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_registered;

LLMs _love_ to use boolean flags to attempt to figure things out that
they can't seem to determine.  Are you _SURE_ this really is needed?
How about unwinding things better so it's not required?  You are just
adding another "state" to the device, adding to the complexity overall,
which is generally not a good idea.

And do you have this hardware to test this with?

thanks,

greg k-h

Re: [PATCH v2] tty: serial: max3100: shut down timer before freeing port

[Fan Wu]

Hi Greg,

Sorry, I don't have MAX3100 hardware, so I have not tested this on
hardware.

You're right about the extra state. I'll rework the cleanup to use the
existing startup/unwind state instead of adding irq_registered, then
send v3 after rebuilding it.

Thanks,
Fan

> On Aug 3, 2026, at 20:56, Greg KH <gregkh@linuxfoundation.org> wrote:

> LLMs _love_ to use boolean flags to attempt to figure things out that
> they can't seem to determine.  Are you _SURE_ this really is needed?
> How about unwinding things better so it's not required?  You are just
> adding another "state" to the device, adding to the complexity overall,
> which is generally not a good idea.
> 
> And do you have this hardware to test this with?
> 
> thanks,
> 
> greg k-h

[PATCH v3] tty: serial: max3100: shut down timer before freeing port

[Fan Wu]

max3100_shutdown() stops the polling timer but returns early during
system suspend. If the SPI device is unbound before resume, the serial
core does not call max3100_shutdown() again, so max3100_remove() frees
the port while the timer remains armed. max3100_timeout() may then
access the freed port and re-arm the timer.

Add final timer teardown to max3100_remove() and use
timer_shutdown_sync() to prevent a racing callback from re-arming it.
Also free the IRQ and destroy the workqueue there before freeing the
port. Keep timer_delete_sync() in max3100_shutdown() so that a
subsequent open() can re-arm the timer.

The workqueue is created before request_irq() and destroyed on both
request_irq() failure and normal shutdown. Its presence at remove thus
identifies the IRQ left registered when suspend bypasses shutdown.

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>
---
Changes since v2:
  - Drop irq_registered; use the workqueue lifetime to decide whether
    remove must release an IRQ left by the suspend path.

v1: https://lore.kernel.org/all/20260721035631.3186613-1-fanwu01@zju.edu.cn/
v2: https://lore.kernel.org/all/20260801061208.356142-1-fanwu01@zju.edu.cn/
---
 drivers/tty/serial/max3100.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/drivers/tty/serial/max3100.c b/drivers/tty/serial/max3100.c
index 44b745fa26c6..48c66b6e1c18 100644
--- a/drivers/tty/serial/max3100.c
+++ b/drivers/tty/serial/max3100.c
@@ -537,9 +537,8 @@ static void max3100_shutdown(struct uart_port *port)
 	if (s->workqueue) {
 		destroy_workqueue(s->workqueue);
 		s->workqueue = NULL;
-	}
-	if (port->irq)
 		free_irq(port->irq, s);
+	}
 
 	/* set shutdown mode to save power */
 	max3100_sr(s, MAX3100_WC | MAX3100_SHDN, &rx);
@@ -752,6 +751,14 @@ 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);
+
+			s->force_end_work = 1;
+			timer_shutdown_sync(&s->timer);
+			if (s->workqueue) {
+				destroy_workqueue(s->workqueue);
+				s->workqueue = NULL;
+				free_irq(s->port.irq, s);
+			}
 			kfree(max3100s[i]);
 			max3100s[i] = NULL;
 			break;
-- 
2.34.1