Re: [PATCH] Bluetooth: Add conn type to identify addr type with SMP over BR/EDR

Hi Jiangbo,

quoted

quoted

quoted

quoted

quoted

quoted

quoted

quoted

quoted

If pair a device that unpair firstly that remove encryption
key,
encryption key event will be emitted. kernel will receive
'L2CAP_CID_SMP_BREDR' frame, and then it will use SMP to
distribute
key.  SMP would like to use LTK, IRK and CRSK to notify user.
If it
don't identify device by which conn type they are, only marks
LE as
the device type,

Why would that happen? Before SMP over BR/EDR happens pairing
would
have
happened over BR/EDR, so bluetoothd should know that BR/EDR is
supported
as well (it would even be aware of an existing BR/EDR
connection). Are
you perhaps trying to work around some bluetoothd bug with all
this?

I use upstream bluez source code without change.

Yes, bluetoothd scan will find device type is BR/EDR or LE. As my
case,
device is BR/EDR. But if kernel report CRSK notify, bluetoothd will
change

the device type to LE. The code you can see:
 new_csrk_callback -> btd_adapter_get_device ->

btd_adapter_find_device

quoted

quoted

quoted

         if (bdaddr_type == BDADDR_BREDR)

                 device_set_bredr_support(device);

         else

                 device_set_le_support(device, bdaddr_type);

As Marcel mentioned before, LTK, IRK and CRSK are only valid for LE
link.
So the rootcause is why remote start to pair a BR/EDR device, the
kernel
will receive CRSK event.

This is the first pair, and it will pair success even if receive
CRSK
notify. And the second and the next all pair will be failed with
remote
device unpair and then pair again.

quoted

quoted

while Bluetoothd will use this 'addr' and 'addr type' to reply
the
comfirm to kernel.

What reply are you talking about? There's no user interaction
involved
with SMP over BR/EDR - that would already have occurred when SSP
over
BR/EDR happened.

Sorry to confuse the case, the pairing failed coming with next pair
procedure. Because at the last pair with CRSK notify, device type
will
be
changed to LE, following is the failed scenario after last success
with
CRSK notify. Remote unpair and pair again.

This reply is SPP, user confirm passkey reply. When pairing
proceduce,
User
confirm the pairing request through bluetoothd, that will send mgmt
op
'MGMT_OP_USER_CONFIRM_REPLY' with device address and device type in
mgmt_cp_user_confirm_reply. Kernel use the device address and type
to
lookup hci conn. Unfortunately, it will lookup hci_conn from LE
hashtable, that don't include hci conn. So spp reply couldn't send
to
remote, caused pair failed.

quoted

quoted

At the same time kernel always uses them to lookup hci_conn in
LE
hashtable firstly, because addr type always marks as LE.
Obviously
it
will failed with SMP over BR/EDR.

I don't follow this either since there shouldn't have been any
"reply"
from user space for SMP over BR/EDR. All there should be are
events
from
the kernel for the generated LE keys.

quoted

Actually, SPM is only for LE in SPEC,

That's not true. SMP is specified both for LE-U and ACL-U.

quoted

but kernel already support and use SMP over BR/EDR. if BR/EDR
exchanges key with SMP, it will never reply pairing response to
remote, in other words it will be never paired, that is
happened in
our products.

Szymon recently implemented SMP over BR/EDR for Zephyr and used
Linux/BlueZ as a reference for testing. He didn't report any
issues
like
this. It might help if you could provide some logs (particularly
HCI/btmon but also from bluetoothd) to understand what's the
actual
issue you're seeing.

Johan

Sorry to confuse this issue, the log is not in my hand right now,
so it maybe later.

So I was able to reproduce this issue. This is bluetoothd bug and not
kernel one. This bug is no specific to cross-transport pairing. It
can
happen with any dual-mode device that is doing BR/EDR pairing while
being
known as dual mode by bluetoothd when agent replies with passkey or
confirmation.

To fix this we probably need to hold extra information in
'struct authentication_req' in device.c about type of pairing (LE or
BR/EDR). This is not a one-liner-fix so I don't have a patch ready
yet.

Totally agree with you about dual-mode device pairing known as dual
mode.
But i want to known is that reasonable about device is to do BR/EDR
pairing
will generate CRSK notify? I'm very intersting about this fixing, this
bug
is hight priority in our product. In my opinion hold extra informatin
in
'struct authentication_req' may not fix this bug. Because if CRSK event
is
still report, then bluetoothd will change the device type to LE even if
we
pair device that is scaned with BR/EDR. So i think the rootcase is find
does CRSK event make sense in BR/EDR pairing, and how to handle CRSK
events
in BR/EDR pairing if it make sense. I'm confuse with those.

