[PATCH v3] Bluetooth: Fix l2cap conn failures for ssp devices

[PATCH v3] Bluetooth: Fix l2cap conn failures for ssp devices

[Peter Hurley]

Q29tbWl0IDMzMDYwNTQyM2MgZml4ZWQgbDJjYXAgY29ubiBlc3RhYmxpc2htZW50IGZvciBub24t
c3NwIHJlbW90ZQ0KZGV2aWNlcyBieSBub3Qgc2V0dGluZyBIQ0lfQ09OTl9FTkNSWVBUX1BFTkQg
ZXZlcnkgdGltZSBjb25uIHNlY3VyaXR5DQppcyB0ZXN0ZWQgKHdoaWNoIHdhcyBhbHdheXMgcmV0
dXJuaW5nIGZhaWx1cmUgb24gYW55IHN1YnNlcXVlbnQNCnNlY3VyaXR5IGNoZWNrcykuDQoNCkhv
d2V2ZXIsIHRoaXMgYnJva2UgbDJjYXAgY29ubiBlc3RhYmxpc2htZW50IGZvciBzc3AgcmVtb3Rl
IGRldmljZXMNCndoZW4gYW4gQUNMIGxpbmsgd2FzIGFscmVhZHkgZXN0YWJsaXNoZWQgYXQgU0RQ
LWxldmVsIHNlY3VyaXR5LiBUaGlzDQpmaXggZW5zdXJlcyB0aGF0IGVuY3J5cHRpb24gbXVzdCBi
ZSBwZW5kaW5nIHdoZW5ldmVyIGF1dGhlbnRpY2F0aW9uDQppcyBhbHNvIHBlbmRpbmcuDQoNClNp
Z25lZC1vZmYtYnk6IFBldGVyIEh1cmxleSA8cGV0ZXJAaHVybGV5c29mdHdhcmUuY29tPg0KLS0t
DQoNCnYyOiBBdm9pZHMgcG9zc2libGUgcmFjZSBjb25kaXRpb24gYmV0d2VlbiBoY2lfY29ubl9z
ZWN1cml0eSBhbmQNCmhjaV9jb25uX2NvbXBsZXRlX2V2dA0KdjM6IEFjdHVhbGx5IGNvbXBpbGVz
IDopDQoNCiBuZXQvYmx1ZXRvb3RoL2hjaV9jb25uLmMgfCAgICA0ICsrKysNCiAxIGZpbGVzIGNo
YW5nZWQsIDQgaW5zZXJ0aW9ucygrKSwgMCBkZWxldGlvbnMoLSkNCg0KZGlmZiAtLWdpdCBhL25l
dC9ibHVldG9vdGgvaGNpX2Nvbm4uYyBiL25ldC9ibHVldG9vdGgvaGNpX2Nvbm4uYw0KaW5kZXgg
NTMwZjkwMS4uZTMzMzI3NCAxMDA2NDQNCi0tLSBhL25ldC9ibHVldG9vdGgvaGNpX2Nvbm4uYw0K
KysrIGIvbmV0L2JsdWV0b290aC9oY2lfY29ubi5jDQpAQCAtNTUzLDYgKzU1MywxMCBAQCBzdGF0
aWMgaW50IGhjaV9jb25uX2F1dGgoc3RydWN0IGhjaV9jb25uICpjb25uLCBfX3U4IHNlY19sZXZl
bCwgX191OCBhdXRoX3R5cGUpDQogDQogCWlmICghdGVzdF9hbmRfc2V0X2JpdChIQ0lfQ09OTl9B
VVRIX1BFTkQsICZjb25uLT5wZW5kKSkgew0KIAkJc3RydWN0IGhjaV9jcF9hdXRoX3JlcXVlc3Rl
ZCBjcDsNCisNCisJCS8qIGVuY3J5cHQgbXVzdCBiZSBwZW5kaW5nIGlmIGF1dGggaXMgYWxzbyBw
ZW5kaW5nICovDQorCQlzZXRfYml0KEhDSV9DT05OX0VOQ1JZUFRfUEVORCwgJmNvbm4tPnBlbmQp
Ow0KKw0KIAkJY3AuaGFuZGxlID0gY3B1X3RvX2xlMTYoY29ubi0+aGFuZGxlKTsNCiAJCWhjaV9z
ZW5kX2NtZChjb25uLT5oZGV2LCBIQ0lfT1BfQVVUSF9SRVFVRVNURUQsDQogCQkJCQkJCXNpemVv
ZihjcCksICZjcCk7DQotLSANCjEuNy40LjENCg0K

