From b9704dcc07d0d0fba509a0a6c685ccfbd4894a2f Mon Sep 17 00:00:00 2001 From: Kevin Foucault Date: Mon, 28 Sep 2026 00:05:05 +0200 Subject: [PATCH 1/3] Reject MSPv2 payloads larger than the receive buffer and bound OSD writes esp32_handler_process_byte() copies the 16-bit payload_size from the MSPv2 header and then stores every payload byte into packet.payload[], which is MSP_PORT_INBUF_SIZE (64) bytes. Nothing rejected a larger size, and the CRC is only checked after the payload, so a single corrupted or malicious frame on the backpack UART could write up to 64 KiB past the buffer into the neighbouring statics before being discarded. Drop the frame as soon as the header announces more than 64 bytes. handle_osd() case 0x03 ("write string") also indexed elrs_osd_overlay [HD_VMAX][HD_HMAX] with the row and column taken straight from the frame. The length was clamped against col, but neither row nor col was checked, and a payload shorter than 4 bytes read past the data received. Check the size and both indices before writing. --- src/core/elrs.c | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-) diff --git a/src/core/elrs.c b/src/core/elrs.c index 3a0356b2..bb4b8e3d 100644 --- a/src/core/elrs.c +++ b/src/core/elrs.c @@ -234,7 +234,12 @@ bool esp32_handler_process_byte(uint8_t c) { packet.flags = header->flags; // reset the offset iterator for re-use in payload below offset = 0; - if (packet.payload_size == 0) + if (packet.payload_size > MSP_PORT_INBUF_SIZE) { + // Would overrun packet.payload (the CRC is only checked after the + // payload), so drop the frame and wait for the next '$'. + LOGE("MSP payload too large (%u > %u), dropping packet", packet.payload_size, MSP_PORT_INBUF_SIZE); + input_state = MSP_IDLE; + } else if (packet.payload_size == 0) input_state = MSP_CHECKSUM_V2_NATIVE; else input_state = MSP_PAYLOAD_V2_NATIVE; @@ -527,6 +532,8 @@ void elrs_clear_osd() { } static void handle_osd(uint8_t payload[], uint8_t size) { + if (size < 1) + return; switch (payload[0]) { case 0x00: // hearbeat break; @@ -540,9 +547,13 @@ static void handle_osd(uint8_t payload[], uint8_t size) { break; case 0x03: // write string { + if (size < 4) + break; uint8_t row = payload[1]; uint8_t col = payload[2]; uint8_t attr = payload[3]; + if (row >= HD_VMAX || col >= HD_HMAX) + break; // HDZero only allows 2 pages of characters (i.e. 512 characters) int page = (attr & 1) * 256; uint8_t len = size - 4 < HD_HMAX - col ? size - 4 : HD_HMAX - col; From 3b15722f7842b6dd8676600a772eaaa486fbb2f3 Mon Sep 17 00:00:00 2001 From: Kevin Foucault Date: Sun, 11 Oct 2026 03:14:43 +0200 Subject: [PATCH 2/3] Range-check channel settings on load and in channel2str() settings_load() reads scan.channel, source.hdzero_band and source.analog_channel from setting.ini without fully checking them: - analog_channel above ANALOG_CHANNEL_NUM reset scan.channel instead of analog_channel (copy-paste slip), and 0 was not rejected; - scan.channel 0 was not rejected; - hdzero_band was not checked at all. These values index the channel name tables in channel2str(): analog_channel 0 reads analog_channel_name[-1], and an hdzero_band other than 0 or 1 reads past hdzero_channel_name[2][]. Read the three of them through settings_get_long_bounded(), which checks the long returned by ini_getl() before it is narrowed to the field's type (257 stored in the uint8_t analog_channel would otherwise pass as 1), and falls back to the default when out of range. scan.channel is now read after hdzero_band, as HDZERO_CHANNEL_NUM depends on it. channel2str() also guards its analog index like the HDZero branch already does. --- src/core/osd.c | 8 +++++++- src/core/settings.c | 23 ++++++++++++----------- 2 files changed, 19 insertions(+), 12 deletions(-) diff --git a/src/core/osd.c b/src/core/osd.c index e4c65fdc..fcfc2cb6 100644 --- a/src/core/osd.c +++ b/src/core/osd.c @@ -389,7 +389,13 @@ char *channel2str(uint8_t is_hdzero, uint8_t is_lowband, uint8_t channel) // cha else return hdzero_channel_name[is_lowband][0]; } else { - return analog_channel_name[channel - 1]; + // channel comes from source.analog_channel, read from the settings file; + // an out-of-range value (e.g. 0) would index outside analog_channel_name. + static char unknown_channel_name[] = "--"; + if ((channel > 0) && (channel <= ANALOG_CHANNEL_NUM)) + return analog_channel_name[channel - 1]; + else + return unknown_channel_name; } } diff --git a/src/core/settings.c b/src/core/settings.c index b2245972..ae18361c 100644 --- a/src/core/settings.c +++ b/src/core/settings.c @@ -309,6 +309,13 @@ int settings_put_bool(char *section, char *key, bool value) { return ini_puts(section, key, value ? "true" : "false", SETTING_INI); } +// Checked on the long returned by ini_getl(), before it is narrowed to the +// field's type: 257 stored in a uint8_t would otherwise pass as 1. +static long settings_get_long_bounded(char *section, char *key, long default_val, long min, long max) { + long value = ini_getl(section, key, default_val, SETTING_INI); + return (value < min || value > max) ? default_val : value; +} + void settings_reset(void) { char buf[256]; @@ -342,9 +349,6 @@ void settings_load(void) { // Start with a fully configured structure then update! memcpy(&g_setting, &g_setting_defaults, sizeof(g_setting)); - // scan - g_setting.scan.channel = ini_getl("scan", "channel", g_setting_defaults.scan.channel, SETTING_INI); - // fans g_setting.fans.auto_mode = settings_get_bool("fans", "auto", g_setting_defaults.fans.auto_mode); g_setting.fans.top_speed = ini_getl("fans", "top_speed", g_setting_defaults.fans.top_speed, SETTING_INI); @@ -354,15 +358,12 @@ void settings_load(void) { // source g_setting.source.analog_format = ini_getl("source", "analog_format", g_setting_defaults.source.analog_format, SETTING_INI); g_setting.source.analog_ratio = ini_getl("source", "analog_ratio", g_setting_defaults.source.analog_ratio, SETTING_INI); - g_setting.source.hdzero_band = ini_getl("source", "hdzero_band", g_setting_defaults.source.hdzero_band, SETTING_INI); + g_setting.source.hdzero_band = settings_get_long_bounded("source", "hdzero_band", g_setting_defaults.source.hdzero_band, + SETTING_SOURCES_HDZERO_BAND_RACEBAND, SETTING_SOURCES_HDZERO_BAND_LOWBAND); + // scan, read after hdzero_band: HDZERO_CHANNEL_NUM depends on it + g_setting.scan.channel = settings_get_long_bounded("scan", "channel", g_setting_defaults.scan.channel, 1, HDZERO_CHANNEL_NUM); g_setting.source.hdzero_bw = ini_getl("source", "hdzero_bw", g_setting_defaults.source.hdzero_bw, SETTING_INI); - g_setting.source.analog_channel = ini_getl("source", "analog_channel", g_setting_defaults.source.analog_channel, SETTING_INI); - if (g_setting.scan.channel > HDZERO_CHANNEL_NUM) { - g_setting.scan.channel = 1; - } - if (g_setting.source.analog_channel > ANALOG_CHANNEL_NUM) { - g_setting.scan.channel = 33; - } + g_setting.source.analog_channel = settings_get_long_bounded("source", "analog_channel", g_setting_defaults.source.analog_channel, 1, ANALOG_CHANNEL_NUM); // autoscan g_setting.autoscan.status = ini_getl("autoscan", "status", g_setting_defaults.autoscan.status, SETTING_INI); From a6ae23c116aab9b3fcce215688920c29aaccac5a Mon Sep 17 00:00:00 2001 From: Kevin Foucault Date: Mon, 28 Sep 2026 00:05:05 +0200 Subject: [PATCH 3/3] Fix use of dirent name after closedir() in page_version_find_latest_fw() The function kept a pointer to entry->d_name, called closedir() and returned that pointer. d_name lives in the DIR's internal buffer: it is reused as readdir() refills it (so comparing against an earlier entry was already unreliable) and freed by closedir(). The caller then formatted a path from freed memory. It happened to work because the allocator had not reused the block yet. Copy the winning name into a caller-provided buffer before closing the directory. --- src/ui/page_version.c | 34 ++++++++++++++++++---------------- 1 file changed, 18 insertions(+), 16 deletions(-) diff --git a/src/ui/page_version.c b/src/ui/page_version.c index ced67aa7..872888ce 100644 --- a/src/ui/page_version.c +++ b/src/ui/page_version.c @@ -405,26 +405,27 @@ int generate_current_version(sys_version_t *sys_ver) { return 0; } -static const char *page_version_find_latest_fw(const char *path) { - char *dname = NULL; +// Copies the name of the highest-versioned sub-directory of `path` into +// `latest` and returns true, or returns false if there is none. The name is +// copied because entry->d_name points into the DIR's internal buffer: it may +// be reused by the next readdir() call and is freed by closedir(). +static bool page_version_find_latest_fw(const char *path, char *latest, size_t size) { + bool found = false; DIR *dir = opendir(path); if (dir) { struct dirent *entry = readdir(dir); while (entry != NULL) { if (entry->d_type == DT_DIR) { - if (dname != NULL) { - if (str_compare_versions(entry->d_name, dname) > 0) { - dname = entry->d_name; - } - } else { - dname = entry->d_name; + if (!found || str_compare_versions(entry->d_name, latest) > 0) { + snprintf(latest, size, "%s", entry->d_name); + found = true; } } entry = readdir(dir); } closedir(dir); } - return dname; + return found; } static int page_version_get_latest_fw_path(const char *device, char *path, size_t size) { @@ -432,20 +433,21 @@ static int page_version_get_latest_fw_path(const char *device, char *path, size_ snprintf(tmp_path, sizeof(tmp_path), "/tmp/FIRMWARE/%s", device); snprintf(sdc_path, sizeof(sdc_path), "/mnt/extsd/FIRMWARE/%s", device); - const char *tmp_latest = page_version_find_latest_fw(tmp_path); - const char *sdc_latest = page_version_find_latest_fw(sdc_path); + char tmp_latest[256], sdc_latest[256]; + bool has_tmp = page_version_find_latest_fw(tmp_path, tmp_latest, sizeof(tmp_latest)); + bool has_sdc = page_version_find_latest_fw(sdc_path, sdc_latest, sizeof(sdc_latest)); - if (tmp_latest && sdc_latest) { + if (has_tmp && has_sdc) { if (str_compare_versions(tmp_latest, sdc_latest) > 0) { - sdc_latest = NULL; + has_sdc = false; } else { - tmp_latest = NULL; + has_tmp = false; } } - if (tmp_latest) { + if (has_tmp) { return snprintf(path, size, "%s/%s", tmp_path, tmp_latest); - } else if (sdc_latest) { + } else if (has_sdc) { return snprintf(path, size, "%s/%s", sdc_path, sdc_latest); } else { return 0;