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

New implementation of RTC retain mem custom implementation is not backwards compatible. (IDFGH-11747) #12849

Closed
not-my-real-name opened this issue Dec 20, 2023 · 4 comments
Assignees
Labels
Resolution: NA Issue resolution is unavailable Status: Done Issue is done internally

Comments

@not-my-real-name
Copy link

Hi Everyone,

I've noticed a backwards compatibility issue with the rtc retain memory. I'm using the custom memory to send a small amount of data between my custom bootloader hooks and the main application.

Here is the old implementation:

return esp_rom_crc32_le(UINT32_MAX, (uint8_t*)rtc_retain_mem, sizeof(rtc_retain_mem_t) - sizeof(rtc_retain_mem->crc)) == rtc_retain_mem->crc && rtc_retain_mem->crc != UINT32_MAX;

Here is the new implementation:

return esp_rom_crc32_le(UINT32_MAX, (uint8_t*)rtc_retain_mem, rtc_retain_mem_size()) == rtc_retain_mem->crc && rtc_retain_mem->crc != UINT32_MAX;

The new implementation is subtly different in that it uses a function to get the size, rather than sizeof(). It includes the following explanation:
/* A custom memory has been reserved by the user, do not consider this memory into CRC calculation as it may change without * the have the user updating the CRC. Return the offset of the custom field, which is equivalent to size of the structure * minus the size of everything after (including) custom */

This is a problem for me because the application can be updated without the bootloader changing, so they can be from incompatible IDF versions.

I missed this breaking change and have units out there that I'm going to have to patch around this change. I can do it, so its not the end of the world, but its a bit of a pain.

Can I suggest to the team that you add a menu config option for maintaining backwards compatability here?

Something like CONFIG_BOOTLOADER_CUSTOM_RESERVE_RTC_LEGACY_CRC?

