[EFR32] Lock-app updates, adding multiple users and credentials #19229
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
pullapprove
bot
requested review from
anush-apple,
arkq,
Byungjoo-Lee,
bzbarsky-apple,
carol-apple,
chrisdecenzo,
chshu,
chulspro,
Damian-Nordic,
dhrishi,
electrocucaracha,
franck-apple,
gjc13,
harimau-qirex,
hawk248,
harsha-rajendran,
isiu-apple,
jelderton,
jepenven-silabs,
jmartinez-silabs,
jtung-apple,
kghost,
kpschoedel,
lazarkov and
LuDuda
pullapprove
bot
requested review from
wbschiller,
woody-apple,
xylophone21,
yufengwangca and
yunhanw-google
|
PR #19229: Size comparison from 9c1660c to 97d2504 Increases above 0.2%:
Increases (1 build for efr32)
Full report (41 builds for cc13x2_26x2, cyw30739, efr32, esp32, k32w, linux, mbed, nrfconnect, p6, telink)
|
|
PR #19229: Size comparison from c868f81 to 3fa0780 Increases (6 builds for cc13x2_26x2, nrfconnect)
Decreases (20 builds for cc13x2_26x2, cyw30739, esp32, k32w, p6, telink)
Full report (36 builds for cc13x2_26x2, cyw30739, esp32, k32w, linux, mbed, nrfconnect, p6, telink)
|
jmartinez-silabs
approved these changes
Jun 8, 2022
woody-apple
approved these changes
Jun 9, 2022
There was a problem hiding this comment.
@mykrupp I think the credentials bits here are still broken, in a way that is an exploitable security hole.
Comment on lines
+83
to
+84
| EFR32Config::ReadConfigValueBin(EFR32Config::kConfigKey_LockUser, reinterpret_cast<uint8_t *>(&mLockUsers), | ||
| sizeof(EmberAfPluginDoorLockUserInfo) * ArraySize(mLockUsers), outLen); |
There was a problem hiding this comment.
This can still leave part of mLockUsers un-initialized if the size increased, or fail to read if the size decreased.....
Comment on lines
+95
to
+96
| EFR32Config::ReadConfigValueBin(EFR32Config::kConfigKey_UserCredentials, reinterpret_cast<uint8_t *>(mCredentials[0].Get()), | ||
| sizeof(DlCredential) * mMaxUsers * mMaxCredentialsPerUser, outLen); |
There was a problem hiding this comment.
This makes no sense. There are mMaxUsers buffers, each of size mMaxCredentialsPerUser, right? But this is going to read mMaxUsers * mMaxCredentialsPerUser credentials into the first buffer, writing outside its bounds, trashing the heap, and not necessarily filling the other buffers at all. Unless I am missing something? What am I missing?
There was a problem hiding this comment.
I guess I didn't test thoroughly enough here. The members in mCredentials(credentialType and credentialIndex) are only accessed by the getUser command. I was testing this by sending a lock/unlock command after reset.
Since there's no way to pass a memory offset to the ReadConfigValueBin function, I guess each user will need it's own config key.
There was a problem hiding this comment.
Addressed in this PR - b7677b1
| } | ||
|
|
||
| // Check the PIN code | ||
| for (uint8_t i; i < 10; i++) | ||
| for (uint8_t i = 0; i < mMaxCredentialsPerUser; i++) |
There was a problem hiding this comment.
This loop needs to go to MAX_CREDENTIALS, no? If MAX_CREDENTIALS is 20 and mMaxCredentialsPerUser is 5, say, this loop does not do the right thing...
There was a problem hiding this comment.
Addressed in this PR - b7677b1
mkardous-silabs
pushed a commit
to mkardous-silabs/connectedhomeip
that referenced
this pull request
Jun 10, 2022
…ect-chip#19229) * code review comments. Fix mCredentials, fix NVM flash saving * restyle * cleanup * remove unused array * remove TotalCredentials key * Check mMaxUsers against DOOR_LOCK_MAX_USERS
rochaferraz
pushed a commit
to rochaferraz/connectedhomeip
that referenced
this pull request
Jun 15, 2022
…ect-chip#19229) * code review comments. Fix mCredentials, fix NVM flash saving * restyle * cleanup * remove unused array * remove TotalCredentials key * Check mMaxUsers against DOOR_LOCK_MAX_USERS
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.
Problem
Door-lock-server was updated to accommodate multiple users and credentials
Change overview
EFR32 platform now accommodates multiple users and credentials.
Added logic for RequirePINForRemoteOperation
Read maxNumberOfCredentials per user
Store new users and credentials in nvm flash
Fix non-happy path cases(i.e. lock command sent door is already locked)
Testing
This was tested by sending relevant commands affected by the recent door-lock-server changes
./out/chip-tool doorlock set-user 0 1 mike 5 1 0 0 1 1 --timedInteractionTimeoutMs 1000
./out/chip-tool doorlock set-user 0 2 matt 6 1 0 0 1 1 --timedInteractionTimeoutMs 1000
./out/chip-tool doorlock set-user 0 3 nick 7 1 0 0 1 1 --timedInteractionTimeoutMs 1000
// set credential
./out/chip-tool doorlock set-credential 0 '{ "credentialType": 1, "credentialIndex": 1 }' "123456" 1 null null 1 1 --timedInteractionTimeoutMs 1000
./out/chip-tool doorlock set-credential 0 '{ "credentialType": 1, "credentialIndex": 2 }' "123457” 1 null null 1 1 --timedInteractionTimeoutMs 1000
// get user
./out/chip-tool doorlock get-user 1 1 1 --timedInteractionTimeoutMs 1000
Reset EFR32
./out/chip-tool doorlock get-user 1 1 1 --timedInteractionTimeoutMs 1000
See that users and credentials are still available after reset