2010-08-19 11:06:10

by Luiz Augusto von Dentz

[permalink] [raw]
Subject: [PATCH] bluetooth: fix not setting security level when creating a rfcomm session

From: Luiz Augusto von Dentz <[email protected]>

This cause 'No Bonding' to be used if userspace has not yet been paired
with remote device since the l2cap socket used to create the rfcomm
session does not have any security level set.

Signed-off-by: Luiz Augusto von Dentz <[email protected]>
---
net/bluetooth/rfcomm/core.c | 13 ++++++++++---
1 files changed, 10 insertions(+), 3 deletions(-)

diff --git a/net/bluetooth/rfcomm/core.c b/net/bluetooth/rfcomm/core.c
index 7dca91b..08f5757 100644
--- a/net/bluetooth/rfcomm/core.c
+++ b/net/bluetooth/rfcomm/core.c
@@ -79,7 +79,10 @@ static void rfcomm_make_uih(struct sk_buff *skb, u8 addr);

static void rfcomm_process_connect(struct rfcomm_session *s);

-static struct rfcomm_session *rfcomm_session_create(bdaddr_t *src, bdaddr_t *dst, int *err);
+static struct rfcomm_session *rfcomm_session_create(bdaddr_t *src,
+ bdaddr_t *dst,
+ u8 sec_level,
+ int *err);
static struct rfcomm_session *rfcomm_session_get(bdaddr_t *src, bdaddr_t *dst);
static void rfcomm_session_del(struct rfcomm_session *s);

@@ -402,7 +405,7 @@ static int __rfcomm_dlc_open(struct rfcomm_dlc *d, bdaddr_t *src, bdaddr_t *dst,

s = rfcomm_session_get(src, dst);
if (!s) {
- s = rfcomm_session_create(src, dst, &err);
+ s = rfcomm_session_create(src, dst, d->sec_level, &err);
if (!s)
return err;
}
@@ -680,7 +683,10 @@ static void rfcomm_session_close(struct rfcomm_session *s, int err)
rfcomm_session_put(s);
}

