2023-12-05 12:05:42

by Yao Xiao

[permalink] [raw]
Subject: [PATCH] Bluetooth: MGMT/SMP: Fix address type when using smp over BREDR

From: Xiao Yao <[email protected]>

When using SMP over BREDR, the identity address(irk/ltk/csrk) is
distributed during the SMP key distribution phase.

> ACL Data RX: Handle 11 flags 0x02 dlen 12
BR/EDR SMP: Identity Address Information (0x09) len 7
Address: F8:7D:76:XX:XX:XX (OUI F8-7D-76)
@ MGMT Event: New Identity Resolving Key (0x0018) plen 30
Store hint: Yes (0x01)
Random address: 00:00:00:00:00:00 (Non-Resolvable)
BR/EDR Address: F8:7D:76:XX:XX:XX (OUI F8-7D-76)
Key: 30cac11ec2bbc046a3658f62xxxxxxxx
@ MGMT Event: New Long Term Key (0x000a) plen 37
Store hint: Yes (0x01)
LE Address: F8:7D:76:XX:XX:XX (OUI F8-7D-76)
Key type: Authenticated key from P-256 (0x03)
Central: 0x00
Encryption size: 16
Diversifier: 0000
Randomizer: 0000000000000000
Key: a3ca3f9e06cfa8c512edc13axxxxxxxx

In the mgmt_new_irk/ltk/csrk() function, addr.type is forcefully converted
to BDADDR_LE_PUBLIC using link_to_bdaddr(LE_LINK, irk/ltk/csrk->addr_type).
However, the actual address type should be BDADDR_BREDR. Therefore, let's
pass the actual link type to determine the address type.

Signed-off-by: Xiao Yao <[email protected]>
---
include/net/bluetooth/hci_core.h | 8 +++++---
net/bluetooth/mgmt.c | 14 ++++++++------
net/bluetooth/smp.c | 10 +++++-----
3 files changed, 18 insertions(+), 14 deletions(-)

diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
index eed6c9f37b12..5088fbf4c7f5 100644
--- a/include/net/bluetooth/hci_core.h
+++ b/include/net/bluetooth/hci_core.h
@@ -2278,10 +2278,12 @@ void mgmt_suspending(struct hci_dev *hdev, u8 state);
void mgmt_resuming(struct hci_dev *hdev, u8 reason, bdaddr_t *bdaddr,
u8 addr_type);
bool mgmt_powering_down(struct hci_dev *hdev);
-void mgmt_new_ltk(struct hci_dev *hdev, struct smp_ltk *key, bool persistent);
-void mgmt_new_irk(struct hci_dev *hdev, struct smp_irk *irk, bool persistent);
+void mgmt_new_ltk(struct hci_dev *hdev, struct smp_ltk *key, bool persistent,
+ u8 link_type);
+void mgmt_new_irk(struct hci_dev *hdev, struct smp_irk *irk, bool persistent,
+ u8 link_type);
void mgmt_new_csrk(struct hci_dev *hdev, struct smp_csrk *csrk,
- bool persistent);
+ bool persistent, u8 link_type);
void mgmt_new_conn_param(struct hci_dev *hdev, bdaddr_t *bdaddr,
u8 bdaddr_type, u8 store_hint, u16 min_interval,
u16 max_interval, u16 latency, u16 timeout);
diff --git a/net/bluetooth/mgmt.c b/net/bluetooth/mgmt.c
index da79a2369dd7..fdfb395e29ba 100644
--- a/net/bluetooth/mgmt.c
+++ b/net/bluetooth/mgmt.c
@@ -9550,7 +9550,8 @@ static u8 mgmt_ltk_type(struct smp_ltk *ltk)
return MGMT_LTK_UNAUTHENTICATED;
}

