Hi Luiz, Please check the comment inline, Please correct me if I'm wrong.
On 8/26/2024 9:56 PM, Luiz Augusto von Dentz wrote:
> Hi Cheng,
>
> On Mon, Aug 26, 2024 at 3:05 AM Cheng Jiang <quic_chejiang@xxxxxxxxxxx> wrote:
>>
>> Cross-transport derived ltk/csrk/irk are reported to bluetoothd from
>> kernel with BR/EDR address type, and bluetoothd doesn't treat it as LE
>> paired/bonded. In this case, bluetoothd won't send remove bond operation
>> with LE address type to kernel when executing unpair, so SMP keys are
>> retained in kernel list. Then RPA is getting resolved since we still
>> have irk which was not deleted when unpair device is done because only
>> link key is deleted since addr type is bredr.
>>
>> What’s more, pair LE of the same address will always fail in kernel
>> for ltk existence, and send back AlreadyExists error, but device state
>> is still unpaired/unbonded in bluetoothd.
>>
>> So bluetoothd needs to consider LE paired/bonded when receiving SMP keys
>> even if they are cross-transport derived.
>> ---
>> src/adapter.c | 53 ++++++++++++++++++++++++++++++++++++++-------------
>> 1 file changed, 40 insertions(+), 13 deletions(-)
>>
>> diff --git a/src/adapter.c b/src/adapter.c
>> index 245de4456..4e5af9579 100644
>> --- a/src/adapter.c
>> +++ b/src/adapter.c
>> @@ -8647,6 +8647,7 @@ static void new_link_key_callback(uint16_t index, uint16_t length,
>> struct btd_adapter *adapter = user_data;
>> struct btd_device *device;
>> char dst[18];
>> + uint8_t addr_type;
>>
>> if (length < sizeof(*ev)) {
>> btd_error(adapter->dev_id, "Too small new link key event");
>> @@ -8666,7 +8667,13 @@ static void new_link_key_callback(uint16_t index, uint16_t length,
>> return;
>> }
>>
>> - device = btd_adapter_get_device(adapter, &addr->bdaddr, addr->type);
>> + /*
>> + * For LE public address, key here is cross-transport derived,
>> + * so consider it as BR/EDR public address.
>> + *
>> + */
>> + addr_type = addr->type == BDADDR_LE_PUBLIC ? BDADDR_BREDR : addr->type;
>> + device = btd_adapter_get_device(adapter, &addr->bdaddr, addr_type);
>> if (!device) {
>> btd_error(adapter->dev_id,
>> "Unable to get device object for %s", dst);
>> @@ -8682,7 +8689,7 @@ static void new_link_key_callback(uint16_t index, uint16_t length,
>> device_set_bonded(device, BDADDR_BREDR);
>> }
>>
>> - bonding_complete(adapter, &addr->bdaddr, addr->type, 0);
>> + bonding_complete(adapter, &addr->bdaddr, addr_type, 0);
>> }
>>
>> static void store_ltk_group(struct btd_adapter *adapter, const bdaddr_t *peer,
>> @@ -8773,6 +8780,7 @@ static void new_long_term_key_callback(uint16_t index, uint16_t length,
>> struct btd_device *device;
>> bool persistent;
>> char dst[18];
>> + uint8_t addr_type;
>>
>> if (length < sizeof(*ev)) {
>> btd_error(adapter->dev_id, "Too small long term key event");
>> @@ -8784,7 +8792,13 @@ static void new_long_term_key_callback(uint16_t index, uint16_t length,
>> DBG("hci%u new LTK for %s type %u enc_size %u",
>> adapter->dev_id, dst, ev->key.type, ev->key.enc_size);
>>
>> - device = btd_adapter_get_device(adapter, &addr->bdaddr, addr->type);
>> + /*
>> + * For BR/EDR public address, key here is cross-transport derived,
>> + * so consider it as LE public address for SMP.
>> + *
>> + */
>
> This is only the case if, an only if, the device is a dual-mode
> device, so this probably need to be checked that we don't attempt to
> do this regardless.
Only the dual-mode device supports Cross-transport derived Keys. From BLE keys to BR/EDR keys, vice versa. If
single mode device, the addr->type always is BDADDR_BREDR in link key callback. and BDADDR_LE_PUBLIC or
BDADDR_LE_RANDOM for LTK/CSRK/IRK callback.
Link Key is for BR/EDR, so we check the address type is BDADDR_LE_PUBLIC or not, if yes, treated as BR/EDR,
it means the link key is derived from BLE bearer. Otherwise, use the original addr type. LTK, CSRK, IRK are
related to BLE, so checked the address type is BDADDR_BREDR or not, if yes, treated as BLE address, it means
the BLE related key are derived from BR/EDR bearer.
The change seems not affect the current logic, could you please help to explain more what need to check? Thanks!
>
>> + addr_type = addr->type == BDADDR_BREDR ? BDADDR_LE_PUBLIC : addr->type;
>> + device = btd_adapter_get_device(adapter, &addr->bdaddr, addr_type);
>> if (!device) {
>> btd_error(adapter->dev_id,
>> "Unable to get device object for %s", dst);
>> @@ -8802,8 +8816,7 @@ static void new_long_term_key_callback(uint16_t index, uint16_t length,
>> * be persistently stored.
>> *
>> */
>> - if (addr->type == BDADDR_LE_RANDOM &&
>> - (addr->bdaddr.b[5] & 0xc0) != 0xc0)
>> + if (addr_type == BDADDR_LE_RANDOM && (addr->bdaddr.b[5] & 0xc0) != 0xc0)
>> persistent = false;
>> else
>> persistent = !!ev->store_hint;
>> @@ -8817,15 +8830,15 @@ static void new_long_term_key_callback(uint16_t index, uint16_t length,
>> rand = le64_to_cpu(key->rand);
>>
>> store_longtermkey(adapter, &key->addr.bdaddr,
>> - key->addr.type, key->val, key->central,
>> + addr_type, key->val, key->central,
>> key->type, key->enc_size, ediv, rand);
>>
>> - device_set_bonded(device, addr->type);
>> + device_set_bonded(device, addr_type);
>> }
>>
>> device_set_ltk(device, ev->key.val, ev->key.central, ev->key.enc_size);
>>
>> - bonding_complete(adapter, &addr->bdaddr, addr->type, 0);
>> + bonding_complete(adapter, &addr->bdaddr, addr_type, 0);
>> }
>>
>> static void new_csrk_callback(uint16_t index, uint16_t length,
>> @@ -8837,6 +8850,7 @@ static void new_csrk_callback(uint16_t index, uint16_t length,
>> struct btd_adapter *adapter = user_data;
>> struct btd_device *device;
>> char dst[18];
>> + uint8_t addr_type;
>>
>> if (length < sizeof(*ev)) {
>> btd_error(adapter->dev_id, "Too small CSRK event");
>> @@ -8848,7 +8862,13 @@ static void new_csrk_callback(uint16_t index, uint16_t length,
>> DBG("hci%u new CSRK for %s type %u", adapter->dev_id, dst,
>> ev->key.type);
>>
>> - device = btd_adapter_get_device(adapter, &addr->bdaddr, addr->type);
>> + /*
>> + * For BR/EDR public address, key here is cross-transport derived,
>> + * so consider it as LE public address for SMP.
>> + *
>> + */
>> + addr_type = addr->type == BDADDR_BREDR ? BDADDR_LE_PUBLIC : addr->type;
>
> Ditto.
>
>> + device = btd_adapter_get_device(adapter, &addr->bdaddr, addr_type);
>> if (!device) {
>> btd_error(adapter->dev_id,
>> "Unable to get device object for %s", dst);
>> @@ -8911,6 +8931,7 @@ static void new_irk_callback(uint16_t index, uint16_t length,
>> struct btd_device *device, *duplicate;
>> bool persistent;
>> char dst[18], rpa[18];
>> + uint8_t addr_type;
>>
>> if (length < sizeof(*ev)) {
>> btd_error(adapter->dev_id, "Too small New IRK event");
>> @@ -8922,16 +8943,22 @@ static void new_irk_callback(uint16_t index, uint16_t length,
>>
>> DBG("hci%u new IRK for %s RPA %s", adapter->dev_id, dst, rpa);
>>
>> + /*
>> + * For BR/EDR public address, key here is cross-transport derived,
>> + * so consider it as LE public address for SMP.
>> + *
>> + */
>> + addr_type = addr->type == BDADDR_BREDR ? BDADDR_LE_PUBLIC : addr->type;
>
> Ditto.
>
>> if (bacmp(&ev->rpa, BDADDR_ANY)) {
>> device = btd_adapter_get_device(adapter, &ev->rpa,
>> BDADDR_LE_RANDOM);
>> duplicate = btd_adapter_find_device(adapter, &addr->bdaddr,
>> - addr->type);
>> + addr_type);
>> if (duplicate == device)
>> duplicate = NULL;
>> } else {
>> device = btd_adapter_get_device(adapter, &addr->bdaddr,
>> - addr->type);
>> + addr_type);
>> duplicate = NULL;
>> }
>>
>> @@ -8941,7 +8968,7 @@ static void new_irk_callback(uint16_t index, uint16_t length,
>> return;
>> }
>>
>> - device_update_addr(device, &addr->bdaddr, addr->type);
>> + device_update_addr(device, &addr->bdaddr, addr_type);
>>
>> if (duplicate)
>> device_merge_duplicate(device, duplicate);
>> @@ -8950,7 +8977,7 @@ static void new_irk_callback(uint16_t index, uint16_t length,
>> if (!persistent)
>> return;
>>
>> - store_irk(adapter, &addr->bdaddr, addr->type, irk->val);
>> + store_irk(adapter, &addr->bdaddr, addr_type, irk->val);
>>
>> btd_device_set_temporary(device, false);
>> }
>> --
>> 2.25.1
>>
>>
>
>
