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

[RSDK-9927] increase min data needed before attempting parse firmware header #412

Merged
merged 3 commits into from
Feb 7, 2025
Merged

[RSDK-9927] increase min data needed before attempting parse firmware header #412

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
1ff86e9
increase min data needed before attempting parse firmware header
mattjperez Feb 5, 2025
26ecca7
fix native part, add extra check on target size before downloading
mattjperez Feb 7, 2025
5f51bcb
add clarifying comments
mattjperez Feb 7, 2025
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
24 changes: 19 additions & 5 deletions micro-rdk/src/common/ota.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,10 @@ use crate::{
#[cfg(feature = "esp32")]
use crate::esp32::esp_idf_svc::{
ota::{EspFirmwareInfoLoader, EspOta},
sys::{esp_ota_get_next_update_partition, esp_partition_t},
sys::{
esp_app_desc_t, esp_image_header_t, esp_image_segment_header_t,
esp_ota_get_next_update_partition, esp_partition_t,
},
};
use async_io::Timer;
use futures_lite::{FutureExt, StreamExt};
Expand All @@ -64,7 +67,18 @@ use {bincode::Decode, futures_lite::AsyncWriteExt};
const CONN_RETRY_SECS: u64 = 1;
const NUM_RETRY_CONN: usize = 3;
const DOWNLOAD_TIMEOUT_SECS: u64 = 30;
const SIZEOF_APPDESC: usize = 256;
Copy link
Member

Choose a reason for hiding this comment

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

looks like this is still being referenced in L. 481 (in the native case), probably meant to replace with FIRMWARE_HEADER_SIZE?

Copy link
Member

Choose a reason for hiding this comment

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

+1


/// https://github.com/esp-rs/esp-idf-svc/blob/4ccf3182b32129b55082b5810d837a1cf5bc1a08/src/ota.rs#L94
/// https://github.com/espressif/esp-idf/commit/3b9cb25fe18c5a6ed64ddd6a1dc4d0ce0b6cdc2a
#[cfg(feature = "esp32")]
static FIRMWARE_HEADER_SIZE: Lazy<usize> = Lazy::new(|| {
std::mem::size_of::<esp_image_header_t>()
+ std::mem::size_of::<esp_image_segment_header_t>()
Copy link
Member

Choose a reason for hiding this comment

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

IIUC you are using this to compute whether the frame you got contains enough data to get you to the esp_app_desc_t. But is there an assumption here that the first segment will be the DROM segment which contains the esp_app_desc_t? Could the segments be in another order?

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't believe so, not for something valid that goes directly into an app partition at least. This was pulled directly from esp-idf-svc's FirmwareInfoLoader.

Found a seemingly relevant commit comment as well.

The esp_app_desc_t is located in fixed place in start of ROM secotor. It is located after structures esp_image_header_t and esp_image_segment_header_t.

Copy link
Member Author

Choose a reason for hiding this comment

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

side note: this is probably why the outputs of my handrolled struct at the top was giving me strange results. I was parsing from the image header and segment header up front, not the app_desc_t like I thought

Copy link
Member

Choose a reason for hiding this comment

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

Thanks for checking. It might be worth linking to those resources in a comment on this constant.

Meanwhile, were there any checks that you wanted to make with that struct that you had backed away from doing because you weren't getting sensible results? Would you want to re-introduce them now that this issue has been identified and fixed?

Copy link
Member Author

Choose a reason for hiding this comment

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

not at the minute. There's already an embedded ticket that's backlogged. I'll add this thread to the ticket for context when we get to it.

+ std::mem::size_of::<esp_app_desc_t>()
});
#[cfg(not(feature = "esp32"))]
const FIRMWARE_HEADER_SIZE: &usize = &1024;

const MAX_VER_LEN: usize = 128;
pub const OTA_MODEL_TYPE: &str = "ota_service";
pub static OTA_MODEL_TRIPLET: Lazy<String> =
Expand Down Expand Up @@ -397,7 +411,7 @@ impl<S: OtaMetadataStorage> OtaService<S> {
.parse::<usize>()
.map_err(|e| OtaError::Other(e.to_string()))?;

if file_len > self.max_size {
if file_len > self.max_size || file_len < *FIRMWARE_HEADER_SIZE {
return Err(OtaError::InvalidImageSize(file_len, self.max_size));
}

Expand Down Expand Up @@ -442,7 +456,7 @@ impl<S: OtaMetadataStorage> OtaService<S> {
total_downloaded += data.len();

if !got_info {
if total_downloaded < SIZEOF_APPDESC {
if total_downloaded < *FIRMWARE_HEADER_SIZE {
Copy link
Member

Choose a reason for hiding this comment

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

Micro-nit: Above, we set the max frame size to 16k. If, somehow FIRMWARE_HEADER_SIZE was > 16k, then this would always fail. Maybe the max frame size should be max(16k, FIRMWARE_HEADER_SIZE).

Copy link
Member Author

@mattjperez mattjperez Feb 7, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

I think it's safe to assume (as we're only supporting esp32 at the moment) that a valid header will always be <16K.

We could calculate it out (esp_image_header_t + esp_image_segment_header_t + esp_app_desc_t) and set a magic number accordingly, but I think sizeof provides more context on what we're actually doing. I used 1024 for non-esp32 targets arbitrarily since it's greater than the three types above and it won't have access to the esp_idf_* types.

Also, I'm reluctant to set something greater than the 16K as max_frame_size since its documentation stated it was the max and would need to look deeper to see what the effects of setting it higher would be inside hyper.

log::error!("initial frame too small to retrieve esp_app_desc_t");
} else {
#[cfg(feature = "esp32")]
Expand All @@ -468,7 +482,7 @@ impl<S: OtaMetadataStorage> OtaService<S> {
EspAppDesc,
bincode::config::Configuration,
>(
&data[..SIZEOF_APPDESC],
&data[..*FIRMWARE_HEADER_SIZE],
bincode::config::standard(),
) {
log::debug!("{:?}", decoded.0);
Expand Down