netdev - Re: [PATCH] bluetooth: fix locking bug in the rfcomm socket cleanup handling

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: <AD9D7FA8-9626-473B-838F-B05ED91742D8@holtmann.org>
Date:	Thu, 29 May 2008 08:49:53 +0200
From:	Marcel Holtmann <marcel@...tmann.org>
To:	Arjan van de Ven <arjan@...radead.org>
Cc:	linux-kernel@...r.kernel.org, netdev@...r.kernel.org,
	davem@...emloft.net, akpm@...ux-foundation.org
Subject: Re: [PATCH] bluetooth: fix locking bug in the rfcomm socket cleanup handling

Hi Arjan,

> From: Arjan van de Ven <arjan@...ux.intel.com>
> Subject: [PATCH] bluetooth: fix locking bug in the rfcomm socket  
> cleanup handling
>
> in net/bluetooth/rfcomm/sock.c, rfcomm_sk_state_change() does the
> following operation:
>
>      if (parent && sock_flag(sk, SOCK_ZAPPED)) {
>              /* We have to drop DLC lock here, otherwise
>               * rfcomm_sock_destruct() will dead lock. */
>              rfcomm_dlc_unlock(d);
>              rfcomm_sock_kill(sk);
>              rfcomm_dlc_lock(d);
>      }
> }
>
> which is fine, since rfcomm_sock_kill() will call sk_free() which  
> will call
> rfcomm_sock_destruct() which takes the rfcomm_dlc_lock()... so far  
> so good.
>
> HOWEVER, this assumes that the rfcomm_sk_state_change() function  
> always gets
> called with the rfcomm_dlc_lock() taken. This is the case for all  
> but one
> case, and in that case where we don't have the lock, we do a double  
> unlock
> followed by an attempt to take the lock, which due to underflow isn't
> going anywhere fast.
>
> This patch fixes this by moving the stragling case inside the lock,  
> like
> the other usages of the same call are doing in this code.
>
> This was found with the help of the www.kerneloops.org project,  
> where this
> deadlock was observed 51 times at this point in time:
> http://www.kerneloops.org/search.php?search=rfcomm_sock_destruct
>
> Signed-off-by: Arjan van de Ven <arjan@...ux.intel.com>
> ---
> net/bluetooth/rfcomm/core.c |    2 +-
> 1 files changed, 1 insertions(+), 1 deletions(-)
>
> diff --git a/net/bluetooth/rfcomm/core.c b/net/bluetooth/rfcomm/core.c
> index eb62558..0c2c937 100644
> --- a/net/bluetooth/rfcomm/core.c
> +++ b/net/bluetooth/rfcomm/core.c
> @@ -423,8 +423,8 @@ static int __rfcomm_dlc_close(struct rfcomm_dlc  
> *d, int err)
>
> 		rfcomm_dlc_lock(d);
> 		d->state = BT_CLOSED;
> -		rfcomm_dlc_unlock(d);
> 		d->state_change(d, err);
> +		rfcomm_dlc_unlock(d);
>
> 		skb_queue_purge(&d->tx_queue);
> 		rfcomm_dlc_unlink(d);

this is really embarrassing, but a good catch on your side. I simply  
never realized that mistake when going through that part of the code.

Acked-by: Marcel Holtmann <marcel@...tmann.org>

David, you might wanna queue this up for stable, too.

Regards

Marcel

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html