[PATCH] bluetooth: Fix locking issues in the function l2cap_connect_cfm

[PATCH] bluetooth: Fix locking issues in the function l2cap_connect_cfm

[Bastien Philbert]

This fixes a locking issue in the function l2cap_connect_cfm for
not locking the mutex lock for channels on the l2cap_conn structure
pointer conn before calling __l2cap_get_chan_by_dcid as all callers
need to lock and unlock this mutex before calling this function due
to issues with either concurrent users or race conditions arising

Signed-off-by: Bastien Philbert <bastienphilbert@gmail.com>
---
 net/bluetooth/l2cap_core.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c
index eb4f5f2..2ab103e 100644
--- a/net/bluetooth/l2cap_core.c
+++ b/net/bluetooth/l2cap_core.c
@@ -7308,6 +7308,7 @@ static void l2cap_connect_cfm(struct hci_conn *hcon, u8 status)
 		struct l2cap_chan *chan, *next;
 
 		/* Client fixed channels should override server ones */
+		mutex_lock(&conn->chan_lock);
 		if (__l2cap_get_chan_by_dcid(conn, pchan->scid))
 			goto next;
 
@@ -7324,6 +7325,7 @@ static void l2cap_connect_cfm(struct hci_conn *hcon, u8 status)
 
 		l2cap_chan_unlock(pchan);
 next:
+		mutex_unlock(&conn->chan_lock);
 		next = l2cap_global_fixed_chan(pchan, hcon);
 		l2cap_chan_put(pchan);
 		pchan = next;
-- 
2.5.0

Re: [PATCH] bluetooth:Fix locking issues in the function l2cap_connect_cfm

[Johan Hedberg]

Hi Nicholas,

On Mon, Aug 24, 2015, Nicholas Krause wrote:
> --- a/net/bluetooth/l2cap_core.c
> +++ b/net/bluetooth/l2cap_core.c
> @@ -7285,6 +7285,7 @@ static void l2cap_connect_cfm(struct hci_conn *hcon, u8 status)
>  		struct l2cap_chan *chan, *next;
>  
>  		/* Client fixed channels should override server ones */
> +		mutex_lock(&conn->chan_lock);
>  		if (__l2cap_get_chan_by_dcid(conn, pchan->scid))
>  			goto next;
>  
> @@ -7301,6 +7302,7 @@ static void l2cap_connect_cfm(struct hci_conn *hcon, u8 status)
>  
>  		l2cap_chan_unlock(pchan);
>  next:
> +		mutex_lock(&conn->chan_lock);

Shouldn't this be unlock? Btw, It'd be easier to trac the revisions of
your patches if you versioned them properly, e.g. in this case with
"[PATCH v2} Bluetooth: ...".

Johan

[PATCH] bluetooth:Fix locking issues in the function l2cap_connect_cfm

[Nicholas Krause]

This fixes a locking issue in the function l2cap_connect_cfm for
not locking the mutex lock for channels on the l2cap_conn structure
pointer conn before calling __l2cap_get_chan_by_dcid as all callers
need to lock and unlock this mutex before calling this function due
to issues with either concurrent users or race conditions arising
if this mutex is not locked before these calls.

Signed-off-by: Nicholas Krause <xerofoify@gmail.com>
---
 net/bluetooth/l2cap_core.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c
index 45fffa4..fcee783 100644
--- a/net/bluetooth/l2cap_core.c
+++ b/net/bluetooth/l2cap_core.c
@@ -7285,9 +7285,11 @@ static void l2cap_connect_cfm(struct hci_conn *hcon, u8 status)
 		struct l2cap_chan *chan, *next;
 
 		/* Client fixed channels should override server ones */
+		mutex_lock(&conn->chan_lock);
 		if (__l2cap_get_chan_by_dcid(conn, pchan->scid))
 			goto next;
-
+
+		mutex_unlock(&conn->chan_lock);
 		l2cap_chan_lock(pchan);
 		chan = pchan->ops->new_connection(pchan);
 		if (chan) {
@@ -7301,6 +7303,7 @@ static void l2cap_connect_cfm(struct hci_conn *hcon, u8 status)
 
 		l2cap_chan_unlock(pchan);
 next:
+		mutex_unlock(&conn->chan_lock);
 		next = l2cap_global_fixed_chan(pchan, hcon);
 		l2cap_chan_put(pchan);
 		pchan = next;
-- 
2.1.4

Re: [PATCH] bluetooth:Fix locking issues in the function l2cap_connect_cfm

[Johan Hedberg]

Hi Nicholas,

On Tue, Aug 18, 2015, Nicholas Krause wrote:
> This fixes a locking issue in the function l2cap_connect_cfm for
> not locking the mutex lock for channels on the l2cap_conn structure
> pointer conn before calling __l2cap_get_chan_by_dcid as all callers
> need to lock and unlock this mutex before calling this function due
> to issues with either concurrent users or race conditions arising
> if this mutex is not locked before these calls.
> 
> Signed-off-by: Nicholas Krause <xerofoify@gmail.com>
> ---
>  net/bluetooth/l2cap_core.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c
> index 45fffa4..fcee783 100644
> --- a/net/bluetooth/l2cap_core.c
> +++ b/net/bluetooth/l2cap_core.c
> @@ -7285,9 +7285,11 @@ static void l2cap_connect_cfm(struct hci_conn *hcon, u8 status)
>  		struct l2cap_chan *chan, *next;
>  
>  		/* Client fixed channels should override server ones */
> +		mutex_lock(&conn->chan_lock);
>  		if (__l2cap_get_chan_by_dcid(conn, pchan->scid))
>  			goto next;
> -
> +
> +		mutex_unlock(&conn->chan_lock);
>  		l2cap_chan_lock(pchan);
>  		chan = pchan->ops->new_connection(pchan);
>  		if (chan) {
> @@ -7301,6 +7303,7 @@ static void l2cap_connect_cfm(struct hci_conn *hcon, u8 status)
>  
>  		l2cap_chan_unlock(pchan);
>  next:
> +		mutex_unlock(&conn->chan_lock);
>  		next = l2cap_global_fixed_chan(pchan, hcon);
>  		l2cap_chan_put(pchan);
>  		pchan = next;

This looks broken to me. If __l2cap_get_chan_by_dcid() returns NULL
(i.e. the jump to 'next' is *not* taken then the code will end up
calling mutex_unlock(&conn->chan_lock) twice. We can probably take the
penalty of keeping the lock a bit longer, i.e. you should be able to
drop your first unlock() call. In fact you *have* to keep the lock
longer since __l2cap_chan_add() requires it.

Johan