Fix double free error when using signed images without a secure boot. (IDFGH-4376) #6210
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
- If the signed images are enabled and image verification occurrs after an upgrade in the app itself the the doulbe free takes place. - Double free occurs due to the fact that `verify_secure_boot_signature()` already frees the `sha_handle` pointer. - If the `verify_secure_boot_signature()` does not return `ESP_OK` (for example, when new image is not signed) then the code jums to the `err` label which basically frees the `sha_handle` if it's not NULL causing a double-free. - I've decided to set the `sha_handle` to NULL after `verify_secure_boot_signature()` in the non-bootloader mode fixing the double-free. I didn't do this change for bootloader build because all the bootloader functions that use `sha_handle` expect it to be non-NULL. - I think the better solution is to pass a sha_handle by reference and NULL it inside the `verify_secure_boot_signature()` if needed but it's a risky change and I don't all the required hardware to test it.
|
|
github-actions
bot
changed the title
|
Thanks for your contribution. |
|
Hi @Morozov-5F, Thanks again for sending this fix. We've submitted the same fix internally for merging, but with a small tweak (applied for both bootloader & app). In the bootloader, a non-null sha_handle value means the calculation is still ongoing in the hardware so in this case it's correct for it to be nulled out at this point as well - the only difference is that the bootloader doesn't crash when it's incorrectly left as non-null. This PR will be updated once the fix merges to master. Angus |
|
Thanks @projectgus, that's a great news! I wasn't sure about the bootloader part so thank you for clarification too. |
espressif-bot
pushed a commit
that referenced
this pull request
Jan 9, 2021
sha_handle is "finished" when verify_secure_boot_signature() returns and should be nulled out. Alternative version of fix submitted in #6210 Closes #6210 Signed-off-by: Angus Gratton <[email protected]>
espressif-bot
pushed a commit
that referenced
this pull request
Feb 25, 2021
sha_handle is "finished" when verify_secure_boot_signature() returns and should be nulled out. Alternative version of fix submitted in #6210 Closes #6210 Signed-off-by: Angus Gratton <[email protected]>
Morozov-5F
deleted the
bugfix/fix-double-free-if-signed-images-used-without-secure-boot
branch
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.
verify_secure_boot_signature()already frees thesha_handlepointer.verify_secure_boot_signature()does not returnESP_OK(for example, when new image is not signed) then the code jumps to theerrlabel which basically frees thesha_handleif it's not NULL causing a double-free.sha_handleto NULL afterverify_secure_boot_signature()in the non-bootloader mode fixing the double-free. I didn't do this change for bootloader build because all the bootloader functions that usesha_handleexpect it to be non-NULL.sha_handleby reference and NULL it inside theverify_secure_boot_signature()if needed but it's a risky change and I don't have all the required hardware to test it.