[FL-3759] Fix expansion protocol crash when fed lots of garbage (#3409)

* Fix crash caused by garbage input
* Add unit tests for garbage input
* Enable applications to disable and then restore expansion module support
* GPIO App: disable expansion on app start and re-enable on exit

Co-authored-by: Aleksandr Kutuzov <alleteam@gmail.com>

applications/debug/expansion_test/expansion_test.c

@@ -100,17 +100,19 @@ static void expansion_test_app_start(ExpansionTestApp* instance) {
     // Configure the serial port
     furi_hal_serial_init(instance->handle, EXPANSION_PROTOCOL_DEFAULT_BAUD_RATE);
     // Start waiting for the initial pulse
-    expansion_enable(instance->expansion, HOST_SERIAL_ID);
+    expansion_set_listen_serial(instance->expansion, HOST_SERIAL_ID);
 
     furi_hal_serial_async_rx_start(
         instance->handle, expansion_test_app_serial_rx_callback, instance, false);
 }
 
 static void expansion_test_app_stop(ExpansionTestApp* instance) {
+    // Disable expansion module support
+    expansion_disable(instance->expansion);
     // Give back the module handle
     furi_hal_serial_control_release(instance->handle);
-    // Turn expansion module support off
-    expansion_disable(instance->expansion);
+    // Restore expansion user settings
+    expansion_enable(instance->expansion);
     furi_record_close(RECORD_EXPANSION);
 }
 

applications/debug/unit_tests/expansion/expansion_test.c

@@ -1,8 +1,14 @@
 #include "../minunit.h"
 
 #include <furi.h>
+#include <furi_hal_random.h>
+
 #include <expansion/expansion_protocol.h>
 
+#define EXPANSION_TEST_GARBAGE_MAGIC (0xB19AF)
+#define EXPANSION_TEST_GARBAGE_BUF_SIZE (0x100U)
+#define EXPANSION_TEST_GARBAGE_ITERATIONS (100U)
+
 MU_TEST(test_expansion_encoded_size) {
     ExpansionFrame frame = {};
 
@@ -28,43 +34,62 @@ MU_TEST(test_expansion_encoded_size) {
 MU_TEST(test_expansion_remaining_size) {
     ExpansionFrame frame = {};
 
-    mu_assert_int_eq(1, expansion_frame_get_remaining_size(&frame, 0));
+    size_t remaining_size;
+    mu_check(expansion_frame_get_remaining_size(&frame, 0, &remaining_size));
+    mu_assert_int_eq(1, remaining_size);
 
     frame.header.type = ExpansionFrameTypeHeartbeat;
-    mu_assert_int_eq(1, expansion_frame_get_remaining_size(&frame, 0));
-    mu_assert_int_eq(0, expansion_frame_get_remaining_size(&frame, 1));
-    mu_assert_int_eq(0, expansion_frame_get_remaining_size(&frame, 100));
+    mu_check(expansion_frame_get_remaining_size(&frame, 0, &remaining_size));
+    mu_assert_int_eq(1, remaining_size);
+    mu_check(expansion_frame_get_remaining_size(&frame, 1, &remaining_size));
+    mu_assert_int_eq(0, remaining_size);
+    mu_check(expansion_frame_get_remaining_size(&frame, 100, &remaining_size));
+    mu_assert_int_eq(0, remaining_size);
 
     frame.header.type = ExpansionFrameTypeStatus;
-    mu_assert_int_eq(1, expansion_frame_get_remaining_size(&frame, 0));
-    mu_assert_int_eq(1, expansion_frame_get_remaining_size(&frame, 1));
-    mu_assert_int_eq(0, expansion_frame_get_remaining_size(&frame, 2));
-    mu_assert_int_eq(0, expansion_frame_get_remaining_size(&frame, 100));
+    mu_check(expansion_frame_get_remaining_size(&frame, 0, &remaining_size));
+    mu_assert_int_eq(1, remaining_size);
+    mu_check(expansion_frame_get_remaining_size(&frame, 1, &remaining_size));
+    mu_assert_int_eq(1, remaining_size);
+    mu_check(expansion_frame_get_remaining_size(&frame, 2, &remaining_size));
+    mu_assert_int_eq(0, remaining_size);
+    mu_check(expansion_frame_get_remaining_size(&frame, 100, &remaining_size));
+    mu_assert_int_eq(0, remaining_size);
 
     frame.header.type = ExpansionFrameTypeBaudRate;
-    mu_assert_int_eq(1, expansion_frame_get_remaining_size(&frame, 0));
-    mu_assert_int_eq(4, expansion_frame_get_remaining_size(&frame, 1));
-    mu_assert_int_eq(0, expansion_frame_get_remaining_size(&frame, 5));
-    mu_assert_int_eq(0, expansion_frame_get_remaining_size(&frame, 100));
+    mu_check(expansion_frame_get_remaining_size(&frame, 0, &remaining_size));
+    mu_assert_int_eq(1, remaining_size);
+    mu_check(expansion_frame_get_remaining_size(&frame, 1, &remaining_size));
+    mu_assert_int_eq(4, remaining_size);
+    mu_check(expansion_frame_get_remaining_size(&frame, 5, &remaining_size));
+    mu_assert_int_eq(0, remaining_size);
+    mu_check(expansion_frame_get_remaining_size(&frame, 100, &remaining_size));
+    mu_assert_int_eq(0, remaining_size);
 
     frame.header.type = ExpansionFrameTypeControl;
-    mu_assert_int_eq(1, expansion_frame_get_remaining_size(&frame, 0));
-    mu_assert_int_eq(1, expansion_frame_get_remaining_size(&frame, 1));
-    mu_assert_int_eq(0, expansion_frame_get_remaining_size(&frame, 2));
-    mu_assert_int_eq(0, expansion_frame_get_remaining_size(&frame, 100));
+    mu_check(expansion_frame_get_remaining_size(&frame, 0, &remaining_size));
+    mu_assert_int_eq(1, remaining_size);
+    mu_check(expansion_frame_get_remaining_size(&frame, 1, &remaining_size));
+    mu_assert_int_eq(1, remaining_size);
+    mu_check(expansion_frame_get_remaining_size(&frame, 2, &remaining_size));
+    mu_assert_int_eq(0, remaining_size);
+    mu_check(expansion_frame_get_remaining_size(&frame, 100, &remaining_size));
+    mu_assert_int_eq(0, remaining_size);
 
     frame.header.type = ExpansionFrameTypeData;
     frame.content.data.size = EXPANSION_PROTOCOL_MAX_DATA_SIZE;
-    mu_assert_int_eq(1, expansion_frame_get_remaining_size(&frame, 0));
-    mu_assert_int_eq(1, expansion_frame_get_remaining_size(&frame, 1));
-    mu_assert_int_eq(
-        EXPANSION_PROTOCOL_MAX_DATA_SIZE, expansion_frame_get_remaining_size(&frame, 2));
+    mu_check(expansion_frame_get_remaining_size(&frame, 0, &remaining_size));
+    mu_assert_int_eq(1, remaining_size);
+    mu_check(expansion_frame_get_remaining_size(&frame, 1, &remaining_size));
+    mu_assert_int_eq(1, remaining_size);
+    mu_check(expansion_frame_get_remaining_size(&frame, 2, &remaining_size));
+    mu_assert_int_eq(EXPANSION_PROTOCOL_MAX_DATA_SIZE, remaining_size);
     for(size_t i = 0; i <= EXPANSION_PROTOCOL_MAX_DATA_SIZE; ++i) {
-        mu_assert_int_eq(
-            EXPANSION_PROTOCOL_MAX_DATA_SIZE - i,
-            expansion_frame_get_remaining_size(&frame, i + 2));
+        mu_check(expansion_frame_get_remaining_size(&frame, i + 2, &remaining_size));
+        mu_assert_int_eq(EXPANSION_PROTOCOL_MAX_DATA_SIZE - i, remaining_size);
     }
-    mu_assert_int_eq(0, expansion_frame_get_remaining_size(&frame, 100));
+    mu_check(expansion_frame_get_remaining_size(&frame, 100, &remaining_size));
+    mu_assert_int_eq(0, remaining_size);
 }
 
 typedef struct {
@@ -145,10 +170,28 @@ MU_TEST(test_expansion_encode_decode_frame) {
     mu_assert_mem_eq(&frame_in, &frame_out, encoded_size);
 }
 
+MU_TEST(test_expansion_garbage_input) {
+    uint8_t garbage_data[EXPANSION_TEST_GARBAGE_BUF_SIZE];
+    for(uint32_t i = 0; i < EXPANSION_TEST_GARBAGE_ITERATIONS; ++i) {
+        furi_hal_random_fill_buf(garbage_data, sizeof(garbage_data));
+        size_t remaining_size = EXPANSION_TEST_GARBAGE_MAGIC;
+        if(expansion_frame_get_remaining_size(
+               (ExpansionFrame*)garbage_data, sizeof(garbage_data), &remaining_size)) {
+            // If by chance the garbage data is a valid frame, then the result
+            // must be 0 because the amount of data provided is more than enough
+            mu_assert_int_eq(0, remaining_size);
+        } else {
+            // If the frame is invalid, the remaining_size parameter should be untouched
+            mu_assert_int_eq(EXPANSION_TEST_GARBAGE_MAGIC, remaining_size);
+        }
+    }
+}
+
 MU_TEST_SUITE(test_expansion_suite) {
     MU_RUN_TEST(test_expansion_encoded_size);
     MU_RUN_TEST(test_expansion_remaining_size);
     MU_RUN_TEST(test_expansion_encode_decode_frame);
+    MU_RUN_TEST(test_expansion_garbage_input);
 }
 
 int run_minunit_test_expansion() {

applications/main/gpio/gpio_app.c

@@ -24,6 +24,9 @@ static void gpio_app_tick_event_callback(void* context) {
 GpioApp* gpio_app_alloc() {
     GpioApp* app = malloc(sizeof(GpioApp));
 
+    app->expansion = furi_record_open(RECORD_EXPANSION);
+    expansion_disable(app->expansion);
+
     app->gui = furi_record_open(RECORD_GUI);
     app->gpio_items = gpio_items_alloc();
 
@@ -70,12 +73,7 @@ GpioApp* gpio_app_alloc() {
         GpioAppViewUsbUartCfg,
         variable_item_list_get_view(app->var_item_list));
 
-    if(furi_hal_serial_control_is_busy(FuriHalSerialIdUsart) ||
-       furi_hal_serial_control_is_busy(FuriHalSerialIdLpuart)) {
-        scene_manager_next_scene(app->scene_manager, GpioSceneErrorExpansion);
-    } else {
-        scene_manager_next_scene(app->scene_manager, GpioSceneStart);
-    }
+    scene_manager_next_scene(app->scene_manager, GpioSceneStart);
 
     return app;
 }
@@ -104,6 +102,9 @@ void gpio_app_free(GpioApp* app) {
     furi_record_close(RECORD_GUI);
     furi_record_close(RECORD_NOTIFICATION);
 
+    expansion_enable(app->expansion);
+    furi_record_close(RECORD_EXPANSION);
+
     gpio_items_free(app->gpio_items);
     free(app);
 }

applications/main/gpio/gpio_app_i.h

@@ -17,8 +17,10 @@
 #include "views/gpio_test.h"
 #include "views/gpio_usb_uart.h"
 #include <assets_icons.h>
+#include <expansion/expansion.h>
 
 struct GpioApp {
+    Expansion* expansion;
     Gui* gui;
     NotificationApp* notifications;
     ViewDispatcher* view_dispatcher;

applications/main/gpio/scenes/gpio_scene_config.h

@@ -3,5 +3,4 @@ ADD_SCENE(gpio, test, Test)
 ADD_SCENE(gpio, usb_uart, UsbUart)
 ADD_SCENE(gpio, usb_uart_cfg, UsbUartCfg)
 ADD_SCENE(gpio, usb_uart_close_rpc, UsbUartCloseRpc)
-ADD_SCENE(gpio, error_expansion, ErrorExpansion)
 ADD_SCENE(gpio, exit_confirm, ExitConfirm)

applications/main/gpio/scenes/gpio_scene_error_expansion.c

@@ -1,43 +0,0 @@
-#include "../gpio_app_i.h"
-#include "../gpio_custom_event.h"
-
-void gpio_scene_error_expansion_on_enter(void* context) {
-    GpioApp* app = context;
-
-    widget_add_icon_element(app->widget, 78, 0, &I_ActiveConnection_50x64);
-    widget_add_string_multiline_element(
-        app->widget, 3, 2, AlignLeft, AlignTop, FontPrimary, "Ext. Module\nis connected!");
-    widget_add_string_multiline_element(
-        app->widget,
-        3,
-        30,
-        AlignLeft,
-        AlignTop,
-        FontSecondary,
-        "Disconnect External\n"
-        "Module\n"
-        "to use this function.");
-
-    view_dispatcher_switch_to_view(app->view_dispatcher, GpioAppViewUsbUartCloseRpc);
-}
-
-bool gpio_scene_error_expansion_on_event(void* context, SceneManagerEvent event) {
-    GpioApp* app = context;
-    bool consumed = false;
-
-    if(event.type == SceneManagerEventTypeCustom) {
-        if(event.event == GpioCustomEventErrorBack) {
-            if(!scene_manager_previous_scene(app->scene_manager)) {
-                scene_manager_stop(app->scene_manager);
-                view_dispatcher_stop(app->view_dispatcher);
-            }
-            consumed = true;
-        }
-    }
-    return consumed;
-}
-
-void gpio_scene_error_expansion_on_exit(void* context) {
-    GpioApp* app = context;
-    widget_reset(app->widget);
-}

applications/services/expansion/expansion.c

@@ -394,30 +394,18 @@ void expansion_on_system_start(void* arg) {
     Expansion* instance = expansion_alloc();
     furi_record_create(RECORD_EXPANSION, instance);
 
-    ExpansionSettings settings = {};
-    if(!expansion_settings_load(&settings)) {
-        expansion_settings_save(&settings);
-    } else if(settings.uart_index < FuriHalSerialIdMax) {
-        expansion_enable(instance, settings.uart_index);
-    }
+    expansion_enable(instance);
 }
 
 // Public API functions
 
-void expansion_enable(Expansion* instance, FuriHalSerialId serial_id) {
-    expansion_disable(instance);
-
-    furi_check(furi_mutex_acquire(instance->state_mutex, FuriWaitForever) == FuriStatusOk);
-
-    instance->serial_id = serial_id;
-    instance->state = ExpansionStateEnabled;
-
-    furi_hal_serial_control_set_expansion_callback(
-        instance->serial_id, expansion_detect_callback, instance);
-
-    furi_mutex_release(instance->state_mutex);
-
-    FURI_LOG_D(TAG, "Detection enabled");
+void expansion_enable(Expansion* instance) {
+    ExpansionSettings settings = {};
+    if(!expansion_settings_load(&settings)) {
+        expansion_settings_save(&settings);
+    } else if(settings.uart_index < FuriHalSerialIdMax) {
+        expansion_set_listen_serial(instance, settings.uart_index);
+    }
 }
 
 void expansion_disable(Expansion* instance) {
@@ -435,3 +423,19 @@ void expansion_disable(Expansion* instance) {
 
     furi_mutex_release(instance->state_mutex);
 }
+
+void expansion_set_listen_serial(Expansion* instance, FuriHalSerialId serial_id) {
+    expansion_disable(instance);
+
+    furi_check(furi_mutex_acquire(instance->state_mutex, FuriWaitForever) == FuriStatusOk);
+
+    instance->serial_id = serial_id;
+    instance->state = ExpansionStateEnabled;
+
+    furi_hal_serial_control_set_expansion_callback(
+        instance->serial_id, expansion_detect_callback, instance);
+
+    furi_mutex_release(instance->state_mutex);
+
+    FURI_LOG_D(TAG, "Detection enabled");
+}

applications/services/expansion/expansion.h

@@ -21,18 +21,17 @@ extern "C" {
 typedef struct Expansion Expansion;
 
 /**
- * @brief Enable support for expansion modules on designated serial port.
+ * @brief Enable support for expansion modules.
  *
- * Only one serial port can be used to communicate with an expansion
- * module at a time.
+ * Calling this function will load user settings and enable
+ * expansion module support on the serial port specified in said settings.
  *
- * Calling this function when expansion module support is already enabled
- * will first disable the previous setting, then enable the current one.
+ * If expansion module support was disabled in settings, this function
+ * does nothing.
  *
  * @param[in,out] instance pointer to the Expansion instance.
- * @param[in] serial_id numerical identifier of the serial.
  */
-void expansion_enable(Expansion* instance, FuriHalSerialId serial_id);
+void expansion_enable(Expansion* instance);
 
 /**
  * @brief Disable support for expansion modules.
@@ -41,10 +40,34 @@ void expansion_enable(Expansion* instance, FuriHalSerialId serial_id);
  * expansion module (if any), release the serial handle and
  * reset the respective pins to the default state.
  *
+ * @note Applications requiring serial port access MUST call
+ * this function BEFORE calling furi_hal_serial_control_acquire().
+ * Similarly, an expansion_enable() call MUST be made right AFTER
+ * a call to furi_hal_serial_control_release() to ensure that
+ * the user settings are properly restored.
+ *
  * @param[in,out] instance pointer to the Expansion instance.
  */
 void expansion_disable(Expansion* instance);
 
+/**
+ * @brief Enable support for expansion modules on designated serial port.
+ *
+ * Only one serial port can be used to communicate with an expansion
+ * module at a time.
+ *
+ * Calling this function when expansion module support is already enabled
+ * will first disable the previous setting, then enable the current one.
+ *
+ * @warning This function does not respect user settings for expansion modules,
+ * so calling it might leave the system in inconsistent state. Avoid using it
+ * unless absolutely necessary.
+ *
+ * @param[in,out] instance pointer to the Expansion instance.
+ * @param[in] serial_id numerical identifier of the serial.
+ */
+void expansion_set_listen_serial(Expansion* instance, FuriHalSerialId serial_id);
+
 #ifdef __cplusplus
 }
 #endif

applications/services/expansion/expansion_protocol.h

@@ -193,11 +193,18 @@ static inline size_t expansion_frame_get_encoded_size(const ExpansionFrame* fram
  *
  * @param[in] frame pointer to the frame to be evaluated.
  * @param[in] received_size number of bytes currently availabe for evaluation.
- * @returns number of bytes needed for a complete frame.
+ * @param[out] remaining_size pointer to the variable to contain the number of bytes needed for a complete frame.
+ * @returns true if the remaining size could be calculated, false on error.
  */
-static inline size_t
-    expansion_frame_get_remaining_size(const ExpansionFrame* frame, size_t received_size) {
-    if(received_size < sizeof(ExpansionFrameHeader)) return sizeof(ExpansionFrameHeader);
+static inline bool expansion_frame_get_remaining_size(
+    const ExpansionFrame* frame,
+    size_t received_size,
+    size_t* remaining_size) {
+    if(received_size < sizeof(ExpansionFrameHeader)) {
+        // Frame type is unknown as of now
+        *remaining_size = sizeof(ExpansionFrameHeader);
+        return true;
+    }
 
     const size_t received_content_size = received_size - sizeof(ExpansionFrameHeader);
     size_t content_size;
@@ -217,16 +224,26 @@ static inline size_t
         break;
     case ExpansionFrameTypeData:
         if(received_content_size < sizeof(frame->content.data.size)) {
+            // Data size is unknown as of now
             content_size = sizeof(frame->content.data.size);
+        } else if(frame->content.data.size > sizeof(frame->content.data.bytes)) {
+            // Malformed frame or garbage input
+            return false;
         } else {
             content_size = sizeof(frame->content.data.size) + frame->content.data.size;
         }
         break;
     default:
-        return SIZE_MAX;
+        return false;
     }
 
-    return content_size > received_content_size ? content_size - received_content_size : 0;
+    if(content_size > received_content_size) {
+        *remaining_size = content_size - received_content_size;
+    } else {
+        *remaining_size = 0;
+    }
+
+    return true;
 }
 
 /**
@@ -275,9 +292,7 @@ static inline ExpansionProtocolStatus expansion_protocol_decode(
     size_t remaining_size;
 
     while(true) {
-        remaining_size = expansion_frame_get_remaining_size(frame, total_size);
-
-        if(remaining_size == SIZE_MAX) {
+        if(!expansion_frame_get_remaining_size(frame, total_size, &remaining_size)) {
             return ExpansionProtocolStatusErrorFormat;
         } else if(remaining_size == 0) {
             break;

applications/settings/expansion_settings_app/expansion_settings_app.c

@@ -13,7 +13,7 @@ static void expansion_settings_app_uart_changed(VariableItem* item) {
     app->settings.uart_index = index;
 
     if(index < FuriHalSerialIdMax) {
-        expansion_enable(app->expansion, index);
+        expansion_set_listen_serial(app->expansion, index);
     } else {
         expansion_disable(app->expansion);
     }

targets/f18/api_symbols.csv

@@ -1,5 +1,5 @@
 entry,status,name,type,params
-Version,+,53.0,,
+Version,+,54.0,,
 Header,+,applications/services/bt/bt_service/bt.h,,
 Header,+,applications/services/cli/cli.h,,
 Header,+,applications/services/cli/cli_vcp.h,,
@@ -790,7 +790,8 @@ Function,-,exp2,double,double
 Function,-,exp2f,float,float
 Function,-,exp2l,long double,long double
 Function,+,expansion_disable,void,Expansion*
-Function,+,expansion_enable,void,"Expansion*, FuriHalSerialId"
+Function,+,expansion_enable,void,Expansion*
+Function,+,expansion_set_listen_serial,void,"Expansion*, FuriHalSerialId"
 Function,-,expf,float,float
 Function,-,expl,long double,long double
 Function,-,explicit_bzero,void,"void*, size_t"

targets/f7/api_symbols.csv

@@ -1,5 +1,5 @@
 entry,status,name,type,params
-Version,+,53.0,,
+Version,+,54.0,,
 Header,+,applications/drivers/subghz/cc1101_ext/cc1101_ext_interconnect.h,,
 Header,+,applications/services/bt/bt_service/bt.h,,
 Header,+,applications/services/cli/cli.h,,
@@ -879,7 +879,8 @@ Function,-,exp2,double,double
 Function,-,exp2f,float,float
 Function,-,exp2l,long double,long double
 Function,+,expansion_disable,void,Expansion*
-Function,+,expansion_enable,void,"Expansion*, FuriHalSerialId"
+Function,+,expansion_enable,void,Expansion*
+Function,+,expansion_set_listen_serial,void,"Expansion*, FuriHalSerialId"
 Function,-,expf,float,float
 Function,-,expl,long double,long double
 Function,-,explicit_bzero,void,"void*, size_t"