Re: [PATCH v3] Bluetooth: Fix l2cap conn failures for ssp devices

[Daniel Wagner]

Hi Peter,

On 09.08.2011 22:26, Peter Hurley wrote:
> Commit 330605423c fixed l2cap conn establishment for non-ssp remote
> devices by not setting HCI_CONN_ENCRYPT_PEND every time conn security
> is tested (which was always returning failure on any subsequent
> security checks).
> 
> However, this broke l2cap conn establishment for ssp remote devices
> when an ACL link was already established at SDP-level security. This
> fix ensures that encryption must be pending whenever authentication
> is also pending.
> 
> Signed-off-by: Peter Hurley <peter@hurleysoftware.com>

git am complaines:

Applying: Bluetooth: Fix l2cap conn failures for ssp devices
/home/wagi/src/bluetooth-next/.git/rebase-apply/patch:18: trailing whitespace.

/home/wagi/src/bluetooth-next/.git/rebase-apply/patch:19: trailing whitespace.
                /* encrypt must be pending if auth is also pending */
/home/wagi/src/bluetooth-next/.git/rebase-apply/patch:20: trailing whitespace.
                set_bit(HCI_CONN_ENCRYPT_PEND, &conn->pend);
/home/wagi/src/bluetooth-next/.git/rebase-apply/patch:21: trailing whitespace.


But it fixes the problem I was debugging. From what I have
learned in the last two days I would say it is also the right fix :)

Tested-by: Daniel Wagner <daniel.wagner@bmw-carit.de>

cheers,
daniel

Re: [PATCH v3] Bluetooth: Fix l2cap conn failures for ssp devices

[Daniel Wagner]

Hi Johan,

On 04.01.2012 11:22, Daniel Wagner wrote:
> Hi Peter,
> 
> On 09.08.2011 22:26, Peter Hurley wrote:
>> Commit 330605423c fixed l2cap conn establishment for non-ssp remote
>> devices by not setting HCI_CONN_ENCRYPT_PEND every time conn security
>> is tested (which was always returning failure on any subsequent
>> security checks).
>>
>> However, this broke l2cap conn establishment for ssp remote devices
>> when an ACL link was already established at SDP-level security. This
>> fix ensures that encryption must be pending whenever authentication
>> is also pending.
>>
>> Signed-off-by: Peter Hurley <peter@hurleysoftware.com>
> 
> git am complaines:
> 
> Applying: Bluetooth: Fix l2cap conn failures for ssp devices
> /home/wagi/src/bluetooth-next/.git/rebase-apply/patch:18: trailing whitespace.
> 
> /home/wagi/src/bluetooth-next/.git/rebase-apply/patch:19: trailing whitespace.
>                 /* encrypt must be pending if auth is also pending */
> /home/wagi/src/bluetooth-next/.git/rebase-apply/patch:20: trailing whitespace.
>                 set_bit(HCI_CONN_ENCRYPT_PEND, &conn->pend);
> /home/wagi/src/bluetooth-next/.git/rebase-apply/patch:21: trailing whitespace.
> 
> 
> But it fixes the problem I was debugging. From what I have
> learned in the last two days I would say it is also the right fix :)

Is there any chance to get this patch in? If it helps I could send a
whitespace fixed version.

cheers,
daniel

Re: [PATCH v3] Bluetooth: Fix l2cap conn failures for ssp devices

[Johan Hedberg]

Hi Daniel,

