[PATCH 1/4] Bluetooth: Fix L2CAP PSM bind issue

[PATCH 1/4] Bluetooth: Fix L2CAP PSM bind issue

[Syam Sidhardhan]

Problem: If we bind a particular PSM with source address as BDADDR_ANY,
then we are able to bind the same PSM with adapter address(or any other
address), which is incorrect.

Solution: Added check for comparing the stored source address and
given source address against BDADDR_ANY, so that irrespective of
BADADDR_ANY or any adapter address, a particular PSM will be allowed
to bind only once.

Details: After successful binding, both the PSM and the source device
address are stored in a global list. Before binding to a new PSM, the
old PSM and the source address from the gobal list are compared against
the new PSM and source device address for make the PSM binding unique.

Before this fix, If we pass the different device addresses, for the same
PSM, both the binding are successful. Further an incoming connection to
that particular PSM goes to incorrect profile (first binded). This PSM
binding issues can be easily reproducible using the l2test utility as
shown below.

Bind to a PSM without the device address
-sh-4.1# l2test -w -P 4097
l2test[2792]: Waiting for connection on psm 4097 ...

In another terminal, bind to same PSM with device address
-sh-4.1# l2test -w -P 4097 -i hci0
l2test[2787]: Waiting for connection on psm 4097 ...

Here we can see the binding is successful for both cases for the same
PSM. After this fix the binding for a particular PSM is allowed only
once.

Signed-off-by: Syam Sidhardhan <s.syam@samsung.com>
---
This patch resolves the HDP incoming connection issue when Obexd is
running in background. Both uses dynamic PSM range. Obex over l2cap
bind with source address as BDADDR_ANY and HDP binds with source address
as the adapter address. Both bindings for the same PSM are getting
success.

 net/bluetooth/l2cap_core.c |   11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c
index d42cdb1..ace7cd8 100644
--- a/net/bluetooth/l2cap_core.c
+++ b/net/bluetooth/l2cap_core.c
@@ -113,7 +113,16 @@ static struct l2cap_chan *__l2cap_global_chan_by_addr(__le16 psm, bdaddr_t *src)
 	struct l2cap_chan *c;
 
 	list_for_each_entry(c, &chan_list, global_l) {
-		if (c->sport == psm && !bacmp(&bt_sk(c->sk)->src, src))
+		if (c->sport != psm)
+			continue;
+
+		/* Exact match */
+		if (!bacmp(&bt_sk(c->sk)->src, src))
+			return c;
+
+		/* BDADDR_ANY match */
+		if (!bacmp(&bt_sk(c->sk)->src, BDADDR_ANY) ||
+		    !bacmp(src, BDADDR_ANY))
 			return c;
 	}
 	return NULL;
-- 
1.7.9.5

[PATCH 2/4] Bluetooth: Fix RFCOMM bind issue

[Syam Sidhardhan]

Problem: If we bind a particular RFCOMM channel with source address as
BDADDR_ANY, then we are able to bind the same RFCOMM channel with the
adapter address, which is incorrect.

Solution: Add check for comparing the stored source address and
given source address against BDADDR_ANY, so that irrespective of
BADADDR_ANY or any adapter address, a particular RFCOMM channel will
be allowed to bind only once.

Details:
Here given the steps to reproduce this issue:
Bind to a RFCOMM channel without the device address.
-sh-4.1# rctest -w -P 5
rctest[2920]: Waiting for connection on channel 5 ...

In another terminal, bind to same RFCOMM channel with a device address
-sh-4.1# rctest -w -P 5 -i hci0
rctest[2922]: Waiting for connection on channel 5 ...

Here we can see the binding is successful for both cases for the same
RFCOMM channel. After this fix, the binding for a particular RFCOMM
channel is allowed only once.

Signed-off-by: Syam Sidhardhan <s.syam@samsung.com>
---
 net/bluetooth/rfcomm/sock.c |   12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/net/bluetooth/rfcomm/sock.c b/net/bluetooth/rfcomm/sock.c
index 867a065..4add776 100644
--- a/net/bluetooth/rfcomm/sock.c
+++ b/net/bluetooth/rfcomm/sock.c
@@ -110,8 +110,16 @@ static struct sock *__rfcomm_get_sock_by_addr(u8 channel, bdaddr_t *src)
 	struct hlist_node *node;
 
 	sk_for_each(sk, node, &rfcomm_sk_list.head) {
-		if (rfcomm_pi(sk)->channel == channel &&
-				!bacmp(&bt_sk(sk)->src, src))
+		if (rfcomm_pi(sk)->channel != channel)
+			continue;
+
+		/* Exact match */
+		if (!bacmp(&bt_sk(sk)->src, src))
+			break;
+
+		/* BDADDR_ANY match */
+		if (!bacmp(&bt_sk(sk)->src, BDADDR_ANY) ||
+		    !bacmp(src, BDADDR_ANY))
 			break;
 	}
 
