Skip to content

Fix nordic security cancellation #7210

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
Merged
merged 7 commits into from
Jun 25, 2018
Merged

Fix nordic security cancellation #7210

merged 7 commits into from
Jun 25, 2018

Conversation

pan-
Copy link
Member

@pan- pan- commented Jun 13, 2018

Description

This PR fix the way Bluetooth pairing is cancelled on Nordic targets. Depending on the state of the device, different calls must be issued to cancel Bluetooth pairing. One state (Bluetooth central) was not managed and a regression in the Nordic stack forced this patch.

Pull request type

[X] Fix
[ ] Refactor
[ ] New target
[ ] Feature
[ ] Breaking change

pan- added 5 commits June 13, 2018 15:02
@pan-
Nordic BLE: Improve return of nRF5xn::getGap.
8e7936d 
Return the derived type instead of the abstract one. This is legal as C++ supports covariant returns.
@pan-
Nordic BLE: Add default parameter to get the local instance of nrF5xn
5c7b8c8
@pan-
Nordic BLE: Store locally role for a given connection.
301e527
@pan-
Nordic BLE: Fix pairing cancellation.
f0c3f14 
Depending on the role and the current state of the local device; pairing cancelation should be made with a call to a specific function. Normally the Nordic stack would reject invalid calls if the device is not in the correct state; therefore it was assumed that it was possible to detect the state from sd errors. Unfortunatelly this is not true with the latest softdevices as some calls succeed even if the device is not in the right state.

To solve that issue cancelation looks at the current state of the device first to select the right function that will trigger the pairing cancellation.

Note: the call to sd_ble_gap_authenticate was missing in the previous algorithm
@pan-
Nordic BLE:
5237c3e 
Cancel pairing if the device fail to allocate the resources necessary for the pairing operation.
0xc0170
0xc0170 previously approved these changes Jun 13, 2018
View reviewed changes
@cmonr
Copy link
Contributor

cmonr commented Jun 13, 2018

/morph build

@mbed-ci
Copy link

mbed-ci commented Jun 13, 2018

Build : SUCCESS

Build number : 2347
Build artifacts/logs : http://mbed-os.s3-website-eu-west-1.amazonaws.com/?prefix=builds/7210/

Triggering tests

/morph test
/morph uvisor-test
/morph export-build
/morph mbed2-build

@mbed-ci
Copy link

mbed-ci commented Jun 13, 2018

Exporter Build : SUCCESS

Build number : 1971
Build artifacts/logs : http://mbed-os.s3-website-eu-west-1.amazonaws.com/?prefix=builds/exporter/7210/

paul-szczepanek-arm
paul-szczepanek-arm suggested changes Jun 14, 2018
View reviewed changes
Copy link
Member

@paul-szczepanek-arm paul-szczepanek-arm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the reject command returns an error during tests

@mbed-ci
Copy link

mbed-ci commented Jun 15, 2018

Test : SUCCESS

Build number : 2132
Test logs :http://mbed-os-logs.s3-website-us-west-1.amazonaws.com/?prefix=logs/7210/2132

@cmonr
Copy link
Contributor

cmonr commented Jun 15, 2018

@paul-szczepanek-arm Mind elaborating on which tests these are?

@paul-szczepanek-arm
Copy link
Member

I used clitest SM_persistent_db_test_01, but the checked in version has been modified to work around this problem

@cmonr
Copy link
Contributor

cmonr commented Jun 15, 2018

the checked in version has been modified to work around this problem

@paul-szczepanek-arm Does that mean that this PR is good to go? If so, would you mind approving the PR?

@paul-szczepanek-arm
Copy link
Member

No, I mean the test has a work around since it's a pre-existing problem. This PR aims to fix it but doesn't. Yet. I used that test without the workaround to check.

@cmonr
Copy link
Contributor

cmonr commented Jun 15, 2018

OH, ok. Thanks for the clarification.

@cmonr
Copy link
Contributor

cmonr commented Jun 15, 2018

@paul-szczepanek-arm Would you mind posting the test results to help with tracking down the issue(s)?

@cmonr
Copy link
Contributor

cmonr commented Jun 21, 2018

Making a note here that a fix is still incoming from @pan- before this can move forward.

@pan-
Copy link
Member Author

pan- commented Jun 22, 2018

@cmonr I've pushed a fix and added a test https://github.com/ARMmbed/ble-tests/pull/44 .

@cmonr
Copy link
Contributor

cmonr commented Jun 23, 2018

/morph build

@mbed-ci
Copy link

mbed-ci commented Jun 23, 2018

Build : SUCCESS

Build number : 2436
Build artifacts/logs : http://mbed-os.s3-website-eu-west-1.amazonaws.com/?prefix=builds/7210/

Triggering tests

/morph test
/morph uvisor-test
/morph export-build
/morph mbed2-build

@mbed-ci
Copy link

mbed-ci commented Jun 23, 2018

Test : SUCCESS

Build number : 2216
Test logs :http://mbed-os-logs.s3-website-us-west-1.amazonaws.com/?prefix=logs/7210/2216

@mbed-ci
Copy link

mbed-ci commented Jun 23, 2018

Exporter Build : SUCCESS

Build number : 2068
Build artifacts/logs : http://mbed-os.s3-website-eu-west-1.amazonaws.com/?prefix=builds/exporter/7210/

@cmonr cmonr merged commit 2e233a9 into ARMmbed:master Jun 25, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cmonr cmonr cmonr approved these changes

@paul-szczepanek-arm paul-szczepanek-arm paul-szczepanek-arm approved these changes

@0xc0170 0xc0170 0xc0170 left review comments

@donatieng donatieng Awaiting requested review from donatieng

Assignees
No one assigned
Labels
devices: nordic release-version: 5.9.2
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@pan- @cmonr @mbed-ci @paul-szczepanek-arm @0xc0170