Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Chip-tool wrongly handle discovery via BLE with manual pairing code #21160

Closed
doublemis1 opened this issue Jul 25, 2022 · 2 comments · Fixed by #21187
Closed

Chip-tool wrongly handle discovery via BLE with manual pairing code #21160

doublemis1 opened this issue Jul 25, 2022 · 2 comments · Fixed by #21187
Assignees
@Damian-Nordic
Labels
request cert blocker

Comments

@doublemis1
Copy link
Contributor

Problem

The Manual Pairing code contains the information about 4 most significant bits of discriminator.
The current implementation of chip-tool commissioning via BLE wrongly handles the discovery of devices that have set the discriminator to e.g. E64 (3684). The commissioning failed at BLE discovery since the manual paring code contains the E (E00 = 3584).

Parsed device manual pairing code:

chip_tool/chip-tool payload parse-setup-payload 34069117130
[1658752774.437685][17773:17773] CHIP:SPL: Parsing decimalRepresentation: 34069117130
[1658752774.437713][17773:17773] CHIP:SPL: Version:       0
[1658752774.437715][17773:17773] CHIP:SPL: VendorID:      0
[1658752774.437717][17773:17773] CHIP:SPL: ProductID:     0
[1658752774.437719][17773:17773] CHIP:SPL: Custom flow:   0    (STANDARD)
[1658752774.437720][17773:17773] CHIP:SPL: Capabilities:  0x00 (NONE)
[1658752774.437722][17773:17773] CHIP:SPL: Discriminator: 3584
[1658752774.437724][17773:17773] CHIP:SPL: Passcode:      28073715

Parsed device QR code payload:

chip-tool payload parse-setup-payload MT:KAYA3E.N16WWU-5KM00
[1658752813.867883][17774:17774] CHIP:SPL: Parsing base38Representation: MT:KAYA3E.N16WWU-5KM00
[1658752813.867921][17774:17774] CHIP:SPL: Version:       0
[1658752813.867924][17774:17774] CHIP:SPL: VendorID:      4735
[1658752813.867926][17774:17774] CHIP:SPL: ProductID:     32781
[1658752813.867928][17774:17774] CHIP:SPL: Custom flow:   0    (STANDARD)
[1658752813.867930][17774:17774] CHIP:SPL: Capabilities:  0x02 (BLE)
[1658752813.867933][17774:17774] CHIP:SPL: Discriminator: 3654
[1658752813.867936][17774:17774] CHIP:SPL: Passcode:      28073715

Proposed Solution

The chip-tool during commissioning by manual pairing code shall connect the device based only on 4 most significant bits rather than the whole discriminator.
@bzbarsky-apple
Copy link
Contributor

@doublemis1 Which chip-tool are you running? Linux or Mac?

The Mac one tries to handle this, in checkDiscriminator in src/platform/Darwin/BleConnectionDelegateImpl.mm. It's not ideal in that it can incorrectly decide there's a match when we have a 12-bit discriminator but the low 8 bits are all zero, if the high 4 bits happen to match the high 4 bits of the device discriminator, but it should not fail to detect matches.

The Linux version (BLEManagerImpl::OnDeviceScanned in src/platform/Linux/BLEManagerImpl.cpp) does in fact look buggy.

@doublemis1
Copy link
Contributor Author

Yes, I used the Linux version.

@bzbarsky-apple bzbarsky-apple mentioned this issue Jul 25, 2022
Closed
@Damian-Nordic Damian-Nordic self-assigned this Jul 25, 2022
@Damian-Nordic Damian-Nordic mentioned this issue Jul 25, 2022
Closed
bzbarsky-apple added a commit to bzbarsky-apple/connectedhomeip that referenced this issue Jul 25, 2022
@bzbarsky-apple
Fix handling of short discriminator in Linux BLE scan.
39ebc91 
A few changes here:

1. Implement a SetupDiscriminator class that commons up logic like "does this
   discriminator, which might be short or long, match the given long
   discriminator?" and "extract short discriminator from long discriminator".
2. Change SetupPayload to more clearly indicate whether it's storing a short or
   long discriminator, instead of storing a short discriminator the same way as
   a long discriminator with the low bits all 0 and hoping consumers check
   isShortDiscriminator.