-void mgmt_new_ltk(struct hci_dev *hdev, struct smp_ltk *key, bool persistent)
+void mgmt_new_ltk(struct hci_dev *hdev, struct smp_ltk *key, bool persistent,
+ u8 link_type)
{
struct mgmt_ev_new_long_term_key ev;

@@ -9574,7 +9575,7 @@ void mgmt_new_ltk(struct hci_dev *hdev, struct smp_ltk *key, bool persistent)
ev.store_hint = persistent;

bacpy(&ev.key.addr.bdaddr, &key->bdaddr);
- ev.key.addr.type = link_to_bdaddr(LE_LINK, key->bdaddr_type);
+ ev.key.addr.type = link_to_bdaddr(link_type, key->bdaddr_type);
ev.key.type = mgmt_ltk_type(key);
ev.key.enc_size = key->enc_size;
ev.key.ediv = key->ediv;
@@ -9593,7 +9594,8 @@ void mgmt_new_ltk(struct hci_dev *hdev, struct smp_ltk *key, bool persistent)
mgmt_event(MGMT_EV_NEW_LONG_TERM_KEY, hdev, &ev, sizeof(ev), NULL);
}

-void mgmt_new_irk(struct hci_dev *hdev, struct smp_irk *irk, bool persistent)
+void mgmt_new_irk(struct hci_dev *hdev, struct smp_irk *irk, bool persistent,
+ u8 link_type)
{
struct mgmt_ev_new_irk ev;

@@ -9603,14 +9605,14 @@ void mgmt_new_irk(struct hci_dev *hdev, struct smp_irk *irk, bool persistent)

bacpy(&ev.rpa, &irk->rpa);
bacpy(&ev.irk.addr.bdaddr, &irk->bdaddr);
- ev.irk.addr.type = link_to_bdaddr(LE_LINK, irk->addr_type);
+ ev.irk.addr.type = link_to_bdaddr(link_type, irk->addr_type);
memcpy(ev.irk.val, irk->val, sizeof(irk->val));

mgmt_event(MGMT_EV_NEW_IRK, hdev, &ev, sizeof(ev), NULL);
}