On Mon, Jan 09, 2012, Daniel Wagner wrote:
> Hi Johan,
> 
> On 04.01.2012 11:22, Daniel Wagner wrote:
> > Hi Peter,
> > 
> > On 09.08.2011 22:26, Peter Hurley wrote:
> >> Commit 330605423c fixed l2cap conn establishment for non-ssp remote
> >> devices by not setting HCI_CONN_ENCRYPT_PEND every time conn security
> >> is tested (which was always returning failure on any subsequent
> >> security checks).
> >>
> >> However, this broke l2cap conn establishment for ssp remote devices
> >> when an ACL link was already established at SDP-level security. This
> >> fix ensures that encryption must be pending whenever authentication
> >> is also pending.
> >>
> >> Signed-off-by: Peter Hurley <peter@hurleysoftware.com>
> > 
> > git am complaines:
> > 
> > Applying: Bluetooth: Fix l2cap conn failures for ssp devices
> > /home/wagi/src/bluetooth-next/.git/rebase-apply/patch:18: trailing whitespace.
> > 
> > /home/wagi/src/bluetooth-next/.git/rebase-apply/patch:19: trailing whitespace.
> >                 /* encrypt must be pending if auth is also pending */
> > /home/wagi/src/bluetooth-next/.git/rebase-apply/patch:20: trailing whitespace.
> >                 set_bit(HCI_CONN_ENCRYPT_PEND, &conn->pend);
> > /home/wagi/src/bluetooth-next/.git/rebase-apply/patch:21: trailing whitespace.
> > 
> > 
> > But it fixes the problem I was debugging. From what I have
> > learned in the last two days I would say it is also the right fix :)
> 
> Is there any chance to get this patch in? If it helps I could send a
> whitespace fixed version.

Feel free to send a whitespace-fixed version, but even then I only apply
patches with Marcel's ack on them. Btw, have you verified that this
doesn't break non-SSP cases? Is this how the code used to behave before
the regression?

Johan

Re: [PATCH v3] Bluetooth: Fix l2cap conn failures for ssp devices

[Peter Hurley]

Hi Daniel & Johan,

On Wed, 2012-01-11 at 06:26 -0500, Johan Hedberg wrote:
> Hi Daniel,
> 
> On Mon, Jan 09, 2012, Daniel Wagner wrote:
> > Hi Johan,
> > 
> > On 04.01.2012 11:22, Daniel Wagner wrote:
> > > Hi Peter,
> > > 
> > > On 09.08.2011 22:26, Peter Hurley wrote:
> > >> Commit 330605423c fixed l2cap conn establishment for non-ssp remote
> > >> devices by not setting HCI_CONN_ENCRYPT_PEND every time conn security
> > >> is tested (which was always returning failure on any subsequent
> > >> security checks).
> > >>
> > >> However, this broke l2cap conn establishment for ssp remote devices
> > >> when an ACL link was already established at SDP-level security. This
> > >> fix ensures that encryption must be pending whenever authentication
> > >> is also pending.
> > >>
> > >> Signed-off-by: Peter Hurley <peter@hurleysoftware.com>
> > > 
> > > git am complaines:
> > > 
> > > Applying: Bluetooth: Fix l2cap conn failures for ssp devices
> > > /home/wagi/src/bluetooth-next/.git/rebase-apply/patch:18: trailing whitespace.
> > > 
> > > /home/wagi/src/bluetooth-next/.git/rebase-apply/patch:19: trailing whitespace.
> > >                 /* encrypt must be pending if auth is also pending */
> > > /home/wagi/src/bluetooth-next/.git/rebase-apply/patch:20: trailing whitespace.
> > >                 set_bit(HCI_CONN_ENCRYPT_PEND, &conn->pend);
> > > /home/wagi/src/bluetooth-next/.git/rebase-apply/patch:21: trailing whitespace.
> > > 
> > > 
> > > But it fixes the problem I was debugging. From what I have
> > > learned in the last two days I would say it is also the right fix :)
> > 
> > Is there any chance to get this patch in? If it helps I could send a
> > whitespace fixed version.
> 
> Feel free to send a whitespace-fixed version, but even then I only apply
> patches with Marcel's ack on them. Btw, have you verified that this
> doesn't break non-SSP cases? Is this how the code used to behave before
> the regression?
> 
> Johan

The situation with this patch is that I asked Gustavo not to apply it
(via IRC). Although the patch works, the overall logic of the
auth/encryption system is flawed.

With respect to this issue specifically (ie, ssp vs. non-ssp auth +
encrypt), the flaw is that the semantic meaning of the
HCI_CONN_ENCRYPT_PEND bit is overloaded. The meaning of one sense is
that an actual HCI_OP_SET_CONN_ENCRYPT command is in-flight, and thus,
for this hci connection, neither an auth nor encrypt request should be
sent. The second meaning is that the event handler *should* submit an
encrypt request upon receiving a successful auth complete event.

