linux-kernel - Re: [PATCH v1 1/1] serial: core: Clearing the circular buffer before NULLifying it

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <f0f200b0-34dc-430b-b55e-b133faf4db44@kernel.org>
Date: Fri, 5 Apr 2024 07:25:03 +0200
From: Jiri Slaby <jirislaby@...nel.org>
To: Andy Shevchenko <andriy.shevchenko@...ux.intel.com>,
 Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
 linux-kernel@...r.kernel.org, linux-serial@...r.kernel.org
Cc: Yicong Yang <yangyicong@...ilicon.com>, Tony Lindgren <tony@...mide.com>,
 kernel test robot <oliver.sang@...el.com>
Subject: Re: [PATCH v1 1/1] serial: core: Clearing the circular buffer before
 NULLifying it

On 04. 04. 24, 16:59, Andy Shevchenko wrote:
> The circular buffer is NULLified in uart_tty_port_shutdown()
> under the spin lock. However, the PM or other timer based callbacks
> may still trigger after this event without knowning that buffer pointer
> is not valid. Since the serial code is a bit inconsistent in checking
> the buffer state (some rely on the head-tail positions, some on the
> buffer pointer), it's better to have both aligned, i.e. buffer pointer
> to be NULL and head-tail possitions to be the same, meaning it's empty.
> This will prevent asynchronous calls to dereference NULL pointer as
> reported recently in 8250 case:
> 
>    BUG: kernel NULL pointer dereference, address: 00000cf5
>    Workqueue: pm pm_runtime_work
>    EIP: serial8250_tx_chars (drivers/tty/serial/8250/8250_port.c:1809)
>    ...
>    ? serial8250_tx_chars (drivers/tty/serial/8250/8250_port.c:1809)
>    __start_tx (drivers/tty/serial/8250/8250_port.c:1551)
>    serial8250_start_tx (drivers/tty/serial/8250/8250_port.c:1654)
>    serial_port_runtime_suspend (include/linux/serial_core.h:667 drivers/tty/serial/serial_port.c:63)
>    __rpm_callback (drivers/base/power/runtime.c:393)
>    ? serial_port_remove (drivers/tty/serial/serial_port.c:50)
>    rpm_suspend (drivers/base/power/runtime.c:447)

Yeah, I noticed start_tx() is called repeatedly after shutdown() 
yesterday too. So thanks for looking into this.

And it's pretty weird. I think it's new with the runtime PM (sure, /me 
reads Fixes: now). I am not sure if it is documented, but most of the 
code in tty/ assumes NO ordinary ->ops (like start_tx()) are called 
after shutdown(). Actually, to me it occurs like serial8250_start_tx() 
should not be called in the first place. It makes no sense after all.

BTW cannot be x_char en/queued at that time too (the other check in the 
if)? But again, serial8250_start_tx() should not be called after shutdown().

> The proposed change will prevent ->start_tx() to be called during
> suspend on shut down port.
> 
> Fixes: 43066e32227e ("serial: port: Don't suspend if the port is still busy")
> Reported-by: kernel test robot <oliver.sang@...el.com>
> Closes: https://lore.kernel.org/oe-lkp/202404031607.2e92eebe-lkp@intel.com
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
> ---
> 
> I have got into the very similar issue while working on max3100 driver.
> I haven't checked the 8250 case, but for mine the culprit is the same
> and this patch fixes it. Hence I assume it will fix the 8250 case as
> well.
> 
>   drivers/tty/serial/serial_core.c | 1 +
>   1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c
> index a005fc06a077..ba3a674a8bbf 100644
> --- a/drivers/tty/serial/serial_core.c
> +++ b/drivers/tty/serial/serial_core.c
> @@ -1788,6 +1788,7 @@ static void uart_tty_port_shutdown(struct tty_port *port)
>   	 * Free the transmit buffer.
>   	 */
>   	uart_port_lock_irq(uport);
> +	uart_circ_clear(&state->xmit);
>   	buf = state->xmit.buf;
>   	state->xmit.buf = NULL;
>   	uart_port_unlock_irq(uport);

-- 
js
suse labs