[SILABS] lock storage refactor #37849
Open
+435
−153
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Closed
|
PR #37849: Size comparison from ba8b539 to 29a9f25 Full report (74 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, efr32, esp32, linux, nrfconnect, nxp, psoc6, qpg, stm32, telink, tizen)
|
bzbarsky-apple
requested changes
Mar 5, 2025
| @@ -377,7 +376,7 @@ bool LockManager::GetUser(chip::EndpointId endpointId, uint16_t userIndex, Ember | |||
| uint16_t credentialSize = static_cast<uint16_t>(sizeof(CredentialStruct) * userInStorage.currentCredentialCount); | |||
There was a problem hiding this comment.
If userInStorage.currentCredentialCount is larger than kMaxCredentialsPerUser for some reason (e.g. firmware update changed the max value), you get a buffer overflow here. Of course in that situation you have other problems too, like not being able to load all the credentials.... But it's bad if some random bit-flip somewhere in storage leads to out of bounds writes in code.
There was a problem hiding this comment.
Good call. Added a check for userInStorage.currentCredentialCount <= kMaxCredentialsPerUser to prevent a buffer overflow in this case
mykrupp
force-pushed
the
silabs_lock_refactor
branch
from
9432b6e to
158d766
Compare
|
PR #37849: Size comparison from cf45d5d to 158d766 Full report (75 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, efr32, esp32, linux, nrfconnect, nxp, psoc6, qpg, stm32, telink, tizen)
|
| WeekDayScheduleInfo weekDayScheduleInStorage; | ||
| YearDayScheduleInfo yearDayScheduleInStorage; | ||
| HolidayScheduleInfo holidayScheduleInStorage; | ||
| CredentialStruct mCredentials[kMaxCredentialsPerUser]; |
There was a problem hiding this comment.
What is the intended purpose of having these variables as members of the class rather than having them as local variables in the members that uses them?
As it is now, the memory for all of these will always be allocated with the static LockManager, whereas if they would be declared in the members where they are used, the memory would only be allocated on the stack when they are needed.
There was a problem hiding this comment.
I will move the schedule storage to local variables in schedule functions, but the user/credential items return spans which need consistent storage somewhere.
This could be revisited if we decide to change EmberAfPluginDoorLockCredentialInfo and EmberAfPluginDoorLockUserInfo to have their own storage
There was a problem hiding this comment.
We can, but I think this will force us to rely on shared pointers in the meantime to ensure the caller of methods where we pass a pointer to our buffers will be done with the buffers when the next call happens.
This runs the risk of being equally tedious if we want to ensure the memory access is safe.
|
|
||
| user.userStatus = userInDb.userStatus; | ||
| if (UserStatusEnum::kAvailable == user.userStatus) | ||
| error = chip::Server::GetInstance().GetPersistentStorage().SyncGetKeyValue(userKey.KeyName(), &userInStorage, size); |
There was a problem hiding this comment.
This is an example, here we only need userInStorage and mCredential in this method, yet all the schedules will nonetheless take memory during and after this method is called.
There was a problem hiding this comment.
The outparam takes pointers into the delegate's storage, so... Not many options here.
There was a problem hiding this comment.
We were considering refactoring the doorlock server to make the caller own the memory buffer and copy into it rather than force the LockManager to own the buffer here.
Another option would be shared pointers that would be freed once the caller's pointer goes out of scope. Not ideal either.
| // Else if KVS read was successful | ||
| else if (error == CHIP_NO_ERROR) | ||
| { | ||
| user.credentials = chip::Span<const CredentialStruct>{ mCredentials, userInStorage.currentCredentialCount }; |
There was a problem hiding this comment.
Given the struct EmberAfPluginDoorLockUserInfo's line:
chip::Span<const CredentialStruct> credentials; /**< Credentials that are associated with user (without data).*/
I can see that we DO want mCredentials as a static member of the LockManager, however, we should consider protection for concurrent access.
| @@ -410,7 +425,7 @@ bool LockManager::SetUser(chip::EndpointId endpointId, uint16_t userIndex, chip: | |||
|
|
|||
| VerifyOrReturnValue(IsValidUserIndex(userIndex), false); | |||
|
|
|||
| auto & userInStorage = mLockUsers[userIndex]; | |||
| VerifyOrReturnValue(kInvalidEndpointId != endpointId, false); | |||
There was a problem hiding this comment.
On the subject of endpoint verification, since we want to have a single Manager for all endpoint, there should be a check that another operation on another endpoint is not ongoing since mCredential is allocated at the Manager level.
There was a problem hiding this comment.
For now I've added checks to each function for kInvalidEndpointId. I will add support for InitEndpoint for an arbitrary endpoint in a subsequent PR
|
|
||
| chip::StorageKeyName key = LockCredentialEndpoint(credentialIndex, credentialType, endpointId); | ||
|
|
||
| error = chip::Server::GetInstance().GetPersistentStorage().SyncGetKeyValue(key.KeyName(), &credentialInStorage, size); |
There was a problem hiding this comment.
same comment as userInStorage, credentialInStorage doesn't appear to need to be a member of the LockManager as all its value are copied into the & credential reference passed to this method.
|
|
||
| if (DlCredentialStatus::kAvailable == credential.status) | ||
| { | ||
| ChipLogDetail(Zcl, "Found unoccupied credential "); | ||
| return true; | ||
| } | ||
| credential.credentialType = credentialInStorage.credentialType; | ||
| credential.credentialData = credentialInStorage.credentialData; | ||
| credential.credentialData = chip::ByteSpan{ credentialInStorage.credentialData, credentialInStorage.credentialDataSize }; |
There was a problem hiding this comment.
And here again we commit to keep uint8_t credentialData[kMaxCredentialSize]; Because EmberAfPluginDoorLockCredentialInfo is designed in a way that forces it.
I suggest we at least move this array to be static like mCredentials so we can only keep these two arrays to be static. Than being said, I am tempted to say that instead we should just refactor EmberAfPluginDoorLockCredentialInfo and EmberAfPluginDoorLockUserInfo so they they hold their own memories...
Otherwise we need to figure out a way to know when credential becomes out of scope.
There was a problem hiding this comment.
Any function callback that returns a span needs memory allocated somewhere. We could ask in slack if people are ok with refactoring EmberAfPluginDoorLockCredentialInfo and EmberAfPluginDoorLockUserInfo to hold their own storage
| credentialInStorage.credentialType = credentialType; | ||
| credentialInStorage.createdBy = creator; | ||
| credentialInStorage.lastModifiedBy = modifier; | ||
| memcpy(credentialInStorage.credentialData, credentialData.data(), credentialInStorage.credentialDataSize); |
There was a problem hiding this comment.
This is one of the reasons why I am not a fan having this as a static structure... the last added credentials will remain in RAM, which is potentially a security risk. In this case I suggest to clear the credentialInStorage.credentialData once we have saved it in flash but we need a better solution that ensures that we do not keep this data allocated once its owner is done using it.
mykrupp
force-pushed
the
silabs_lock_refactor
branch
from
3899de2 to
18ad77e
Compare
|
PR #37849: Size comparison from cfcfef3 to 18ad77e Full report (14 builds for cc13x4_26x4, cc32xx, nrfconnect, qpg, stm32, tizen)
|
…eInStorage from class members to local function variables
|
PR #37849: Size comparison from cfcfef3 to 7bd0669 Full report (75 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, efr32, esp32, linux, nrfconnect, nxp, psoc6, qpg, stm32, telink, tizen)
|
bzbarsky-apple
requested changes
Mar 8, 2025
|
|
||
| user.userStatus = userInDb.userStatus; | ||
| if (UserStatusEnum::kAvailable == user.userStatus) | ||
| error = chip::Server::GetInstance().GetPersistentStorage().SyncGetKeyValue(userKey.KeyName(), &userInStorage, size); |
There was a problem hiding this comment.
The outparam takes pointers into the delegate's storage, so... Not many options here.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The original silabs lock-app implementation was a basic example and did not take in to account RAM and NVM3 efficiency.
This refactor
Testing
Tested by running the TC_DRLK test cases in pipeline that runs silabs hardware.