-- 
1.7.9.5

[PATCH 3/4] Bluetooth: Use __constant modifier for L2CAP SMP CID

[Syam Sidhardhan]

Since the L2CAP_CID_SMP is constant, __constant_cpu_to_le16() is
the right go here.

Signed-off-by: Syam Sidhardhan <s.syam@samsung.com>
---
 net/bluetooth/smp.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/bluetooth/smp.c b/net/bluetooth/smp.c
index 8c225ef..9b54fea 100644
--- a/net/bluetooth/smp.c
+++ b/net/bluetooth/smp.c
@@ -165,7 +165,7 @@ static struct sk_buff *smp_build_cmd(struct l2cap_conn *conn, u8 code,
 
 	lh = (struct l2cap_hdr *) skb_put(skb, L2CAP_HDR_SIZE);
 	lh->len = cpu_to_le16(sizeof(code) + dlen);
-	lh->cid = cpu_to_le16(L2CAP_CID_SMP);
+	lh->cid = __constant_cpu_to_le16(L2CAP_CID_SMP);
 
 	memcpy(skb_put(skb, sizeof(code)), &code, sizeof(code));
 
-- 
1.7.9.5

Re: [PATCH 3/4] Bluetooth: Use __constant modifier for L2CAP SMP CID

[Marcel Holtmann]

Hi Syam,

> Since the L2CAP_CID_SMP is constant, __constant_cpu_to_le16() is
> the right go here.
> 
> Signed-off-by: Syam Sidhardhan <s.syam@samsung.com>
> ---
>  net/bluetooth/smp.c |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

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

Regards

Marcel

[PATCH 4/4] Bluetooth: Use __constant modifier for RFCOMM PSM

[Syam Sidhardhan]

Since the RFCOMM_PSM is constant, __constant_cpu_to_le16() is
the right go here.

Signed-off-by: Syam Sidhardhan <s.syam@samsung.com>
---
 net/bluetooth/rfcomm/core.c |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/net/bluetooth/rfcomm/core.c b/net/bluetooth/rfcomm/core.c
index fb1d83d..201fdf7 100644
--- a/net/bluetooth/rfcomm/core.c
+++ b/net/bluetooth/rfcomm/core.c
@@ -709,7 +709,7 @@ static struct rfcomm_session *rfcomm_session_create(bdaddr_t *src,
 
 	bacpy(&addr.l2_bdaddr, dst);
 	addr.l2_family = AF_BLUETOOTH;
-	addr.l2_psm    = cpu_to_le16(RFCOMM_PSM);
+	addr.l2_psm    = __constant_cpu_to_le16(RFCOMM_PSM);
 	addr.l2_cid    = 0;
 	*err = kernel_connect(sock, (struct sockaddr *) &addr, sizeof(addr), O_NONBLOCK);
 	if (*err == 0 || *err == -EINPROGRESS)
@@ -1987,7 +1987,7 @@ static int rfcomm_add_listener(bdaddr_t *ba)
 	/* Bind socket */
 	bacpy(&addr.l2_bdaddr, ba);
 	addr.l2_family = AF_BLUETOOTH;
-	addr.l2_psm    = cpu_to_le16(RFCOMM_PSM);
+	addr.l2_psm    = __constant_cpu_to_le16(RFCOMM_PSM);
 	addr.l2_cid    = 0;
 	err = kernel_bind(sock, (struct sockaddr *) &addr, sizeof(addr));
 	if (err < 0) {
-- 
1.7.9.5

Re: [PATCH 4/4] Bluetooth: Use __constant modifier for RFCOMM PSM

[Marcel Holtmann]

Hi Syam,

> Since the RFCOMM_PSM is constant, __constant_cpu_to_le16() is
> the right go here.
> 
> Signed-off-by: Syam Sidhardhan <s.syam@samsung.com>
> ---
>  net/bluetooth/rfcomm/core.c |    4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)

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

Regards

Marcel

Re: [PATCH 4/4] Bluetooth: Use __constant modifier for RFCOMM PSM

[Gustavo Padovan]

Hi Syam,

* Syam Sidhardhan <s.syam@samsung.com> [2012-10-10 22:09:29 +0530]:

> Since the RFCOMM_PSM is constant, __constant_cpu_to_le16() is
> the right go here.
> 
> Signed-off-by: Syam Sidhardhan <s.syam@samsung.com>
> ---
>  net/bluetooth/rfcomm/core.c |    4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)

Patch 3 and 4 have been applied to bluetooth-next. Thanks.

	Gustavo

Re: [PATCH 1/4] Bluetooth: Fix L2CAP PSM bind issue

[Marcel Holtmann]

Hi Syam,

> Problem: If we bind a particular PSM with source address as BDADDR_ANY,
> then we are able to bind the same PSM with adapter address(or any other
> address), which is incorrect.

why is this incorrect? Explain that to me.

Regards

Marcel

Re: [PATCH 1/4] Bluetooth: Fix L2CAP PSM bind issue

[Syam Sidhardhan]

Hi Marcel,

----- Original Message ----- 
From: "Marcel Holtmann" <marcel@holtmann.org>
To: "Syam Sidhardhan" <s.syam@samsung.com>
Cc: <linux-bluetooth@vger.kernel.org>
Sent: Wednesday, October 10, 2012 11:17 PM
Subject: Re: [PATCH 1/4] Bluetooth: Fix L2CAP PSM bind issue


> Hi Syam,
>
>> Problem: If we bind a particular PSM with source address as BDADDR_ANY,
>> then we are able to bind the same PSM with adapter address(or any other
>> address), which is incorrect.
>
> why is this incorrect? Explain that to me.
>

Here there is a correction required in the commit message.

As per my understanding the a particular PSM can be binded only once
for a single adapter address. Kindly correct me if I'm wrong here.

As I mentioned in the additional comment part of the patch, there is
a PSM conflict happens between the Obexd and HDP, because of the
source address differences. With this patch, it's getting resolved and
I'm able to run Obexd and HDP simultaniously.

The otherway to fix it, is in userspace. Always provide the source adapter
address during the PSM binding, rather than providing BDADDR_ANY.

If this patch code changes are valid, then I can send a seperate version 
patch with
the following corrected commit message description.

"Problem: If we bind a particular PSM with source address as BDADDR_ANY,
then we are able to bind the same PSM with adapter address." ...

Kindly let me know.

Thanks,
Syam. 

Re: [PATCH 1/4] Bluetooth: Fix L2CAP PSM bind issue

[Marcel Holtmann]

Hi Syam,

> >> Problem: If we bind a particular PSM with source address as BDADDR_ANY,
> >> then we are able to bind the same PSM with adapter address(or any other
> >> address), which is incorrect.
> >
> > why is this incorrect? Explain that to me.
> >
> 
> Here there is a correction required in the commit message.
> 
> As per my understanding the a particular PSM can be binded only once
> for a single adapter address. Kindly correct me if I'm wrong here.

a PSM can be bound once per adapter address. And of course once per
BDADDR_ANY.

So you can bind PSM 23 to BDADDR_ANY and 11:22:33:44:55:66. An incoming
connection to 11:22:33:44:55:66 will arrive to that specific socket, but
an incoming to all others will be handled by the BDADDR_ANY.

The BDADDR_ANY is special. Consider it a wildcard bound with lower
priority. If overwritten with a specific address bound, that will be
considered first of course.

If we are not doing it that way, than that is a bug.

Outgoing connections should behave the same btw. If a specific address
is bound, than that will be used. If not available, then you can not
connect. If BDADDR_ANY is used, then the next available adapter will be
used.

Regards

Marcel

Re: [PATCH 1/4] Bluetooth: Fix L2CAP PSM bind issue

[Luiz Augusto von Dentz]

Hi Marcel,

On Thu, Oct 11, 2012 at 3:56 PM, Marcel Holtmann <marcel@holtmann.org> wrote:
> Hi Syam,
>
>> >> Problem: If we bind a particular PSM with source address as BDADDR_ANY,
>> >> then we are able to bind the same PSM with adapter address(or any other
>> >> address), which is incorrect.
>> >
>> > why is this incorrect? Explain that to me.
>> >
>>
>> Here there is a correction required in the commit message.
>>
>> As per my understanding the a particular PSM can be binded only once
>> for a single adapter address. Kindly correct me if I'm wrong here.
>
> a PSM can be bound once per adapter address. And of course once per
> BDADDR_ANY.
>
> So you can bind PSM 23 to BDADDR_ANY and 11:22:33:44:55:66. An incoming
> connection to 11:22:33:44:55:66 will arrive to that specific socket, but
> an incoming to all others will be handled by the BDADDR_ANY.
>
> The BDADDR_ANY is special. Consider it a wildcard bound with lower
> priority. If overwritten with a specific address bound, that will be
> considered first of course.
>
> If we are not doing it that way, than that is a bug.
>
> Outgoing connections should behave the same btw. If a specific address
> is bound, than that will be used. If not available, then you can not
> connect. If BDADDR_ANY is used, then the next available adapter will be
> used.

The real problem is by giving psm 0 the kernel should return the first
available psm in the non-reserved space, but since we reuse the same
code to do the matching it end up given both obexd and hdp the same
psm, IMO in this case we should consider using Syam code and not
__l2cap_global_chan_by_addr as the userspace is probably asking for a
psm not in use which should  exclude those in use by BDADDR_ANY.

-- 
Luiz Augusto von Dentz

Re: [PATCH 1/4] Bluetooth: Fix L2CAP PSM bind issue

[Syam Sidhardhan]

Hi Marcel, Luiz,

----- Original Message ----- 
From: "Luiz Augusto von Dentz" <luiz.dentz@gmail.com>
To: "Marcel Holtmann" <marcel@holtmann.org>
Cc: "Syam Sidhardhan" <s.syam@samsung.com>; 
<linux-bluetooth@vger.kernel.org>
Sent: Thursday, October 11, 2012 7:59 PM
Subject: Re: [PATCH 1/4] Bluetooth: Fix L2CAP PSM bind issue


> Hi Marcel,
>
> On Thu, Oct 11, 2012 at 3:56 PM, Marcel Holtmann <marcel@holtmann.org> 
> wrote:
>> Hi Syam,
>>
>>> >> Problem: If we bind a particular PSM with source address as 
>>> >> BDADDR_ANY,
>>> >> then we are able to bind the same PSM with adapter address(or any 
>>> >> other
>>> >> address), which is incorrect.
>>> >
>>> > why is this incorrect? Explain that to me.
>>> >
>>>
>>> Here there is a correction required in the commit message.
>>>
>>> As per my understanding the a particular PSM can be binded only once
>>> for a single adapter address. Kindly correct me if I'm wrong here.
>>
>> a PSM can be bound once per adapter address. And of course once per
>> BDADDR_ANY.
>>
>> So you can bind PSM 23 to BDADDR_ANY and 11:22:33:44:55:66. An incoming
>> connection to 11:22:33:44:55:66 will arrive to that specific socket, but
>> an incoming to all others will be handled by the BDADDR_ANY.
>>
>> The BDADDR_ANY is special. Consider it a wildcard bound with lower
>> priority. If overwritten with a specific address bound, that will be
>> considered first of course.
>>
>> If we are not doing it that way, than that is a bug.
>>
>> Outgoing connections should behave the same btw. If a specific address
>> is bound, than that will be used. If not available, then you can not
>> connect. If BDADDR_ANY is used, then the next available adapter will be
>> used.
>
> The real problem is by giving psm 0 the kernel should return the first
> available psm in the non-reserved space, but since we reuse the same
> code to do the matching it end up given both obexd and hdp the same
> psm, IMO in this case we should consider using Syam code and not
> __l2cap_global_chan_by_addr as the userspace is probably asking for a
> psm not in use which should  exclude those in use by BDADDR_ANY.
>

It seems there is a bug.
I tried to listen first using BDADDR_ANY and then again listen
using the adapter address for a particular PSM. If I try to connect
on that particular PSM from the remote device, then the incoming
connection follows the order of the listen rather than the priority.

Ex:
1) Listen
-sh-4.1# /opt/l2test -d -P 11
l2test[3450]: Waiting for connection on psm 11 ...

2) Listen
-sh-4.1# /opt/l2test -w -P 11 -i hci0
l2test[3451]: Waiting for connection on psm 11 ...

If we try to connect from the remote device, then the connection goes to 
First listen
-sh-4.1# /opt/l2test -n -P 11 00:02:4B:28:2C:48

Similarly if we swap the order of the listen 1 & 2, then connection goes to 
hci0 one.

The following diff resolves the issue for me. I'm not sure its correct or 
not.

diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c
index 33b5d34..12f9b52 100644
--- a/net/bluetooth/l2cap_core.c
+++ b/net/bluetooth/l2cap_core.c
@@ -1499,7 +1499,7 @@ static struct l2cap_chan *l2cap_global_chan_by_psm(int 
state, __le16 psm,
                        src_any = !bacmp(&bt_sk(sk)->src, BDADDR_ANY);
                        dst_any = !bacmp(&bt_sk(sk)->dst, BDADDR_ANY);
                        if ((src_match && dst_any) || (src_any && dst_match) 
||
-                           (src_any && dst_any))
+                           (!c1 && src_any && dst_any))
                                c1 = c;
                }
        }

Regards,
Syam