At the time, I had a patch prepared which addressed this duality as part
of a series which enabled true re-auth & sec_level promotion.
Unfortunately, I discovered that a prior patch had been submitted and
applied which specifically disables sec_level promotion for non-ssp
devices. The ml conversation died here
http://marc.info/?l=linux-bluetooth&m=131609282919575&w=2 so I dropped
it.

Regards,
Peter Hurley

Re: [PATCH v3] Bluetooth: Fix l2cap conn failures for ssp devices

[Johan Hedberg]

Hi Peter,

On Mon, Jan 16, 2012, Peter Hurley wrote:
> The situation with this patch is that I asked Gustavo not to apply it
> (via IRC). Although the patch works, the overall logic of the
> auth/encryption system is flawed.
> 
> With respect to this issue specifically (ie, ssp vs. non-ssp auth +
> encrypt), the flaw is that the semantic meaning of the
> HCI_CONN_ENCRYPT_PEND bit is overloaded. The meaning of one sense is
> that an actual HCI_OP_SET_CONN_ENCRYPT command is in-flight, and thus,
> for this hci connection, neither an auth nor encrypt request should be
> sent. The second meaning is that the event handler *should* submit an
> encrypt request upon receiving a successful auth complete event.
> 
> At the time, I had a patch prepared which addressed this duality as part
> of a series which enabled true re-auth & sec_level promotion.
> Unfortunately, I discovered that a prior patch had been submitted and
> applied which specifically disables sec_level promotion for non-ssp
> devices. The ml conversation died here
> http://marc.info/?l=linux-bluetooth&m=131609282919575&w=2 so I dropped
> it.

I think it'd be important to continue with this work. Even for
controllers that don't allow a "security upgrade" you can still detect
the situation when you get an auth_complete without any preceding
link_key or PIN request and just drop the connection in such a case. For
legacy pairing this would only happen when going from MEDIUM to HIGH
since LOW means that you don't have a link key yet.

Btw, could you tell me the commit id of this "patch which specifically
disables sec_level promotion for non-ssp devices"?

Johan

Re: [PATCH v3] Bluetooth: Fix l2cap conn failures for ssp devices

[Peter Hurley]

Hi Johan,

On Tue, 2012-01-17 at 03:27 -0500, Johan Hedberg wrote:
> Hi Peter,
> 
> On Mon, Jan 16, 2012, Peter Hurley wrote:
> > The situation with this patch is that I asked Gustavo not to apply it
> > (via IRC). Although the patch works, the overall logic of the
> > auth/encryption system is flawed.
> > 
> > With respect to this issue specifically (ie, ssp vs. non-ssp auth +
> > encrypt), the flaw is that the semantic meaning of the
> > HCI_CONN_ENCRYPT_PEND bit is overloaded. The meaning of one sense is
> > that an actual HCI_OP_SET_CONN_ENCRYPT command is in-flight, and thus,
> > for this hci connection, neither an auth nor encrypt request should be
> > sent. The second meaning is that the event handler *should* submit an
> > encrypt request upon receiving a successful auth complete event.
> > 
> > At the time, I had a patch prepared which addressed this duality as part
> > of a series which enabled true re-auth & sec_level promotion.
> > Unfortunately, I discovered that a prior patch had been submitted and
> > applied which specifically disables sec_level promotion for non-ssp
> > devices. The ml conversation died here
> > http://marc.info/?l=linux-bluetooth&m=131609282919575&w=2 so I dropped
> > it.
> 
> I think it'd be important to continue with this work. Even for
> controllers that don't allow a "security upgrade" you can still detect
> the situation when you get an auth_complete without any preceding
> link_key or PIN request and just drop the connection in such a case. For
> legacy pairing this would only happen when going from MEDIUM to HIGH
> since LOW means that you don't have a link key yet.
> 
> Btw, could you tell me the commit id of this "patch which specifically
> disables sec_level promotion for non-ssp devices"?

19f8def Bluetooth: Fix auth_complete_evt for legacy units

Peter

Re: [PATCH v3] Bluetooth: Fix l2cap conn failures for ssp devices

[Luiz Augusto von Dentz]

Hi Peter, Johan,