3. Update BLE scanning APIs to take SetupDiscriminator.
4. Fix the Linux and Tizen BLE code to properly handle short discriminators
   (which used to not match if the long discriminator happened to have the low 8
   bits nonzero).
5. Fix the Darwin BLE code to properly handle long discriminators that have 0
   low bits.  Before this change they used to incorrectly match a long
   discriminator which had the same 4 high bits but different 8 low bits.

Fixes project-chip#21160
@bzbarsky-apple bzbarsky-apple mentioned this issue Jul 25, 2022
Merged
bzbarsky-apple added a commit to bzbarsky-apple/connectedhomeip that referenced this issue Jul 26, 2022
@bzbarsky-apple
Fix handling of short discriminator in Linux BLE scan.
fce79f0 
A few changes here:

1. Implement a SetupDiscriminator class that commons up logic like "does this
   discriminator, which might be short or long, match the given long
   discriminator?" and "extract short discriminator from long discriminator".
2. Change SetupPayload to more clearly indicate whether it's storing a short or
   long discriminator, instead of storing a short discriminator the same way as
   a long discriminator with the low bits all 0 and hoping consumers check
   isShortDiscriminator.
3. Update BLE scanning APIs to take SetupDiscriminator.
4. Fix the Linux and Tizen BLE code to properly handle short discriminators
   (which used to not match if the long discriminator happened to have the low 8
   bits nonzero).
5. Fix the Darwin BLE code to properly handle long discriminators that have 0
   low bits.  Before this change they used to incorrectly match a long
   discriminator which had the same 4 high bits but different 8 low bits.

Fixes project-chip#21160
bzbarsky-apple added a commit to bzbarsky-apple/connectedhomeip that referenced this issue Jul 26, 2022
@bzbarsky-apple
Fix handling of short discriminator in Linux BLE scan.
cd6deef 
A few changes here:

1. Implement a SetupDiscriminator class that commons up logic like "does this
   discriminator, which might be short or long, match the given long
   discriminator?" and "extract short discriminator from long discriminator".
2. Change SetupPayload to more clearly indicate whether it's storing a short or
   long discriminator, instead of storing a short discriminator the same way as
   a long discriminator with the low bits all 0 and hoping consumers check
   isShortDiscriminator.
3. Update BLE scanning APIs to take SetupDiscriminator.
4. Fix the Linux and Tizen BLE code to properly handle short discriminators
   (which used to not match if the long discriminator happened to have the low 8
   bits nonzero).
5. Fix the Darwin BLE code to properly handle long discriminators that have 0
   low bits.  Before this change they used to incorrectly match a long
   discriminator which had the same 4 high bits but different 8 low bits.

Fixes project-chip#21160
bzbarsky-apple added a commit to bzbarsky-apple/connectedhomeip that referenced this issue Jul 26, 2022
@bzbarsky-apple
Fix handling of short discriminator in Linux BLE scan.
32166cb 
A few changes here:

1. Implement a SetupDiscriminator class that commons up logic like "does this
   discriminator, which might be short or long, match the given long
   discriminator?" and "extract short discriminator from long discriminator".
2. Change SetupPayload to more clearly indicate whether it's storing a short or
   long discriminator, instead of storing a short discriminator the same way as
   a long discriminator with the low bits all 0 and hoping consumers check
   isShortDiscriminator.
3. Update BLE scanning APIs to take SetupDiscriminator.
4. Fix the Linux and Tizen BLE code to properly handle short discriminators
   (which used to not match if the long discriminator happened to have the low 8
   bits nonzero).
5. Fix the Darwin BLE code to properly handle long discriminators that have 0
   low bits.  Before this change they used to incorrectly match a long
   discriminator which had the same 4 high bits but different 8 low bits.