void mgmt_new_csrk(struct hci_dev *hdev, struct smp_csrk *csrk,
- bool persistent)
+ bool persistent, u8 link_type)
{
struct mgmt_ev_new_csrk ev;

@@ -9632,7 +9634,7 @@ void mgmt_new_csrk(struct hci_dev *hdev, struct smp_csrk *csrk,
ev.store_hint = persistent;

bacpy(&ev.key.addr.bdaddr, &csrk->bdaddr);
- ev.key.addr.type = link_to_bdaddr(LE_LINK, csrk->bdaddr_type);
+ ev.key.addr.type = link_to_bdaddr(link_type, csrk->bdaddr_type);
ev.key.type = csrk->type;
memcpy(ev.key.val, csrk->val, sizeof(csrk->val));

diff --git a/net/bluetooth/smp.c b/net/bluetooth/smp.c
index f1a9fc0012f0..3f640741e07b 100644
--- a/net/bluetooth/smp.c
+++ b/net/bluetooth/smp.c
@@ -1060,7 +1060,7 @@ static void smp_notify_keys(struct l2cap_conn *conn)
}

if (smp->remote_irk) {
- mgmt_new_irk(hdev, smp->remote_irk, persistent);
+ mgmt_new_irk(hdev, smp->remote_irk, persistent, hcon->type);

/* Now that user space can be considered to know the
* identity address track the connection based on it
@@ -1081,25 +1081,25 @@ static void smp_notify_keys(struct l2cap_conn *conn)
if (smp->csrk) {
smp->csrk->bdaddr_type = hcon->dst_type;
bacpy(&smp->csrk->bdaddr, &hcon->dst);
- mgmt_new_csrk(hdev, smp->csrk, persistent);
+ mgmt_new_csrk(hdev, smp->csrk, persistent, hcon->type);
}

if (smp->responder_csrk) {
smp->responder_csrk->bdaddr_type = hcon->dst_type;
bacpy(&smp->responder_csrk->bdaddr, &hcon->dst);
- mgmt_new_csrk(hdev, smp->responder_csrk, persistent);
+ mgmt_new_csrk(hdev, smp->responder_csrk, persistent, hcon->type);
}

if (smp->ltk) {
smp->ltk->bdaddr_type = hcon->dst_type;
bacpy(&smp->ltk->bdaddr, &hcon->dst);
- mgmt_new_ltk(hdev, smp->ltk, persistent);
+ mgmt_new_ltk(hdev, smp->ltk, persistent, hcon->type);
}

if (smp->responder_ltk) {
smp->responder_ltk->bdaddr_type = hcon->dst_type;
bacpy(&smp->responder_ltk->bdaddr, &hcon->dst);
- mgmt_new_ltk(hdev, smp->responder_ltk, persistent);
+ mgmt_new_ltk(hdev, smp->responder_ltk, persistent, hcon->type);
}

if (smp->link_key) {
--
2.34.1



2023-12-05 12:44:04

by bluez.test.bot

[permalink] [raw]
Subject: RE: Bluetooth: MGMT/SMP: Fix address type when using smp over BREDR

This is automated email and please do not reply to this email!

Dear submitter,

Thank you for submitting the patches to the linux bluetooth mailing list.
This is a CI test results with your patch series:
PW Link:https://patchwork.kernel.org/project/bluetooth/list/?series=807009

---Test result---

Test Summary:
CheckPatch PASS 0.77 seconds
GitLint FAIL 1.58 seconds
SubjectPrefix PASS 0.16 seconds
BuildKernel PASS 27.52 seconds
CheckAllWarning PASS 30.08 seconds
CheckSparse PASS 35.30 seconds
CheckSmatch PASS 97.90 seconds
BuildKernel32 PASS 27.38 seconds
TestRunnerSetup PASS 417.74 seconds
TestRunner_l2cap-tester PASS 22.78 seconds
TestRunner_iso-tester PASS 50.71 seconds
TestRunner_bnep-tester PASS 6.93 seconds
TestRunner_mgmt-tester PASS 163.21 seconds
TestRunner_rfcomm-tester PASS 10.73 seconds
TestRunner_sco-tester PASS 14.48 seconds
TestRunner_ioctl-tester PASS 12.04 seconds
TestRunner_mesh-tester PASS 8.76 seconds
TestRunner_smp-tester PASS 9.73 seconds
TestRunner_userchan-tester PASS 7.19 seconds
IncrementalBuild PASS 25.45 seconds

Details
##############################
Test: GitLint - FAIL
Desc: Run gitlint
Output:
Bluetooth: MGMT/SMP: Fix address type when using smp over BREDR

WARNING: I3 - ignore-body-lines: gitlint will be switching from using Python regex 'match' (match beginning) to 'search' (match anywhere) semantics. Please review your ignore-body-lines.regex option accordingly. To remove this warning, set general.regex-style-search=True. More details: https://jorisroovers.github.io/gitlint/configuration/#regex-style-search
9: B3 Line contains hard tab characters (\t): " BR/EDR SMP: Identity Address Information (0x09) len 7"
10: B3 Line contains hard tab characters (\t): " Address: F8:7D:76:XX:XX:XX (OUI F8-7D-76)"
12: B3 Line contains hard tab characters (\t): " Store hint: Yes (0x01)"
13: B3 Line contains hard tab characters (\t): " Random address: 00:00:00:00:00:00 (Non-Resolvable)"
14: B3 Line contains hard tab characters (\t): " BR/EDR Address: F8:7D:76:XX:XX:XX (OUI F8-7D-76)"
15: B3 Line contains hard tab characters (\t): " Key: 30cac11ec2bbc046a3658f62xxxxxxxx"


---
Regards,
Linux Bluetooth

2023-12-05 18:41:35

by Luiz Augusto von Dentz

[permalink] [raw]
Subject: Re: [PATCH] Bluetooth: MGMT/SMP: Fix address type when using smp over BREDR

Hi,

On Tue, Dec 5, 2023 at 7:05 AM Xiao Yao <[email protected]> wrote:
>
> From: Xiao Yao <[email protected]>
>
> When using SMP over BREDR, the identity address(irk/ltk/csrk) is
> distributed during the SMP key distribution phase.
>
> > ACL Data RX: Handle 11 flags 0x02 dlen 12
> BR/EDR SMP: Identity Address Information (0x09) len 7
> Address: F8:7D:76:XX:XX:XX (OUI F8-7D-76)
> @ MGMT Event: New Identity Resolving Key (0x0018) plen 30
> Store hint: Yes (0x01)
> Random address: 00:00:00:00:00:00 (Non-Resolvable)
> BR/EDR Address: F8:7D:76:XX:XX:XX (OUI F8-7D-76)
> Key: 30cac11ec2bbc046a3658f62xxxxxxxx
> @ MGMT Event: New Long Term Key (0x000a) plen 37
> Store hint: Yes (0x01)
> LE Address: F8:7D:76:XX:XX:XX (OUI F8-7D-76)

So I assume the above should change to BR/EDR after applying these
changes? It probably make sense to have the trace of before and after
just to be clearer.

> Key type: Authenticated key from P-256 (0x03)
> Central: 0x00
> Encryption size: 16
> Diversifier: 0000
> Randomizer: 0000000000000000
> Key: a3ca3f9e06cfa8c512edc13axxxxxxxx
>
> In the mgmt_new_irk/ltk/csrk() function, addr.type is forcefully converted
> to BDADDR_LE_PUBLIC using link_to_bdaddr(LE_LINK, irk/ltk/csrk->addr_type).
> However, the actual address type should be BDADDR_BREDR. Therefore, let's
> pass the actual link type to determine the address type.
>
> Signed-off-by: Xiao Yao <[email protected]>
> ---
> include/net/bluetooth/hci_core.h | 8 +++++---
> net/bluetooth/mgmt.c | 14 ++++++++------
> net/bluetooth/smp.c | 10 +++++-----
> 3 files changed, 18 insertions(+), 14 deletions(-)
>
> diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
> index eed6c9f37b12..5088fbf4c7f5 100644
> --- a/include/net/bluetooth/hci_core.h
> +++ b/include/net/bluetooth/hci_core.h
> @@ -2278,10 +2278,12 @@ void mgmt_suspending(struct hci_dev *hdev, u8 state);
> void mgmt_resuming(struct hci_dev *hdev, u8 reason, bdaddr_t *bdaddr,
> u8 addr_type);
> bool mgmt_powering_down(struct hci_dev *hdev);
> -void mgmt_new_ltk(struct hci_dev *hdev, struct smp_ltk *key, bool persistent);
> -void mgmt_new_irk(struct hci_dev *hdev, struct smp_irk *irk, bool persistent);
> +void mgmt_new_ltk(struct hci_dev *hdev, struct smp_ltk *key, bool persistent,
> + u8 link_type);
> +void mgmt_new_irk(struct hci_dev *hdev, struct smp_irk *irk, bool persistent,
> + u8 link_type);
> void mgmt_new_csrk(struct hci_dev *hdev, struct smp_csrk *csrk,
> - bool persistent);
> + bool persistent, u8 link_type);
> void mgmt_new_conn_param(struct hci_dev *hdev, bdaddr_t *bdaddr,
> u8 bdaddr_type, u8 store_hint, u16 min_interval,
> u16 max_interval, u16 latency, u16 timeout);
> diff --git a/net/bluetooth/mgmt.c b/net/bluetooth/mgmt.c
> index da79a2369dd7..fdfb395e29ba 100644
> --- a/net/bluetooth/mgmt.c
> +++ b/net/bluetooth/mgmt.c
> @@ -9550,7 +9550,8 @@ static u8 mgmt_ltk_type(struct smp_ltk *ltk)
> return MGMT_LTK_UNAUTHENTICATED;
> }
>
> -void mgmt_new_ltk(struct hci_dev *hdev, struct smp_ltk *key, bool persistent)
> +void mgmt_new_ltk(struct hci_dev *hdev, struct smp_ltk *key, bool persistent,
> + u8 link_type)
> {
> struct mgmt_ev_new_long_term_key ev;
>
> @@ -9574,7 +9575,7 @@ void mgmt_new_ltk(struct hci_dev *hdev, struct smp_ltk *key, bool persistent)
> ev.store_hint = persistent;
>
> bacpy(&ev.key.addr.bdaddr, &key->bdaddr);
> - ev.key.addr.type = link_to_bdaddr(LE_LINK, key->bdaddr_type);
> + ev.key.addr.type = link_to_bdaddr(link_type, key->bdaddr_type);
> ev.key.type = mgmt_ltk_type(key);
> ev.key.enc_size = key->enc_size;
> ev.key.ediv = key->ediv;
> @@ -9593,7 +9594,8 @@ void mgmt_new_ltk(struct hci_dev *hdev, struct smp_ltk *key, bool persistent)
> mgmt_event(MGMT_EV_NEW_LONG_TERM_KEY, hdev, &ev, sizeof(ev), NULL);
> }
>
> -void mgmt_new_irk(struct hci_dev *hdev, struct smp_irk *irk, bool persistent)
> +void mgmt_new_irk(struct hci_dev *hdev, struct smp_irk *irk, bool persistent,
> + u8 link_type)
> {
> struct mgmt_ev_new_irk ev;
>
> @@ -9603,14 +9605,14 @@ void mgmt_new_irk(struct hci_dev *hdev, struct smp_irk *irk, bool persistent)
>
> bacpy(&ev.rpa, &irk->rpa);
> bacpy(&ev.irk.addr.bdaddr, &irk->bdaddr);
> - ev.irk.addr.type = link_to_bdaddr(LE_LINK, irk->addr_type);
> + ev.irk.addr.type = link_to_bdaddr(link_type, irk->addr_type);

Perhaps we should incorporate the link_type as part of irk, ltk, csrk,
etc, to guarantee this information is always available.

> memcpy(ev.irk.val, irk->val, sizeof(irk->val));
>
> mgmt_event(MGMT_EV_NEW_IRK, hdev, &ev, sizeof(ev), NULL);
> }
>
> void mgmt_new_csrk(struct hci_dev *hdev, struct smp_csrk *csrk,
> - bool persistent)
> + bool persistent, u8 link_type)
> {
> struct mgmt_ev_new_csrk ev;
>
> @@ -9632,7 +9634,7 @@ void mgmt_new_csrk(struct hci_dev *hdev, struct smp_csrk *csrk,
> ev.store_hint = persistent;
>
> bacpy(&ev.key.addr.bdaddr, &csrk->bdaddr);
> - ev.key.addr.type = link_to_bdaddr(LE_LINK, csrk->bdaddr_type);
> + ev.key.addr.type = link_to_bdaddr(link_type, csrk->bdaddr_type);
> ev.key.type = csrk->type;
> memcpy(ev.key.val, csrk->val, sizeof(csrk->val));
>
> diff --git a/net/bluetooth/smp.c b/net/bluetooth/smp.c
> index f1a9fc0012f0..3f640741e07b 100644
> --- a/net/bluetooth/smp.c
> +++ b/net/bluetooth/smp.c
> @@ -1060,7 +1060,7 @@ static void smp_notify_keys(struct l2cap_conn *conn)
> }
>
> if (smp->remote_irk) {
> - mgmt_new_irk(hdev, smp->remote_irk, persistent);
> + mgmt_new_irk(hdev, smp->remote_irk, persistent, hcon->type);
>
> /* Now that user space can be considered to know the
> * identity address track the connection based on it
> @@ -1081,25 +1081,25 @@ static void smp_notify_keys(struct l2cap_conn *conn)
> if (smp->csrk) {
> smp->csrk->bdaddr_type = hcon->dst_type;
> bacpy(&smp->csrk->bdaddr, &hcon->dst);
> - mgmt_new_csrk(hdev, smp->csrk, persistent);
> + mgmt_new_csrk(hdev, smp->csrk, persistent, hcon->type);
> }
>
> if (smp->responder_csrk) {
> smp->responder_csrk->bdaddr_type = hcon->dst_type;
> bacpy(&smp->responder_csrk->bdaddr, &hcon->dst);
> - mgmt_new_csrk(hdev, smp->responder_csrk, persistent);
> + mgmt_new_csrk(hdev, smp->responder_csrk, persistent, hcon->type);
> }
>
> if (smp->ltk) {
> smp->ltk->bdaddr_type = hcon->dst_type;
> bacpy(&smp->ltk->bdaddr, &hcon->dst);
> - mgmt_new_ltk(hdev, smp->ltk, persistent);
> + mgmt_new_ltk(hdev, smp->ltk, persistent, hcon->type);
> }
>
> if (smp->responder_ltk) {
> smp->responder_ltk->bdaddr_type = hcon->dst_type;
> bacpy(&smp->responder_ltk->bdaddr, &hcon->dst);
> - mgmt_new_ltk(hdev, smp->responder_ltk, persistent);
> + mgmt_new_ltk(hdev, smp->responder_ltk, persistent, hcon->type);
> }
>
> if (smp->link_key) {
> --
> 2.34.1
>
>


--
Luiz Augusto von Dentz