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

BLE: Manual BLE security manager db synchronisation #14824

Merged
merged 3 commits into from
Jul 12, 2021
Merged

BLE: Manual BLE security manager db synchronisation #14824

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
7b4d1d5
add new BLE API call to sync security db with persisten storage
paul-szczepanek-arm Jun 23, 2021
c9d0ff8
ble security db complete sync implementation
paul-szczepanek-arm Jun 23, 2021
470917f
add implicit call to preserve state on reset
paul-szczepanek-arm Jun 28, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions UNITTESTS/fakes/ble/SecurityManagerImpl_mock.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@ class SecurityManagerMock : public ble::impl::SecurityManager {
MOCK_METHOD(ble_error_t, init, (bool enableBonding, bool requireMITM, SecurityIOCapabilities_t iocaps, const Passkey_t passkey, bool signing, const char *dbFilepath), (override));
MOCK_METHOD(ble_error_t, setDatabaseFilepath, (const char *dbFilepath), (override));
MOCK_METHOD(ble_error_t, preserveBondingStateOnReset, (bool enable), (override));
MOCK_METHOD(ble_error_t, writeBondingStateToPersistentStorage, (), (override));
MOCK_METHOD(ble_error_t, purgeAllBondingState, (), (override));
MOCK_METHOD(ble_error_t, generateWhitelistFromBondTable, (::ble::whitelist_t *whitelist), (const, override));
MOCK_METHOD(ble_error_t, requestPairing, (ble::connection_handle_t connectionHandle), (override));
Expand Down
2 changes: 2 additions & 0 deletions UNITTESTS/fakes/ble/source/generic/SecurityManagerImpl.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,8 @@ class SecurityManager {

virtual ble_error_t preserveBondingStateOnReset(bool enable) { return BLE_ERROR_NONE; };

virtual ble_error_t writeBondingStateToPersistentStorage() { return BLE_ERROR_NONE; };

////////////////////////////////////////////////////////////////////////////
// List management
//
Expand Down
17 changes: 17 additions & 0 deletions connectivity/FEATURE_BLE/include/ble/SecurityManager.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -501,11 +501,28 @@ class SecurityManager
* Normally all bonding information is lost when device is reset, this requests that the stack
* attempts to save the information and reload it during initialisation. This is not guaranteed.
*
* @note This option is itself saved together with bonding data. When data is read after reset,
* the state of this option decides if data should be restored. If this option has not been saved
Copy link
Contributor

@chrisswinchatt-arm chrisswinchatt-arm Jun 24, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

My read of this is that writeBondingStateToPersistentStorage is effectively a no-op unless preserveBondingStateOnReset is set to true before the reset. What's the rationale behind this?

Copy link
Member Author

Choose a reason for hiding this comment

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

preserveBondingStateOnReset is an option that affects behaviour after the reset. Maybe I should've called it loadBondingDataFromPersistentStorageAfterReset

Copy link
Contributor

Choose a reason for hiding this comment

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

Could writeBondingStateToPersistentStorage implicitly set preserveBondingStateOnReset? Or is there a reason someone would write the data but not automatically load it after a reset?

Copy link
Member Author

Choose a reason for hiding this comment

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

That is a good point. If I can't find a good reason by tomorrow I'll add the call.

* the data will not be restored even if partial data is present.
*
* @param[in] enable if true the stack will attempt to preserve bonding information on reset.
* @return BLE_ERROR_NONE or appropriate error code indicating the failure reason.
*/
ble_error_t preserveBondingStateOnReset(bool enable);

/**
* Some or all of bonding information may be stored in memory while in use. This will write
* bonding data to persistent storage. This will have no effect if no persistent storage is enabled.
*
* @note This implicitly also calls preserveBondingStateOnReset(true) inside.
*
* @note Depending on the driver used to implement the storage solution used this may be a disruptive
Copy link
Member

Choose a reason for hiding this comment

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

Maybe we could use a less ominous tone and instruct the user to prefer calling this function outside of connection otherwise the connection can drop if storage is the internal flash.

Copy link
Member Author

Choose a reason for hiding this comment

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

Well, this is what it means. Other operations may be affected not just connections. That's why I used generic phrasing.

* operation and may cause active connections to drop due to failed processing deadlines.
*
* @return BLE_ERROR_NONE or appropriate error code indicating the failure reason.
*/
ble_error_t writeBondingStateToPersistentStorage();

////////////////////////////////////////////////////////////////////////////
// List management
//
Expand Down
9 changes: 9 additions & 0 deletions connectivity/FEATURE_BLE/source/SecurityManager.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,15 @@ ble_error_t SecurityManager::preserveBondingStateOnReset(bool enable)
return impl->preserveBondingStateOnReset(enable);
}

ble_error_t SecurityManager::writeBondingStateToPersistentStorage()
{
ble_error_t err = impl->preserveBondingStateOnReset(true);
if (err) {
return err;
}
return impl->writeBondingStateToPersistentStorage();
}

ble_error_t SecurityManager::purgeAllBondingState()
{
return impl->purgeAllBondingState();
Expand Down
16 changes: 16 additions & 0 deletions connectivity/FEATURE_BLE/source/generic/FileSecurityDb.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -361,6 +361,22 @@ void FileSecurityDb::restore()

void FileSecurityDb::sync(entry_handle_t db_handle)
{
/* if no entry is selected we will sync all entries */
if (db_handle == invalid_entry_handle) {
/* only write the connected devices as others are already written */
for (size_t i = 0; i < get_entry_count(); i++) {
entry_handle_t db_handle = get_entry_handle_by_index(i);
SecurityDistributionFlags_t* flags = get_distribution_flags(db_handle);

if (flags && flags->connected) {
sync(db_handle);
}
}
/* global sync triggers a flush */
fflush(_db_file);
return;
}

entry_t *entry = as_entry(db_handle);
if (!entry) {
return;
Expand Down
2 changes: 1 addition & 1 deletion connectivity/FEATURE_BLE/source/generic/FileSecurityDb.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -136,7 +136,7 @@ class FileSecurityDb : public SecurityDb {

virtual void restore();

virtual void sync(entry_handle_t db_handle);
virtual void sync(entry_handle_t db_handle = invalid_entry_handle);
Copy link
Member

Choose a reason for hiding this comment

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

What do we gain with the default parameter ? It is an implementation and it won't be called directly by the BLE framework.

Copy link
Member Author

Choose a reason for hiding this comment

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

So that I don't have to modify existing code.


virtual void set_restore(bool reload);

Expand Down
16 changes: 16 additions & 0 deletions connectivity/FEATURE_BLE/source/generic/KVStoreSecurityDb.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -333,6 +333,22 @@ void KVStoreSecurityDb::restore()

void KVStoreSecurityDb::sync(entry_handle_t db_handle)
{
/* storage synchronisation is handled by KVStore itself, no explicit flushing */

/* if no entry is selected we will sync all entries */
if (db_handle == invalid_entry_handle) {
/* only write the connected devices as others are already written */
for (size_t i = 0; i < get_entry_count(); i++) {
entry_handle_t db_handle = get_entry_handle_by_index(i);
SecurityDistributionFlags_t* flags = get_distribution_flags(db_handle);

if (flags && flags->connected) {
sync(db_handle);
}
}
return;
}

entry_t *entry = as_entry(db_handle);
if (!entry) {
return;
Expand Down
2 changes: 1 addition & 1 deletion connectivity/FEATURE_BLE/source/generic/KVStoreSecurityDb.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -200,7 +200,7 @@ class KVStoreSecurityDb : public SecurityDb {

virtual void restore();

virtual void sync(entry_handle_t db_handle);
virtual void sync(entry_handle_t db_handle = invalid_entry_handle);
Copy link
Member

Choose a reason for hiding this comment

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

Same as above

Copy link
Member Author

Choose a reason for hiding this comment

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

See above.


virtual void set_restore(bool reload);

Expand Down
10 changes: 8 additions & 2 deletions connectivity/FEATURE_BLE/source/generic/SecurityDb.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -113,6 +113,8 @@ class SecurityDb {
*/
typedef void* entry_handle_t;

static constexpr entry_handle_t invalid_entry_handle = nullptr;

/* callbacks for asynchronous data retrieval from the security db */

typedef mbed::Callback<void(entry_handle_t, const SecurityEntryKeys_t*)>
Expand Down Expand Up @@ -520,9 +522,13 @@ class SecurityDb {
virtual void restore();

/**
* Flush all values which might be stored in memory into NVM.
* Write all values and attempt to sync persistent storage. Passing in an optional valid
* db_handle will only write the given entry and not attempt to flush buffers.
*
* @param db_handle database entry to write. If invalid all entries are written and
* persistent storage attempts to sync (flush buffers).
*/
virtual void sync(entry_handle_t db_handle);
virtual void sync(entry_handle_t db_handle = invalid_entry_handle);

/**
* Toggle whether values should be preserved across resets.
Expand Down
12 changes: 12 additions & 0 deletions connectivity/FEATURE_BLE/source/generic/SecurityManagerImpl.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -222,6 +222,18 @@ ble_error_t SecurityManager::preserveBondingStateOnReset(bool enabled)
return BLE_ERROR_NONE;
}


ble_error_t SecurityManager::writeBondingStateToPersistentStorage()
{
tr_info("Writing bonding to storage");
if (!_db) {
tr_error("Failure, DB not initialized");
return BLE_ERROR_INITIALIZATION_INCOMPLETE;
}
_db->sync();
return BLE_ERROR_NONE;
}

////////////////////////////////////////////////////////////////////////////
// List management
//
Expand Down
2 changes: 2 additions & 0 deletions connectivity/FEATURE_BLE/source/generic/SecurityManagerImpl.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -85,6 +85,8 @@ class SecurityManager :

ble_error_t preserveBondingStateOnReset(bool enable);

ble_error_t writeBondingStateToPersistentStorage();

////////////////////////////////////////////////////////////////////////////
// List management
//
Expand Down