linux-kernel - Re: [PATCH] Bluetooth: SCO: fix ABBA deadlock in sco_connect

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] [day] [month] [year] [list]

Message-ID: <7e57d93a41b45650506de4fc68dd04fbbc6b55fc.camel@iki.fi>
Date: Sat, 22 Feb 2025 14:21:13 +0200
From: Pauli Virtanen <pav@....fi>
To: Jeongjun Park <aha310510@...il.com>, marcel@...tmann.org, 
	johan.hedberg@...il.com, luiz.dentz@...il.com
Cc: gustavo.padovan@...labora.co.uk, andre.guedes@...nbossa.org, 
	linux-bluetooth@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] Bluetooth: SCO: fix ABBA deadlock in sco_connect_cfm

Hi,

la, 2025-02-22 kello 20:48 +0900, Jeongjun Park kirjoitti:
> Caused by previous commit 405280887f8f causes ABBA deadlock. So we need to 
> change the lock order to prevent deadlock.

The lock ordering eg. in sco_conn_del() or sco_sock_close() is

	hdev_lock > lock_sock > sco_conn_lock

IIRC this is the lock ordering assumed elsewhere in this file, except
the part touched by 405280887f8f (however, it's locking the parent
socket there so not clear if this causes deadlock, can you clarify if
you have observation/proof of deadlock).

In this patch you have

	sco_conn_lock > lock_sock

Does that also cause a deadlock?

> 
> Fixes: 405280887f8f ("Bluetooth: Reduce critical section in sco_conn_ready")
> Signed-off-by: Jeongjun Park <aha310510@...il.com>
> ---
>  net/bluetooth/sco.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/net/bluetooth/sco.c b/net/bluetooth/sco.c
> index aa7bfe26cb40..8f1377f4a27c 100644
> --- a/net/bluetooth/sco.c
> +++ b/net/bluetooth/sco.c
> @@ -289,13 +289,11 @@ static int sco_chan_add(struct sco_conn *conn, struct sock *sk,
>  {
>  	int err = 0;
>  
> -	sco_conn_lock(conn);
>  	if (conn->sk)
>  		err = -EBUSY;
>  	else
>  		__sco_chan_add(conn, sk, parent);
>  
> -	sco_conn_unlock(conn);
>  	return err;
>  }
>  
> @@ -343,11 +341,13 @@ static int sco_connect(struct sock *sk)
>  		goto unlock;
>  	}
>  
> +	sco_conn_lock(conn);
>  	lock_sock(sk);
>  
>  	err = sco_chan_add(conn, sk, NULL);
>  	if (err) {
>  		release_sock(sk);
> +		sco_conn_unlock(conn);
>  		goto unlock;
>  	}
>  
> @@ -363,6 +363,7 @@ static int sco_connect(struct sock *sk)
>  	}
>  
>  	release_sock(sk);
> +	sco_conn_unlock(conn);
>  
>  unlock:
>  	hci_dev_unlock(hdev);
> --

-- 
Pauli Virtanen