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: <656ffd1d-e7cf-d2c0-e0e6-c10215ba422b@kernel.org>
Date:   Thu, 7 Apr 2022 09:02:05 +0200
From:   Jiri Slaby <jirislaby@...nel.org>
To:     Duoming Zhou <duoming@....edu.cn>, linux-kernel@...r.kernel.org
Cc:     chris@...kel.net, jcmvbkbc@...il.com, mustafa.ismail@...el.com,
        shiraz.saleem@...el.com, jgg@...pe.ca, wg@...ndegger.com,
        mkl@...gutronix.de, davem@...emloft.net, kuba@...nel.org,
        pabeni@...hat.com, jes@...ined-monkey.org,
        gregkh@...uxfoundation.org, alexander.deucher@....com,
        linux-xtensa@...ux-xtensa.org, linux-rdma@...r.kernel.org,
        linux-can@...r.kernel.org, netdev@...r.kernel.org,
        linux-hippi@...site.dk, linux-staging@...ts.linux.dev,
        linux-serial@...r.kernel.org, linux-usb@...r.kernel.org,
        Russell King - ARM Linux <linux@...linux.org.uk>
Subject: Re: [PATCH 01/11] drivers: tty: serial: Fix deadlock in
 sa1100_set_termios()

On 07. 04. 22, 8:33, Duoming Zhou wrote:
> There is a deadlock in sa1100_set_termios(), which is shown
> below:
> 
>     (Thread 1)              |      (Thread 2)
>                             | sa1100_enable_ms()
> sa1100_set_termios()       |  mod_timer()
>   spin_lock_irqsave() //(1) |  (wait a time)
>   ...                       | sa1100_timeout()
>   del_timer_sync()          |  spin_lock_irqsave() //(2)
>   (wait timer to stop)      |  ...
> 
> We hold sport->port.lock in position (1) of thread 1 and
> use del_timer_sync() to wait timer to stop, but timer handler
> also need sport->port.lock in position (2) of thread 2. As a result,
> sa1100_set_termios() will block forever.
> 
> This patch extracts del_timer_sync() from the protection of
> spin_lock_irqsave(), which could let timer handler to obtain
> the needed lock.
> 
> Signed-off-by: Duoming Zhou <duoming@....edu.cn>
> ---
>   drivers/tty/serial/sa1100.c | 2 ++
>   1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/tty/serial/sa1100.c b/drivers/tty/serial/sa1100.c
> index 5fe6cccfc1a..3a5f12ced0b 100644
> --- a/drivers/tty/serial/sa1100.c
> +++ b/drivers/tty/serial/sa1100.c
> @@ -476,7 +476,9 @@ sa1100_set_termios(struct uart_port *port, struct ktermios *termios,
>   				UTSR1_TO_SM(UTSR1_ROR);
>   	}
>   
> +	spin_unlock_irqrestore(&sport->port.lock, flags);

Unlocking the lock at this point doesn't look safe at all. Maybe moving 
the timer deletion before the lock? There is no current maintainer to 
ask. Most of the driver originates from rmk. Ccing him just in case.

FWIW the lock was moved by this commit around linux 2.5.55 (from 
full-history-linux [1])
commit f38aef3e62c26a33ea360a86fde9b27e183a3748
Author: Russell King <rmk@...nt.arm.linux.org.uk>
Date:   Fri Jan 3 15:42:09 2003 +0000

     [SERIAL] Convert change_speed() to settermios()

[1] 
https://archive.org/download/git-history-of-linux/full-history-linux.git.tar

>   	del_timer_sync(&sport->timer);
> +	spin_lock_irqsave(&sport->port.lock, flags);
>   
>   	/*
>   	 * Update the per-port timeout.

thanks,
-- 
js
suse labs

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