-static struct rfcomm_session *rfcomm_session_create(bdaddr_t *src, bdaddr_t *dst, int *err)
+static struct rfcomm_session *rfcomm_session_create(bdaddr_t *src,
+ bdaddr_t *dst,
+ u8 sec_level,
+ int *err)
{
struct rfcomm_session *s = NULL;
struct sockaddr_l2 addr;
@@ -705,6 +711,7 @@ static struct rfcomm_session *rfcomm_session_create(bdaddr_t *src, bdaddr_t *dst
sk = sock->sk;
lock_sock(sk);
l2cap_pi(sk)->imtu = l2cap_mtu;
+ l2cap_pi(sk)->sec_level = sec_level;
if (l2cap_ertm)
l2cap_pi(sk)->mode = L2CAP_MODE_ERTM;
release_sock(sk);
--
1.7.0.4



2010-08-20 10:55:39

by Ville Tervo

[permalink] [raw]
Subject: Re: [PATCH] bluetooth: fix not setting security level when creating a rfcomm session

On Thu, Aug 19, 2010 at 01:06:10PM +0200, Luiz Augusto von Dentz wrote:
> From: Luiz Augusto von Dentz <[email protected]>
>
> This cause 'No Bonding' to be used if userspace has not yet been paired
> with remote device since the l2cap socket used to create the rfcomm
> session does not have any security level set.

Looks good.

> Signed-off-by: Luiz Augusto von Dentz <[email protected]>

Acked-by: Ville Tervo <[email protected]>

--
Ville

2010-08-03 08:27:33

by Luiz Augusto von Dentz

[permalink] [raw]
Subject: Re: [PATCH] bluetooth: fix not setting security level when creating a rfcomm session

Hi Marcel,

On Tue, Aug 3, 2010 at 10:59 AM, Marcel Holtmann <[email protected]> wrot=
e:
> Hi Luiz,
>
>> This cause 'No Bonding' to be used if userspace has not yet been paired
>> with remote device since the l2cap socket used to create the rfcomm
>> session does not have any security level set.
>>
>> Signed-off-by: Luiz Augusto von Dentz <[email protected]>
>> ---
>> =A0net/bluetooth/rfcomm/core.c | =A0 13 ++++++++++---
>> =A01 files changed, 10 insertions(+), 3 deletions(-)
>>
>> diff --git a/net/bluetooth/rfcomm/core.c b/net/bluetooth/rfcomm/core.c
>> index 7dca91b..08f5757 100644
>> --- a/net/bluetooth/rfcomm/core.c
>> +++ b/net/bluetooth/rfcomm/core.c
>> @@ -79,7 +79,10 @@ static void rfcomm_make_uih(struct sk_buff *skb, u8 a=
ddr);
>>
>> =A0static void rfcomm_process_connect(struct rfcomm_session *s);
>>
>> -static struct rfcomm_session *rfcomm_session_create(bdaddr_t *src, bdad=
dr_t *dst, int *err);
>> +static struct rfcomm_session *rfcomm_session_create(bdaddr_t *src,
>> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =
=A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 bdaddr_t *dst,
>> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =
=A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 u8 sec_level,
>> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =
=A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 int *err);
>> =A0static struct rfcomm_session *rfcomm_session_get(bdaddr_t *src, bdadd=
r_t *dst);
>> =A0static void rfcomm_session_del(struct rfcomm_session *s);
>>
>> @@ -402,7 +405,7 @@ static int __rfcomm_dlc_open(struct rfcomm_dlc *d, b=
daddr_t *src, bdaddr_t *dst,
>>
>> =A0 =A0 =A0 s =3D rfcomm_session_get(src, dst);
>> =A0 =A0 =A0 if (!s) {
>> - =A0 =A0 =A0 =A0 =A0 =A0 s =3D rfcomm_session_create(src, dst, &err);
>> + =A0 =A0 =A0 =A0 =A0 =A0 s =3D rfcomm_session_create(src, dst, d->sec_l=
evel, &err);
>> =A0 =A0 =A0 =A0 =A0 =A0 =A0 if (!s)
>> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 return err;
>> =A0 =A0 =A0 }
>> @@ -680,7 +683,10 @@ static void rfcomm_session_close(struct rfcomm_sess=
ion *s, int err)
>> =A0 =A0 =A0 rfcomm_session_put(s);
>> =A0}
>>
>> -static struct rfcomm_session *rfcomm_session_create(bdaddr_t *src, bdad=
dr_t *dst, int *err)
>> +static struct rfcomm_session *rfcomm_session_create(bdaddr_t *src,
>> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =
=A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 bdaddr_t *dst,
>> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =
=A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 u8 sec_level,
>> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =
=A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 int *err)
>> =A0{
>> =A0 =A0 =A0 struct rfcomm_session *s =3D NULL;
>> =A0 =A0 =A0 struct sockaddr_l2 addr;
>> @@ -705,6 +711,7 @@ static struct rfcomm_session *rfcomm_session_create(=
bdaddr_t *src, bdaddr_t *dst
>> =A0 =A0 =A0 sk =3D sock->sk;
>> =A0 =A0 =A0 lock_sock(sk);
>> =A0 =A0 =A0 l2cap_pi(sk)->imtu =3D l2cap_mtu;
>> + =A0 =A0 l2cap_pi(sk)->sec_level =3D sec_level;
>> =A0 =A0 =A0 if (l2cap_ertm)
>> =A0 =A0 =A0 =A0 =A0 =A0 =A0 l2cap_pi(sk)->mode =3D L2CAP_MODE_ERTM;
>> =A0 =A0 =A0 release_sock(sk);
>
> I see what you are trying to do here, but why is this needed? Shouldn't
> it be already correctly set for PSM 3?

I couldn't find any special check for PSM 3, but I guess it would make
sense to make sure it always set the security level from dlc which
triggered the session, doesn't it? As it is now it first triggers No
Bonding and then when we actually connect the dlc it might have a
different sec_level which might triggers another round of io
capability negotiation, not only this doesn't work always, because
there is a bug in bluetoothd that won't invalidate/remove stored link
keys once we got a new one that is not supposed to be stored (No
Bonding), but IMO much simpler too. Maybe we can do as SDP which seems
to do the check in l2cap.c:l2cap_do_connect, but for rfcomm we
actually have to look in the dlcs to check witch security level should
be use.

--=20
Luiz Augusto von Dentz
Computer Engineer

2010-08-03 07:59:56

by Marcel Holtmann

[permalink] [raw]
Subject: Re: [PATCH] bluetooth: fix not setting security level when creating a rfcomm session

Hi Luiz,

> This cause 'No Bonding' to be used if userspace has not yet been paired
> with remote device since the l2cap socket used to create the rfcomm
> session does not have any security level set.
>
> Signed-off-by: Luiz Augusto von Dentz <[email protected]>
> ---
> net/bluetooth/rfcomm/core.c | 13 ++++++++++---
> 1 files changed, 10 insertions(+), 3 deletions(-)
>
> diff --git a/net/bluetooth/rfcomm/core.c b/net/bluetooth/rfcomm/core.c
> index 7dca91b..08f5757 100644
> --- a/net/bluetooth/rfcomm/core.c
> +++ b/net/bluetooth/rfcomm/core.c
> @@ -79,7 +79,10 @@ static void rfcomm_make_uih(struct sk_buff *skb, u8 addr);
>
> static void rfcomm_process_connect(struct rfcomm_session *s);
>
> -static struct rfcomm_session *rfcomm_session_create(bdaddr_t *src, bdaddr_t *dst, int *err);
> +static struct rfcomm_session *rfcomm_session_create(bdaddr_t *src,
> + bdaddr_t *dst,
> + u8 sec_level,
> + int *err);
> static struct rfcomm_session *rfcomm_session_get(bdaddr_t *src, bdaddr_t *dst);
> static void rfcomm_session_del(struct rfcomm_session *s);
>
> @@ -402,7 +405,7 @@ static int __rfcomm_dlc_open(struct rfcomm_dlc *d, bdaddr_t *src, bdaddr_t *dst,
>
> s = rfcomm_session_get(src, dst);
> if (!s) {
> - s = rfcomm_session_create(src, dst, &err);
> + s = rfcomm_session_create(src, dst, d->sec_level, &err);
> if (!s)
> return err;
> }
> @@ -680,7 +683,10 @@ static void rfcomm_session_close(struct rfcomm_session *s, int err)
> rfcomm_session_put(s);
> }
>
> -static struct rfcomm_session *rfcomm_session_create(bdaddr_t *src, bdaddr_t *dst, int *err)
> +static struct rfcomm_session *rfcomm_session_create(bdaddr_t *src,
> + bdaddr_t *dst,
> + u8 sec_level,
> + int *err)
> {
> struct rfcomm_session *s = NULL;
> struct sockaddr_l2 addr;
> @@ -705,6 +711,7 @@ static struct rfcomm_session *rfcomm_session_create(bdaddr_t *src, bdaddr_t *dst
> sk = sock->sk;
> lock_sock(sk);
> l2cap_pi(sk)->imtu = l2cap_mtu;
> + l2cap_pi(sk)->sec_level = sec_level;
> if (l2cap_ertm)
> l2cap_pi(sk)->mode = L2CAP_MODE_ERTM;
> release_sock(sk);

I see what you are trying to do here, but why is this needed? Shouldn't
it be already correctly set for PSM 3?

Regards

Marcel



2010-10-28 13:07:08

by Gustavo Padovan

[permalink] [raw]
Subject: Re: [PATCH] bluetooth: fix not setting security level when creating a rfcomm session

Hi Luiz,

* Luiz Augusto von Dentz <[email protected]> [2010-08-19 14:06:10 +0300]:

> From: Luiz Augusto von Dentz <[email protected]>
>
> This cause 'No Bonding' to be used if userspace has not yet been paired
> with remote device since the l2cap socket used to create the rfcomm
> session does not have any security level set.
>
> Signed-off-by: Luiz Augusto von Dentz <[email protected]>
> ---
> net/bluetooth/rfcomm/core.c | 13 ++++++++++---
> 1 files changed, 10 insertions(+), 3 deletions(-)

Patch has been applied to the bluetooth-2.6 tree. Thanks.

--
Gustavo F. Padovan
ProFUSION embedded systems - http://profusion.mobi