Merge branch 'bugfix/esp_flash_forbid_invalid_init' into 'master'

spi_flash: forbid writing to main flash when using invalid init arguments

Closes IDFGH-6936

See merge request espressif/esp-idf!17761

components/spi_flash/esp_flash_spi_init.c

@@ -166,6 +166,52 @@ static IRAM_ATTR NOINLINE_ATTR void cs_initialize(esp_flash_t *chip, const esp_f
     chip->os_func->end(chip->os_func_data);
 }
 
+static bool use_bus_lock(int host_id)
+{
+    if (host_id != SPI1_HOST) {
+        return true;
+    }
+#if CONFIG_SPI_FLASH_SHARE_SPI1_BUS
+    return true;
+#else
+    return false;
+#endif
+}
+
+static esp_err_t acquire_spi_device(const esp_flash_spi_device_config_t *config, int* out_dev_id, spi_bus_lock_dev_handle_t* out_dev_handle)
+{
+    esp_err_t ret = ESP_OK;
+    int dev_id = -1;
+    spi_bus_lock_dev_handle_t dev_handle = NULL;
+
+    if (use_bus_lock(config->host_id)) {
+        spi_bus_lock_handle_t lock = spi_bus_lock_get_by_id(config->host_id);
+        spi_bus_lock_dev_config_t config = {.flags = SPI_BUS_LOCK_DEV_FLAG_CS_REQUIRED};
+
+        ret = spi_bus_lock_register_dev(lock, &config, &dev_handle);
+        if (ret == ESP_OK) {
+            dev_id = spi_bus_lock_get_dev_id(dev_handle);
+        } else if (ret == ESP_ERR_NOT_SUPPORTED) {
+            ESP_LOGE(TAG, "No free CS.");
+        } else if (ret == ESP_ERR_INVALID_ARG) {
+            ESP_LOGE(TAG, "Bus lock not initialized (check CONFIG_SPI_FLASH_SHARE_SPI1_BUS).");
+        }
+    } else {
+        const bool is_main_flash = (config->host_id == SPI1_HOST && config->cs_id == 0);
+        if (config->cs_id >= SOC_SPI_PERIPH_CS_NUM(config->host_id) || config->cs_id < 0 || is_main_flash) {
+            ESP_LOGE(TAG, "Not valid CS.");
+            ret = ESP_ERR_INVALID_ARG;
+        } else {
+            dev_id = config->cs_id;
+            assert(dev_handle == NULL);
+        }
+    }
+
+    *out_dev_handle = dev_handle;
+    *out_dev_id = dev_id;
+    return ret;
+}
+
 esp_err_t spi_bus_add_flash_device(esp_flash_t **out_chip, const esp_flash_spi_device_config_t *config)
 {
     if (out_chip == NULL) {
@@ -197,25 +243,22 @@ esp_err_t spi_bus_add_flash_device(esp_flash_t **out_chip, const esp_flash_spi_d
         goto fail;
     }
 
-    int dev_id = -1;
-    esp_err_t err = esp_flash_init_os_functions(chip, config->host_id, &dev_id);
-    if (err == ESP_ERR_NOT_SUPPORTED) {
-        ESP_LOGE(TAG, "Init os functions failed! No free CS.");
-    } else if (err == ESP_ERR_INVALID_ARG) {
-        ESP_LOGE(TAG, "Init os functions failed! Bus lock not initialized (check CONFIG_SPI_FLASH_SHARE_SPI1_BUS).");
-    }
+    int dev_id;
+    spi_bus_lock_dev_handle_t dev_handle;
+    esp_err_t err = acquire_spi_device(config, &dev_id, &dev_handle);
     if (err != ESP_OK) {
         ret = err;
         goto fail;
     }
-    // When `CONFIG_SPI_FLASH_SHARE_SPI1_BUS` is not enabled on SPI1 bus, the
-    // `esp_flash_init_os_functions` will not be able to assign a new device ID. In this case, we
-    // use the `cs_id` in the config structure.
-    if (dev_id == -1 && config->host_id == SPI1_HOST) {
-        dev_id = config->cs_id;
-    }
-    assert(dev_id < SOC_SPI_PERIPH_CS_NUM(config->host_id) && dev_id >= 0);
 
+    err = esp_flash_init_os_functions(chip, config->host_id, dev_handle);
+    if (err != ESP_OK) {
+        ret = err;
+        goto fail;
+    }
+
+    //avoid conflicts with main flash
+    assert(config->host_id != SPI1_HOST || dev_id != 0);
     bool use_iomux = spicommon_bus_using_iomux(config->host_id);
     memspi_host_config_t host_cfg = {
         .host_id = config->host_id,
@@ -245,7 +288,12 @@ esp_err_t spi_bus_remove_flash_device(esp_flash_t *chip)
     if (chip==NULL) {
         return ESP_ERR_INVALID_ARG;
     }
-    esp_flash_deinit_os_functions(chip);
+
+    spi_bus_lock_dev_handle_t dev_handle = NULL;
+    esp_flash_deinit_os_functions(chip, &dev_handle);
+    if (dev_handle) {
+        spi_bus_lock_unregister_dev(dev_handle);
+    }
     free(chip->host);
     free(chip);
     return ESP_OK;

components/spi_flash/include/esp_flash_internal.h

@@ -1,16 +1,8 @@
-// Copyright 2015-2019 Espressif Systems (Shanghai) PTE LTD
-//
-// Licensed under the Apache License, Version 2.0 (the "License");
-// you may not use this file except in compliance with the License.
-// You may obtain a copy of the License at
-//
-//     http://www.apache.org/licenses/LICENSE-2.0
-//
-// Unless required by applicable law or agreed to in writing, software
-// distributed under the License is distributed on an "AS IS" BASIS,
-// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
-// See the License for the specific language governing permissions and
-// limitations under the License.
+/*
+ * SPDX-FileCopyrightText: 2015-2022 Espressif Systems (Shanghai) CO LTD
+ *
+ * SPDX-License-Identifier: Apache-2.0
+ */
 
 #pragma once
 #include "esp_err.h"
@@ -69,21 +61,23 @@ esp_err_t esp_flash_app_disable_protect(bool disable);
  *
  * @param chip The chip to init os functions.
  * @param host_id Which SPI host to use, 1 for SPI1, 2 for SPI2 (HSPI), 3 for SPI3 (VSPI)
- * @param out_dev_id Output of occupied device slot
+ * @param dev_handle SPI bus lock device handle to acquire during flash operations
  *
  * @return
  *      - ESP_OK if success
  *      - ESP_ERR_INVALID_ARG if host_id is invalid
  */
-esp_err_t esp_flash_init_os_functions(esp_flash_t *chip, int host_id, int *out_dev_id);
+esp_err_t esp_flash_init_os_functions(esp_flash_t *chip, int host_id, spi_bus_lock_dev_handle_t dev_handle);
 
 /**
  * @brief Deinitialize OS-level functions
  *
- * @param chip  The chip to deinit os functions
+ * @param chip              The chip to deinit os functions
+ * @param out_dev_handle    The SPI bus lock passed from `esp_flash_init_os_functions`. The caller should deinitialize
+ *                          the lock.
  * @return always ESP_OK.
  */
-esp_err_t esp_flash_deinit_os_functions(esp_flash_t* chip);
+esp_err_t esp_flash_deinit_os_functions(esp_flash_t* chip, spi_bus_lock_dev_handle_t* out_dev_handle);
 
 /**
  * @brief Initialize the bus lock on the SPI1 bus. Should be called if drivers (including esp_flash)

components/spi_flash/spi_flash_os_func_app.c

@@ -210,30 +210,22 @@ static const esp_flash_os_functions_t esp_flash_spi23_default_os_functions = {
     .yield = NULL,
 };
 
-static spi_bus_lock_dev_handle_t register_dev(int host_id)
+static bool use_bus_lock(int host_id)
 {
-    spi_bus_lock_handle_t lock = spi_bus_lock_get_by_id(host_id);
-    spi_bus_lock_dev_handle_t dev_handle;
-    spi_bus_lock_dev_config_t config = {.flags = SPI_BUS_LOCK_DEV_FLAG_CS_REQUIRED};
-    esp_err_t err = spi_bus_lock_register_dev(lock, &config, &dev_handle);
-    if (err != ESP_OK) {
-        return NULL;
+    if (host_id != SPI1_HOST) {
+        return true;
     }
-    return dev_handle;
+#if CONFIG_SPI_FLASH_SHARE_SPI1_BUS
+    return true;
+#else
+    return false;
+#endif
 }
 
-esp_err_t esp_flash_init_os_functions(esp_flash_t *chip, int host_id, int* out_dev_id)
+esp_err_t esp_flash_init_os_functions(esp_flash_t *chip, int host_id, spi_bus_lock_dev_handle_t dev_handle)
 {
-    spi_bus_lock_dev_handle_t dev_handle = NULL;
-
-    // Skip initializing the bus lock when the bus is SPI1 and the bus is not shared with SPI Master
-    // driver, leaving dev_handle = NULL
-    bool skip_register_dev = (host_id == SPI1_HOST);
-#if CONFIG_SPI_FLASH_SHARE_SPI1_BUS
-    skip_register_dev = false;
-#endif
-    if (!skip_register_dev) {
-        dev_handle = register_dev(host_id);
+    if (use_bus_lock(host_id) && !dev_handle) {
+        return ESP_ERR_INVALID_ARG;
     }
 
     if (host_id == SPI1_HOST) {
@@ -259,28 +251,20 @@ esp_err_t esp_flash_init_os_functions(esp_flash_t *chip, int host_id, int* out_d
             return ESP_ERR_NO_MEM;
         }
         *(app_func_arg_t*) chip->os_func_data = (app_func_arg_t) {
-                .dev_lock = dev_handle,
+            .dev_lock = dev_handle,
         };
     } else {
         return ESP_ERR_INVALID_ARG;
     }
 
-    // Bus lock not initialized, the device ID should be directly given by application.
-    if (dev_handle) {
-        *out_dev_id = spi_bus_lock_get_dev_id(dev_handle);
-    }
-
     return ESP_OK;
 }
 
-esp_err_t esp_flash_deinit_os_functions(esp_flash_t* chip)
+esp_err_t esp_flash_deinit_os_functions(esp_flash_t* chip, spi_bus_lock_dev_handle_t* out_dev_handle)
 {
     if (chip->os_func_data) {
-        spi_bus_lock_dev_handle_t dev_lock = ((app_func_arg_t*)chip->os_func_data)->dev_lock;
-        // SPI bus lock is possible not used on SPI1 bus
-        if (dev_lock) {
-            spi_bus_lock_unregister_dev(dev_lock);
-        }
+        // SPI bus lock is possibly not used on SPI1 bus
+        *out_dev_handle = ((app_func_arg_t*)chip->os_func_data)->dev_lock;
         free(chip->os_func_data);
     }
     chip->os_func = NULL;

tools/ci/check_copyright_ignore.txt

@@ -1597,7 +1597,6 @@ components/soc/lldesc.c
 components/soc/soc_include_legacy_warn.c
 components/spi_flash/cache_utils.h
 components/spi_flash/include/esp_flash.h
-components/spi_flash/include/esp_flash_internal.h
 components/spi_flash/include/esp_flash_spi_init.h
 components/spi_flash/include/esp_spi_flash.h
 components/spi_flash/include/esp_spi_flash_counters.h