It doesn't change the device to LE but to dual mode device. This is
*cross-transport* pairing so keys for other transport are generated.
baddr_type specify only LE address type, not BR/EDR since there is no
address type for BR/EDR. This is mostly true but few places in
bluetoothd seem to asusme that for device supporting BR/EDR type is
equal 0. Which is not true if device is dual mode.

You should be able to reproduce this bug with dual-mode devices that
don't
support cross-transport pairing: enable advertising, scan from linux,
when
device is found stop advertising and make device discoverable over
BR/EDR
(inquiry). When device is found over BR/EDR stop scanning and start
pairing.> > 

quoted

I noticed that if quikly reply the passkey confirm, this bug always be
reproduced, but if wait for 2~3s to reply the passkey confirm, it works
well every time. In terms of code, wait for 2~3s will cause l2cap chan
timeout for info timer that created by HCI_EV_REMOTE_EXT_FEATURES
event,
and timeout will change l2cap chan to BT_CONNECTED. So next SMP
resume/ready don't distribute key also CRSK events.

It can't reproduce with btmgmt, because it reply passkey confirm always
only use BR/EDR in 'struct mgmt_cp_user_confirm_reply' not use device
relation type.

bluetoothd.log and btmon.log are attached. It records two pair request
sequence, one is pair success that have CRSK event, another is next
pair
reqeust don't success any, hope those maybe help you to annlyze this
bug.

I've sent a patch "[RFC] core: Fix BR/EDR pairing for dual mode devices".
Please check if this solves issue you are seeing.

Thanks for your patch. Maybe it can resolve the issue, but it will cause
other issues. For example, some operations also use device->bdaddr as
parameter in MGMT operations, unpair is the same. If kernel hold the device
as BR/EDR type in hdev->conn_hash, unpair operator won't find the hci conn,
so it couldn't terminal the link. but the link is exist at the moment. MGMT
also complete when don't terminal the link. So bluetoothd and the user
don't feel the different. But is that we would like? The code implies we
should terminate the connection if it is exist.

The patch use auth_req with BDADDR_BREDR to handle pairing request. It could
resolve the pairing procedure. But kernel hold the device as BR/EDR, even
if cross-tranport is generated on BR/EDR hci conn. Meanwhile bluetoothd
will set device->bdaddr_type to BDADDR_LE_PUBLIC with new_csrk_callback
that generated by cross-transport. I mean, the user-space hold the device
with BDADDR_LE_PUBLIC (yes, device->bredr and device->le are true, but
addr-type is BDADDR_LE_PUBLIC), the kernel hold the device with
BDADDR_BREDR. Whenever user-space use couple {addr, addr-type} to send
request to kernel. It maybe failed.

bdaddr_type is used only with LE address (not with BR/EDR address) so I'm not 
sure what other issues you are talking about. Could you provide some logs?

quoted

As the end. In my case, i don't do the steps you mentioned(enable
advertising, scan from linux, stop advertising). I only start BR/EDR
discovering with discovery filter, pair device and unpair device to
reproduce this bug. I don't start/stop LE advertising.

So you still see your issue with this patch?

You can see in my case, all of them are BR/EDR. I use D-Bus method 'SetDiscoveryFilter'
to enable bredr scan only. So scanning is BR/EDR, pairing is BR/EDR, HCI conn is
created with BR/EDR. Even if crsk is generated, it's also transport SMP over BR/EDR.

SMP over BR/EDR is for transporting LE keys. It is not for transporting BR/EDR keys. Full stop here. Please understand what I wrote initially. The IRK and CSRK transported via SMP over BR/EDR are LE keys. If they are transported, your remote device offers cross-transport pairing and it is a dual-mode device.

That you discover only BR/EDR devices has nothing to do with when pairing them. If you get keys for both BR/EDR and LE, then that is because BlueZ supports cross-transport pairing and so does the remote side.

bluetoothd/kernel all consider peers is BR/EDR.

Your case enable LE advertising, so you think peers is LE. But it's not correct
in my case. I only use BR/EDR. So your patch can resolve the pairing procedure,
but what about other operations? They all hold the addr type is BR/EDR, not LE.
I maintain my submission patch locally, and it works fine in our product.

Do _not_ maintain the kernel patch you send initially. It is _wrong_ and it will make it specification non-compliant.

If the remote side offers cross-transport pairing, then the remote side is a dual-mode device. It is plain simple as that. It means it will allow you to also connect via LE since all the keys are exchanged. LE related keys like LTK, IRK and CSRK are send with the LE address type. That is meant to be this way.

Regards

Marcel