1. Patchew
  2. linux
  3. View series

[PATCH] serial: imx: cancel RS485 trigger hrtimers in shutdown and remove

Fan Wu posted 1 patch 1 month, 1 week ago 
drivers/tty/serial/imx.c | 8 ++++++++
1 file changed, 8 insertions(+)
[PATCH] serial: imx: cancel RS485 trigger hrtimers in shutdown and remove
Posted by Fan Wu 1 month, 1 week ago 
The rs485 delay hrtimers trigger_start_tx and trigger_stop_tx are
embedded in the devm allocated struct imx_port, and their callbacks
reach the port through container_of() and touch registers under the
port lock.  Nothing cancels them synchronously: the tx paths only
call hrtimer_try_to_cancel(), which does not wait for a running
callback, and the bounded wait in imx_uart_shutdown() can give up,
force tx_state to OFF, and leave a timer armed.  After
imx_uart_remove() returns, devm frees the port and a late callback
dereferences freed memory.

Cancel both timers at the end of imx_uart_shutdown(), after the port
lock is dropped and before the clocks are disabled, and again in
imx_uart_remove() before the devm free: serial core does not call the
driver shutdown on every path that reaches remove().

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

Fixes: bd78ecd6056d ("serial: imx: use hrtimers for rs485 delays")
Cc: stable@vger.kernel.org
Assisted-by: Codex:gpt-5.6
Signed-off-by: Fan Wu <fanwu01@zju.edu.cn>
---
 drivers/tty/serial/imx.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c
index 251a50c8aa38..86c99f73c50a 100644
--- a/drivers/tty/serial/imx.c
+++ b/drivers/tty/serial/imx.c
@@ -1707,6 +1707,10 @@ static void imx_uart_shutdown(struct uart_port *port)
 
 	uart_port_unlock_irqrestore(&sport->port, flags);
 
+	/* The rs485 trigger callbacks take the port lock and touch registers. */
+	hrtimer_cancel(&sport->trigger_start_tx);
+	hrtimer_cancel(&sport->trigger_stop_tx);
+
 	clk_disable_unprepare(sport->clk_per);
 	clk_disable_unprepare(sport->clk_ipg);
 }
@@ -2649,6 +2653,10 @@ static void imx_uart_remove(struct platform_device *pdev)
 	struct imx_port *sport = platform_get_drvdata(pdev);
 
 	uart_remove_one_port(&imx_uart_uart_driver, &sport->port);
+
+	/* Serial core can reach remove() without calling the driver shutdown. */
+	hrtimer_cancel(&sport->trigger_start_tx);
+	hrtimer_cancel(&sport->trigger_stop_tx);
 }
 
 static void imx_uart_restore_context(struct imx_port *sport)
Re: [PATCH] serial: imx: cancel RS485 trigger hrtimers in shutdown and remove
Posted by Greg Kroah-Hartman 5 days, 8 hours ago 
On Wed, Aug 19, 2026 at 02:19:16AM +0000, Fan Wu wrote:
> The rs485 delay hrtimers trigger_start_tx and trigger_stop_tx are
> embedded in the devm allocated struct imx_port, and their callbacks
> reach the port through container_of() and touch registers under the
> port lock.  Nothing cancels them synchronously: the tx paths only
> call hrtimer_try_to_cancel(), which does not wait for a running
> callback, and the bounded wait in imx_uart_shutdown() can give up,
> force tx_state to OFF, and leave a timer armed.  After
> imx_uart_remove() returns, devm frees the port and a late callback
> dereferences freed memory.
> 
> Cancel both timers at the end of imx_uart_shutdown(), after the port
> lock is dropped and before the clocks are disabled, and again in
> imx_uart_remove() before the devm free: serial core does not call the
> driver shutdown on every path that reaches remove().
> 
> This issue was found by an in-house static analysis tool.
> 
> Fixes: bd78ecd6056d ("serial: imx: use hrtimers for rs485 delays")
> Cc: stable@vger.kernel.org
> Assisted-by: Codex:gpt-5.6
> Signed-off-by: Fan Wu <fanwu01@zju.edu.cn>
> ---
>  drivers/tty/serial/imx.c | 8 ++++++++
>  1 file changed, 8 insertions(+)

How was this tested?

thanks,

greg k-h
Re: [PATCH] serial: imx: cancel RS485 trigger hrtimers in shutdown and remove
Posted by Fan Wu 1 day, 4 hours ago 
Hi Greg,

Honestly: when I sent the patch it was compile-tested only
(CONFIG_SERIAL_IMX=m, drivers/tty/serial/imx.o rebuilt with no warnings).
I should have said so.  There was no runtime test behind it.