Fixes project-chip#21160
bzbarsky-apple added a commit that referenced this issue Jul 26, 2022
@bzbarsky-apple
Fix handling of short discriminator in Linux BLE scan. (#21187)
79e8c9e 
* Fix handling of short discriminator in Linux BLE scan.

A few changes here:

1. Implement a SetupDiscriminator class that commons up logic like "does this
   discriminator, which might be short or long, match the given long
   discriminator?" and "extract short discriminator from long discriminator".
2. Change SetupPayload to more clearly indicate whether it's storing a short or
   long discriminator, instead of storing a short discriminator the same way as
   a long discriminator with the low bits all 0 and hoping consumers check
   isShortDiscriminator.
3. Update BLE scanning APIs to take SetupDiscriminator.
4. Fix the Linux and Tizen BLE code to properly handle short discriminators
   (which used to not match if the long discriminator happened to have the low 8
   bits nonzero).
5. Fix the Darwin BLE code to properly handle long discriminators that have 0
   low bits.  Before this change they used to incorrectly match a long
   discriminator which had the same 4 high bits but different 8 low bits.

Fixes #21160

* Address review comment.
github-actions bot pushed a commit that referenced this issue Jul 26, 2022
@bzbarsky-apple @web-flow
Fix handling of short discriminator in Linux BLE scan. (#21187)
9e041aa 
* Fix handling of short discriminator in Linux BLE scan.

A few changes here:

1. Implement a SetupDiscriminator class that commons up logic like "does this
   discriminator, which might be short or long, match the given long
   discriminator?" and "extract short discriminator from long discriminator".
2. Change SetupPayload to more clearly indicate whether it's storing a short or
   long discriminator, instead of storing a short discriminator the same way as
   a long discriminator with the low bits all 0 and hoping consumers check
   isShortDiscriminator.
3. Update BLE scanning APIs to take SetupDiscriminator.
4. Fix the Linux and Tizen BLE code to properly handle short discriminators
   (which used to not match if the long discriminator happened to have the low 8
   bits nonzero).
5. Fix the Darwin BLE code to properly handle long discriminators that have 0
   low bits.  Before this change they used to incorrectly match a long
   discriminator which had the same 4 high bits but different 8 low bits.

Fixes #21160

* Address review comment.
@woody-apple woody-apple mentioned this issue Jul 26, 2022
Merged
woody-apple added a commit that referenced this issue Jul 27, 2022
@woody-apple @bzbarsky-apple
Fix handling of short discriminator in Linux BLE scan. (#21187) (#21241)
bc36684 
* Fix handling of short discriminator in Linux BLE scan.

A few changes here:

1. Implement a SetupDiscriminator class that commons up logic like "does this
   discriminator, which might be short or long, match the given long
   discriminator?" and "extract short discriminator from long discriminator".
2. Change SetupPayload to more clearly indicate whether it's storing a short or
   long discriminator, instead of storing a short discriminator the same way as
   a long discriminator with the low bits all 0 and hoping consumers check
   isShortDiscriminator.
3. Update BLE scanning APIs to take SetupDiscriminator.
4. Fix the Linux and Tizen BLE code to properly handle short discriminators
   (which used to not match if the long discriminator happened to have the low 8
   bits nonzero).
5. Fix the Darwin BLE code to properly handle long discriminators that have 0
   low bits.  Before this change they used to incorrectly match a long
   discriminator which had the same 4 high bits but different 8 low bits.

Fixes #21160

* Address review comment.

Co-authored-by: Boris Zbarsky <[email protected]>
isiu-apple pushed a commit to isiu-apple/connectedhomeip that referenced this issue Sep 16, 2022
@bzbarsky-apple @isiu-apple
Fix handling of short discriminator in Linux BLE scan. (project-chip#…
0a544a0 
…21187)

* Fix handling of short discriminator in Linux BLE scan.

A few changes here:

1. Implement a SetupDiscriminator class that commons up logic like "does this
   discriminator, which might be short or long, match the given long
   discriminator?" and "extract short discriminator from long discriminator".
2. Change SetupPayload to more clearly indicate whether it's storing a short or
   long discriminator, instead of storing a short discriminator the same way as
   a long discriminator with the low bits all 0 and hoping consumers check
   isShortDiscriminator.
3. Update BLE scanning APIs to take SetupDiscriminator.
4. Fix the Linux and Tizen BLE code to properly handle short discriminators
   (which used to not match if the long discriminator happened to have the low 8
   bits nonzero).
5. Fix the Darwin BLE code to properly handle long discriminators that have 0
   low bits.  Before this change they used to incorrectly match a long
   discriminator which had the same 4 high bits but different 8 low bits.

Fixes project-chip#21160

* Address review comment.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@Damian-Nordic Damian-Nordic

Labels
request cert blocker
Projects
None yet
Milestone
No milestone
3 participants
@doublemis1 @bzbarsky-apple @Damian-Nordic