Bluetooth: Fix l2cap conn failures for ssp devices

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 <[email protected]>

Hi Peter,

Quoted 11 lines · Peter Hurley

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 <[email protected]>

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 :)

Hi Johan,

Quoted 27 lines · Daniel Wagner

Hi Peter,

Quoted 11 lines · Peter Hurley

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 <[email protected]>

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.

Quoted 2 lines

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.

Hi Daniel,

Quoted 30 lines · Daniel Wagner

Hi Johan,

Quoted 27 lines · Daniel Wagner

Hi Peter,

Quoted 11 lines · Peter Hurley

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 <[email protected]>

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.

Quoted 5 lines

Quoted 2 lines

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.

Hi Peter,

Quoted 12 lines · Peter Hurley

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.

Quoted 7 lines

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"?

Quoted 6 lines

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.

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 <[email protected]>

---

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(-)

Quoted 13 lines

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 <[email protected]>

---

Quoted 6 lines

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 <[email protected]>

Hi,

Quoted 17 lines · Marcel Holtmann

Quoted 17 lines

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 <[email protected]>

---

v2: Avoids possible race condition between hci_conn_security and hci_conn_complete_evt
v3: Actually compiles :)

Quoted 4 lines

Quoted 2 lines

 net/bluetooth/hci_conn.c |    4 ++++
 1 files changed, 4 insertions(+), 0 deletions(-)

Acked-by Marcel Holtmann <[email protected]>

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