Fix three out-of-bounds / use-after-free issues (MSP parser, channel name, firmware lookup)

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;

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;
     }
 }
 

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);

src/ui/page_version.c

@@ -405,47 +405,49 @@ 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) {
     char tmp_path[256], sdc_path[256];
     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;