After your mail I built a QEMU/KASAN reproduction (i.MX6UL EVK machine,
armhf, kernel = this patch's base a13c140cc289 + CONFIG_KASAN).  The
remove-side race is deterministic on the unpatched base:

  - ttymxc0 is a registered console; configure RS485 via TIOCSRS485 with
    delay_rts_after_send set (driver clamps it to 100 ms), write() - TX
    completes and imx_uart_stop_tx() arms trigger_stop_tx
  - with the port still open, unbind: for console ports
    tty_port_shutdown() returns early, and serial_core_remove_one_port()
    never calls the driver .shutdown, so nothing sync-cancels the timer
  - imx_uart_remove() returns, devres frees struct imx_port, the queued
    hrtimer expires and the hrtimer core walks the freed object:

BUG: KASAN: slab-use-after-free in rb_erase_linked+0x54/0xa8
Write of size 4 at addr c4046a8c by task swapper/0
Call trace:
 rb_erase_linked from __remove_hrtimer+0x50/0x124
 __remove_hrtimer from __hrtimer_run_queues+0x198/0x25c
 __hrtimer_run_queues from hrtimer_interrupt+0x26c/0x624
 hrtimer_interrupt from arch_timer_handler_phys+0x38/0x40
 ...
Allocated by task 1:
 devm_kmalloc+0x34/0x144
 imx_uart_probe+0x90/0xacc
 ...
Freed by task 1:
 kfree+0xc8/0x2d8
 devres_release_all+0x100/0x18c
 device_unbind_cleanup+0x3c/0xe4
 device_release_driver_internal+0x23c/0x294
 unbind_store+0x64/0xa4
 ...
The buggy address is located 652 bytes inside of
 freed 1024-byte region [c4046800, c4046c00)

i.e. the expired timer is still enqueued inside the freed struct imx_port.
A second report fires from task context via hrtimer_try_to_cancel ->
rb_erase, same alloc/free stacks.

With the patch applied the same sequence (fd-open unbind, and the
last-close-then-unbind variant) completes cleanly; repeated RS485
write/close/unbind cycles on a non-console port stay clean too, with no
new warnings or hangs.  This is QEMU only - no i.MX hardware test yet,
so I have not added a Tested-by.  I can send the reproducer initramfs and
the QEMU command line if that is useful.

Thanks,
Fan
Re: [PATCH] serial: imx: cancel RS485 trigger hrtimers in shutdown and remove
Posted by Greg Kroah-Hartman 1 day, 2 hours ago 
On Sun, Sep 27, 2026 at 02:35:50PM +0000, Fan Wu wrote:
> Hi Greg,
> 
> Honestly: when I sent the patch it was compile-tested only
> (CONFIG_SERIAL_IMX=m, drivers/tty/serial/imx.o rebuilt with no warnings).
> I should have said so.  There was no runtime test behind it.

Sorry, I have no context here as you top-posted and did not include any
context.  Please don't do that.  Remember, some of us get 1000+ emails a
day...

greg k-h
Re: [PATCH] serial: imx: cancel RS485 trigger hrtimers in shutdown and remove
Posted by Fan Wu 17 hours ago 
On Sun, Sep 27, 2026 at 06:28:42PM +0200, Greg Kroah-Hartman wrote:
> Sorry, I have no context here as you top-posted and did not include any
> context.  Please don't do that.  Remember, some of us get 1000+ emails a
> day...

You are right, sorry about that.  Answering your Sep 23 question with
context this time:

On Wed, Sep 23, 2026 at 12:17:41PM +0200, Greg Kroah-Hartman wrote:
> On Wed, Aug 19, 2026 at 02:19:16AM +0000, Fan Wu wrote:
> > The rs485 delay hrtimers trigger_start_tx and trigger_stop_tx are
> > embedded in the devm allocated struct imx_port, and their callbacks
> > reach the port through container_of() and touch registers under the
> > port lock.  Nothing cancels them synchronously: the tx paths only call
> > hrtimer_try_to_cancel(), which does not wait for a running callback,
> > and the bounded wait in imx_uart_shutdown() can give up, force tx_state
> > to OFF, and leave a timer armed.  After imx_uart_remove() returns, devm
> > frees the port and a late callback dereferences freed memory.
> >
> > Cancel both timers at the end of imx_uart_shutdown(), after the port
> > lock is dropped and before the clocks are disabled, and again in
> > imx_uart_remove() before the devm free: serial core does not call the
> > driver shutdown on every path that reaches remove().
> >
> > This issue was found by an in-house static analysis tool.
> >
> > Fixes: bd78ecd6056d ("serial: imx: use hrtimers for rs485 delays")
> >
> > Assisted-by: Codex:gpt-5.6
> > Signed-off-by: Fan Wu <fanwu01@zju.edu.cn>
> How was this tested?

When I sent the patch, it was compile-tested only (CONFIG_SERIAL_IMX=m,
drivers/tty/serial/imx.o rebuilt with no warnings).  I should have said
so.  There was no runtime test behind it.

After your mail I built a QEMU/KASAN reproduction (i.MX6UL EVK machine,
armhf, kernel = this patch's base a13c140cc289 + CONFIG_KASAN).  The
remove-side race is deterministic on the unpatched base:

  - ttymxc0 is a registered console; configure RS485 via TIOCSRS485 with
    delay_rts_after_send set (driver clamps it to 100 ms), write() - TX
    completes and imx_uart_stop_tx() arms trigger_stop_tx
  - with the port still open, unbind: for console ports
    tty_port_shutdown() returns early, and serial_core_remove_one_port()
    never calls the driver .shutdown, so nothing sync-cancels the timer
  - imx_uart_remove() returns, devres frees struct imx_port, the queued
    hrtimer expires and the hrtimer core walks the freed object:

BUG: KASAN: slab-use-after-free in rb_erase_linked+0x54/0xa8
Write of size 4 at addr c4046a8c by task swapper/0
Call trace:
 rb_erase_linked from __remove_hrtimer+0x50/0x124
 __remove_hrtimer from __hrtimer_run_queues+0x198/0x25c
 __hrtimer_run_queues from hrtimer_interrupt+0x26c/0x624
 hrtimer_interrupt from arch_timer_handler_phys+0x38/0x40
 ...
Allocated by task 1:
 devm_kmalloc+0x34/0x144
 imx_uart_probe+0x90/0xacc
 ...
Freed by task 1:
 kfree+0xc8/0x2d8
 devres_release_all+0x100/0x18c
 device_unbind_cleanup+0x3c/0xe4
 device_release_driver_internal+0x23c/0x294
 unbind_store+0x64/0xa4
 ...
The buggy address is located 652 bytes inside of
 freed 1024-byte region [c4046800, c4046c00)

i.e. the expired timer is still enqueued inside the freed struct imx_port.
A second report fires from task context via hrtimer_try_to_cancel ->
rb_erase, same alloc/free stacks.

With the patch applied the same sequence (fd-open unbind, and the
last-close-then-unbind variant) completes cleanly; repeated RS485
write/close/unbind cycles on a non-console port stay clean too, with no
new warnings or hangs.  This is QEMU only - no i.MX hardware test yet,
so I have not added a Tested-by.  I can send the reproducer initramfs and
the QEMU command line if that is useful.

Thanks,
Fan
Re: [PATCH] serial: imx: cancel RS485 trigger hrtimers in shutdown and remove
Posted by Jiri Slaby 1 month, 1 week ago 
On 19. 08. 26, 4:19, Fan Wu wrote:
> The rs485 delay hrtimers trigger_start_tx and trigger_stop_tx are
> embedded in the devm allocated struct imx_port, and their callbacks
> reach the port through container_of() and touch registers under the
> port lock.  Nothing cancels them synchronously: the tx paths only
> call hrtimer_try_to_cancel(), which does not wait for a running
> callback, and the bounded wait in imx_uart_shutdown() can give up,
> force tx_state to OFF, and leave a timer armed.  After
> imx_uart_remove() returns, devm frees the port and a late callback
> dereferences freed memory.
> 
> Cancel both timers at the end of imx_uart_shutdown(), after the port
> lock is dropped and before the clocks are disabled, and again in
> imx_uart_remove() before the devm free:


> serial core does not call the
> driver shutdown on every path that reaches remove().

Could you be more specific on what path it does not?

thanks,
-- 
js
suse labs
Re: [PATCH] serial: imx: cancel RS485 trigger hrtimers in shutdown and remove
Posted by Fan Wu 1 month, 1 week ago 
> On Aug 19, 2026, at 12:26, Jiri Slaby <jirislaby@kernel.org> wrote:
> 
>> serial core does not call the
>> driver shutdown on every path that reaches remove().
> 
> Could you be more specific on what path it does not?
> 
> thanks,
> -- 
> js
> suse labs

Hi Jiri

Yes. The relevant path is a console port whose last TTY user closes it
before the device is unbound.

tty_port_shutdown() skips port->ops->shutdown() for console ports. The
last close then clears the active state and dissociates the TTY, so the
later tty_port_tty_vhangup() in the remove path has no TTY to hang up
and cannot invoke the driver shutdown.  That is the path the cancel in
imx_uart_remove() covers.

trigger_stop_tx can already be pending at that point: after TXDC,
imx_uart_stop_tx() starts it for delay_rts_after_send, whereas the close
path only waits for TXDC.

I can add this to the changelog if you would prefer the rationale to be
spelled out there.

Thanks,
Fan

Patchew 2.1-devel-ea86937

© 2016 - 2026 Red Hat, Inc.