Link to forum topic and further details if required.
(https://www.esp32.com/viewtopic.php?f=13&t=26557&p=125084#p125084)

@espressif-bot espressif-bot added the Status: Opened Issue is new label Dec 20, 2023
@github-actions github-actions bot changed the title New implementation of RTC retain mem custom implementation is not backwards compatible. New implementation of RTC retain mem custom implementation is not backwards compatible. (IDFGH-11747) Dec 20, 2023
@o-marshmallow
Copy link
Collaborator

Hello @not-my-real-name ,

Which ESP-IDF versions exactly are you working with? I guess that your bootloader and application used to be in v4.3, but you want to update your application to v4.4?

If I understand correctly, your current bootloader is compiled from ESP-IDF v4.3, with CONFIG_BOOTLOADER_CUSTOM_RESERVE_RTC enabled, so the CRC calculation takes into account the custom data.
But you want to update your application to use ESP-IDF v4.4, where custom data is not taken into account anymore for the CRC calculation, right?

One solution would be to trigger a recaculation of the CRC as soon as you enter your app_main, which won't take into account the custom data. Afterwards, in your application you can freely use the bootloader_support API:

#include "esp_image_format.h"
#include "esp_rom_crc.h"

void recalculate_crc(void)
{
    extern rtc_retain_mem_t *const rtc_retain_mem;
    rtc_retain_mem->crc = esp_rom_crc32_le(UINT32_MAX, (uint8_t*)rtc_retain_mem, offsetof(rtc_retain_mem_t, custom));
}

void app_main() {
    recalculate_crc();
    /* Use the bootloader_support API */
}

@not-my-real-name
Copy link
Author

Hi Omar,
Thanks for taking the time to reply.

I am the lead developer in a commercial project. We have a few thousand devices in service. We've used lots of different points in the IDF, but are currently on a 4.4.5 based branch.

Yes, you are correct. This issue is devices that are in the field, with secure boot enabled that were released several years ago with a bootloader based on IDF 4.3 or older. The firmware updates fine to newer versions build with IDF4.4.5, but the checksum does not compute in the application because it assumes the bootloader is a new one.

I appreciate you coding out a suggested work around, but I don't think its going to work for me. All this is going to do is force the CRC to compute, regardless of the validity of the data. The data could be completely corrupt before you re-compute and it would just compute a new checksum based on the corrupted values. This would hide the problem and lead to corrupt data being used in the application.

I do have a work around for this now and it covers most of my issues. If the api indicates the checksum is invalid, I work out the checksum the old way manually (including the custom). If this computes, I know I can trust the data in the custom array even though the normal checksum methods do not compute. I just have to be very careful which IDF functions I call as some will reset the memory on checksum failure.

If a backwards compatability flag was introduced (and I had realised at the time!) I could have forced my new IDF code to build the bootloader for new devices, and new versions of the app with the legacy checksum implementation. To be honest I prefer this implementation as it guarantees the integrity of the custom memory as well. I've since introduced a secondary checksum in some free bytes in the custom memory.

Actually, an alternate solution would be to modify the routines in the main application to do what my code is doing, and work the checksum out both ways and trust it if either computes. That would be even better, although the tidy way to do that would have been a version field in the rtc retain memory structure.

You can't safely update the bootloader on a device using secure boot v2 in production mode, although I have just developed code to do just this. The issue is if it fails during the update the device is totally dead.

@espressif-bot espressif-bot added Status: In Progress Work is in progress Status: Reviewing Issue is being reviewed and removed Status: Opened Issue is new Status: In Progress Work is in progress labels Dec 27, 2023
@o-marshmallow
Copy link
Collaborator

o-marshmallow commented Dec 28, 2023

Hello @not-my-real-name ,

Can you please try this patch on your end? It should add a new configuration option, BOOTLOADER_CUSTOM_RESERVE_RTC_IN_CRC, that enables the legacy behavior for the CRC calculation:
rtc_retain_mem_legacy.diff.txt

If it works properly for you, we will merge it internally to ESP-IDF v4.4.

@not-my-real-name
Copy link
Author

Thanks @o-marshmallow

I've applied the patch and looked through the changes and it looks good. I'm not able to build/test it right now, but will report back as soon as I am able to.

@espressif-bot espressif-bot added Status: Done Issue is done internally Resolution: NA Issue resolution is unavailable and removed Status: Reviewing Issue is being reviewed labels Feb 21, 2024
espressif-bot pushed a commit that referenced this issue Mar 1, 2024
* Closes #12849

In former versions of ESP-IDF, the user custom memory data in the retained memory
was taken into account during the CRC calculation. This was changed in a later
commit, the custom memory was ignored, therefore this can seen as a breaking change.
This commit gives the possibility to choose between the former (legacy) or
new way of calculating the CRC.
espressif-bot pushed a commit that referenced this issue Mar 5, 2024
* Closes #12849

In former versions of ESP-IDF, the user custom memory data in the retained memory
was taken into account during the CRC calculation. This was changed in a later
commit, the custom memory was ignored, therefore this can seen as a breaking change.
This commit gives the possibility to choose between the former (legacy) or new way of calculating the CRC.
espressif-bot pushed a commit that referenced this issue Mar 5, 2024
* Closes #12849

In former versions of ESP-IDF, the user custom memory data in the retained memory
was taken into account during the CRC calculation. This was changed in a later
commit, the custom memory was ignored, therefore this can seen as a breaking change.
This commit gives the possibility to choose between the former (legacy) or
new way of calculating the CRC.
espressif-bot pushed a commit that referenced this issue Apr 17, 2024
* Closes #12849

In former versions of ESP-IDF, the user custom memory data in the retained memory
was taken into account during the CRC calculation. This was changed in a later
commit, the custom memory was ignored, therefore this can seen as a breaking change.
This commit gives the possibility to choose between the former (legacy) or
new way of calculating the CRC.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Resolution: NA Issue resolution is unavailable Status: Done Issue is done internally
Projects
None yet
Development

No branches or pull requests

3 participants