On Tue, Jan 17, 2012 at 4:34 AM, Peter Hurley <peter@hurleysoftware.com> wrote:
>> I think it'd be important to continue with this work. Even for
>> controllers that don't allow a "security upgrade" you can still detect
>> the situation when you get an auth_complete without any preceding
>> link_key or PIN request and just drop the connection in such a case. For
>> legacy pairing this would only happen when going from MEDIUM to HIGH
>> since LOW means that you don't have a link key yet.

Perhaps we should apply this patch so at least we are able to connect
anything that requires service discovery before connect attempt e.g.
OBEX which IMO its a very serious issue, we can fix properly the
overload latter but leaving it as it is worst, iirc is this has been
reproducible since 3.0.

-- 
Luiz Augusto von Dentz

Re: [PATCH v3] Bluetooth: Fix l2cap conn failures for ssp devices

[Marcel Holtmann]

Hi Peter,

> Commit 330605423c fixed l2cap conn establishment for non-ssp remote
> devices by not setting HCI_CONN_ENCRYPT_PEND every time conn security
> is tested (which was always returning failure on any subsequent
> security checks).
> 
> However, this broke l2cap conn establishment for ssp remote devices
> when an ACL link was already established at SDP-level security. This
> fix ensures that encryption must be pending whenever authentication
> is also pending.
> 
> Signed-off-by: Peter Hurley <peter@hurleysoftware.com>
> ---
> 
> v2: Avoids possible race condition between hci_conn_security and
> hci_conn_complete_evt
> v3: Actually compiles :)
> 
>  net/bluetooth/hci_conn.c |    4 ++++
>  1 files changed, 4 insertions(+), 0 deletions(-)

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

Regards

Marcel

Re: [PATCH v3] Bluetooth: Fix l2cap conn failures for ssp devices

[Johan Hedberg]

Hi,

On Wed, Feb 01, 2012, Marcel Holtmann wrote:
> > Commit 330605423c fixed l2cap conn establishment for non-ssp remote
> > devices by not setting HCI_CONN_ENCRYPT_PEND every time conn security
> > is tested (which was always returning failure on any subsequent
> > security checks).
> > 
> > However, this broke l2cap conn establishment for ssp remote devices
> > when an ACL link was already established at SDP-level security. This
> > fix ensures that encryption must be pending whenever authentication
> > is also pending.
> > 
> > Signed-off-by: Peter Hurley <peter@hurleysoftware.com>
> > ---
> > 
> > v2: Avoids possible race condition between hci_conn_security and
> > hci_conn_complete_evt
> > v3: Actually compiles :)
> > 
> >  net/bluetooth/hci_conn.c |    4 ++++
> >  1 files changed, 4 insertions(+), 0 deletions(-)
> 
> Acked-by: Marcel Holtmann <marcel@holtmann.org>

The patch is now in my bluetooth-next tree after manually fixing
conn->pend to conn->flags (otherwise the patch wouldn't apply).

Johan

Re: [PATCH v3] Bluetooth: Fix l2cap conn failures for ssp devices

[Daniel Wagner]

Hi Johan

On 02.02.2012 01:26, Johan Hedberg wrote:
> Hi,
> 
> On Wed, Feb 01, 2012, Marcel Holtmann wrote:
>>> Commit 330605423c fixed l2cap conn establishment for non-ssp remote
>>> devices by not setting HCI_CONN_ENCRYPT_PEND every time conn security
>>> is tested (which was always returning failure on any subsequent
>>> security checks).
>>>
>>> However, this broke l2cap conn establishment for ssp remote devices
>>> when an ACL link was already established at SDP-level security. This
>>> fix ensures that encryption must be pending whenever authentication
>>> is also pending.
>>>
>>> Signed-off-by: Peter Hurley <peter@hurleysoftware.com>
>>> ---
>>>
>>> v2: Avoids possible race condition between hci_conn_security and
>>> hci_conn_complete_evt
>>> v3: Actually compiles :)
>>>
>>>  net/bluetooth/hci_conn.c |    4 ++++
>>>  1 files changed, 4 insertions(+), 0 deletions(-)
>>
>> Acked-by: Marcel Holtmann <marcel@holtmann.org>
> 
> The patch is now in my bluetooth-next tree after manually fixing
> conn->pend to conn->flags (otherwise the patch wouldn't apply).

You might consider to send this one to stable as well. At least 3.0
could really need this patch, since it is long time stable tree, IIRC.

cheers,
daniel