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.

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

BLE: Added call to delete the security database object upon SM reset. #11831

Merged
merged 1 commit into from Nov 7, 2019

Conversation

@AGlass0fMilk
Copy link
Contributor

AGlass0fMilk commented Nov 6, 2019

Description (required)

This patch deletes the security database object upon calling SecurityManager::reset().

Resolves #11768 (for now...)

Summary of change (What the change is for and why)

Previously, there was no normal method to have the security manager close the security database file (if using a file-based security database, as the Mbed SM does if not using a volatile, RAM-based database).

In some cases, this prevented the file from ever being fully flushed to flash if using a block device or filesystem that buffers write operations. The result of this was that for some applications/targets there was no official way to get BLE bonding persistence to work.

This fix allows the file to be closed by the OS and flushed to disk upon calling SecurityManager::reset().

Users should note that if calling SecurityManager::reset() to flush the database file to flash, they will also have to reinitialize the SecurityManager (set up an event handler, configure settings, etc).

Documentation (Details of any document updates required)

See issue #11768 for details.


Pull request type (required)

[X] Patch update (Bug fix / Target update / Docs update / Test update / Refactor)
[] Feature update (New feature / Functionality change / New API)
[] Major update (Breaking change E.g. Return code change / API behaviour change)

Test results (required)

I identified the problem and fix through the following method:

Tested on target: EP_AGORA but should be same for all nRF52840-based targets.

1.) Upon startup, application initializes SPIFBlockDevice and FATFileSystem (also used LittleFileSystem with same results)
2.) BLE was initialized and SecurityManager configured to write to a file -- /fs/sec.dat
3.) nRFConnect and an nRF52840_DK were used to connect and pair/bond with the target -- triggering the SecurityManager to store keys related to the connection
4.) Upon disconnection, a USBMSD object was created to present the FATFileSystem as a mass storage device to my laptop
5.) While the file existed in both cases, when I inspected the size and content of the file I found that with the code currently in master the file was never written to -- size of 0 bytes. With this fix, the file is correctly written to and is 568 or so bytes in size.

[] No Tests required for this change (E.g docs only update)
[X] Covered by existing mbed-os tests (Greentea or Unittest)
[] Tests / results supplied as part of this PR

Reviewers (optional)

@paul-szczepanek-arm


Release Notes (required for feature/major PRs)

Summary of changes
Impact of changes
Migration actions required
… allows the file to be closed by the OS and flushed to disk.
@ciarmcom ciarmcom requested review from paul-szczepanek-arm and ARMmbed/mbed-os-maintainers Nov 6, 2019
@ciarmcom

This comment has been minimized.

Copy link
Member

ciarmcom commented Nov 6, 2019

@ciarmcom ciarmcom requested a review from ARMmbed/mbed-os-pan Nov 6, 2019
@AGlass0fMilk

This comment has been minimized.

Copy link
Contributor Author

AGlass0fMilk commented Nov 6, 2019

@paul-szczepanek-arm

This fix is pretty much a workaround for the situation described above. However, after reviewing the SecurityManager API a bit more in detail I think there are some ways that it could be improved and made more flexible. We should start a discussion on future improvements to the BLE SM API presented by Mbed.

I have started a forum post here: https://os.mbed.com/forum/bugs-suggestions/topic/36266/

Copy link
Member

paul-szczepanek-arm left a comment

thanks, this helps but I guess we also need an automatic solution

@0xc0170
0xc0170 approved these changes Nov 7, 2019
@mbed-ci

This comment has been minimized.

Copy link

mbed-ci commented Nov 7, 2019

Test run: SUCCESS

Summary: 11 of 11 test jobs passed
Build number : 1
Build artifacts

@0xc0170 0xc0170 merged commit 9d4bd44 into ARMmbed:master Nov 7, 2019
26 checks passed
26 checks passed
continuous-integration/jenkins/pr-head This commit looks good
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
jenkins-ci/build-ARM Success
Details
jenkins-ci/build-GCC_ARM Success
Details
jenkins-ci/build-IAR Success
Details
jenkins-ci/cloud-client-test Success
Details
jenkins-ci/dynamic-memory-usage RTOS ROM(+0 bytes) RAM(+72 bytes)
Details
jenkins-ci/exporter Success
Details
jenkins-ci/greentea-test Success
Details
jenkins-ci/mbed2-build-ARM Success
Details
jenkins-ci/mbed2-build-GCC_ARM Success
Details
jenkins-ci/mbed2-build-IAR Success
Details
jenkins-ci/unittests Success
Details
travis-ci/astyle Success!
Details
travis-ci/docs Success!
Details
travis-ci/doxy-spellcheck Success!
Details
travis-ci/events Success! Runtime is 8698 cycles.
Details
travis-ci/gitattributestest Success!
Details
travis-ci/include_check Success!
Details
travis-ci/licence_check Success!
Details
travis-ci/littlefs Success! Code size is 8420B.
Details
travis-ci/psa-autogen Success!
Details
travis-ci/tools-py2.7 Success!
Details
travis-ci/tools-py3.5 Success!
Details
travis-ci/tools-py3.6 Success!
Details
travis-ci/tools-py3.7 Success!
Details
@0xc0170 0xc0170 removed the ready for merge label Nov 7, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.