From bb0fa0746b6ed756808a94d9cec0810037f18c30 Mon Sep 17 00:00:00 2001 From: Dominik Ermel Date: Sun, 1 Dec 2024 14:39:29 +0100 Subject: [PATCH] stream_flash: Enforce size to be explicitly present on init The commit changes requirements for stream_flash_init, where size can no longer be 0 and has to be explicitly set, to avoid situation where size autodetection, invoked by size == 0, would miss changes in layout and silently allow overflow of Stream Flash into other partitions. There has also been new Kconfig option CONFIG_STREAM_FLASH_INSPECT, set to y by default to keep legacy behaviour, that can be used to turn off stream_flash_ctx vs device inspection, allowing user to shed inspection code once it is not useful anymore. Fixes: #71042 Signed-off-by: Dominik Ermel --- doc/releases/migration-guide-4.1.rst | 8 +++ doc/releases/release-notes-4.1.rst | 4 ++ include/zephyr/storage/stream_flash.h | 2 - subsys/storage/stream/Kconfig | 11 ++- subsys/storage/stream/stream_flash.c | 72 ++++++++++++++----- .../storage/stream/stream_flash/src/main.c | 19 ++--- 6 files changed, 86 insertions(+), 30 deletions(-) diff --git a/doc/releases/migration-guide-4.1.rst b/doc/releases/migration-guide-4.1.rst index 29805486d9c..fce9ffa3c13 100644 --- a/doc/releases/migration-guide-4.1.rst +++ b/doc/releases/migration-guide-4.1.rst @@ -393,6 +393,14 @@ LoRa additional ``user_data`` parameter, which is a void pointer. This parameter can be used to reference any user-defined data structure. To maintain the current behavior, set this parameter to ``NULL``. +Stream Flash +============ + +* The function :c:func:`stream_flash_init` no longer does auto-detection of device size + when ``size`` parameter is set to 0 and will return error in such case. User is now + required to explicitly provide device size. Issue :github:`71042` provides rationale + for the change. + Architectures ************* diff --git a/doc/releases/release-notes-4.1.rst b/doc/releases/release-notes-4.1.rst index 3620b60c512..aec178a5d33 100644 --- a/doc/releases/release-notes-4.1.rst +++ b/doc/releases/release-notes-4.1.rst @@ -24,6 +24,10 @@ https://docs.zephyrproject.org/latest/security/vulnerabilities.html API Changes *********** + * Stream Flash initialization function :c:func:`stream_flash_init` no longer does + device size autodetection and instead requires user to explicitly provide size + of area available for Stream Flash instance operation. + Removed APIs in this release ============================ diff --git a/include/zephyr/storage/stream_flash.h b/include/zephyr/storage/stream_flash.h index 23f24059425..a3e4473526d 100644 --- a/include/zephyr/storage/stream_flash.h +++ b/include/zephyr/storage/stream_flash.h @@ -81,8 +81,6 @@ struct stream_flash_ctx { * Must be multiple of the flash device write-block-size. * @param offset Offset within flash device to start writing to * @param size Number of bytes available for performing buffered write. - * If this is '0', the size will be set to the total size - * of the flash device minus the offset. * @param cb Callback to be invoked on completed flash write operations. * Callback is supported when CONFIG_STREAM_FLASH_POST_WRITE_CALLBACK * is enabled. diff --git a/subsys/storage/stream/Kconfig b/subsys/storage/stream/Kconfig index 25df2546196..bed472a038a 100644 --- a/subsys/storage/stream/Kconfig +++ b/subsys/storage/stream/Kconfig @@ -6,12 +6,21 @@ menuconfig STREAM_FLASH bool "Stream to flash" - select FLASH_PAGE_LAYOUT help Enable support of stream to flash API if STREAM_FLASH +config STREAM_FLASH_INSPECT + bool "Check whether device layout is OK with Stream Flash definition" + default y + select FLASH_PAGE_LAYOUT + help + Runs simple check to find whether provided device can be used for + stream flash by verifying that buffer size will fit into page. + Once correct configuration has been established and tested it is + worth to disable the option to cut out some unneeded code. + config STREAM_FLASH_POST_WRITE_CALLBACK bool "Write complete callback" default y diff --git a/subsys/storage/stream/stream_flash.c b/subsys/storage/stream/stream_flash.c index 7822a2817c0..cdb1885885e 100644 --- a/subsys/storage/stream/stream_flash.c +++ b/subsys/storage/stream/stream_flash.c @@ -246,6 +246,7 @@ size_t stream_flash_bytes_written(const struct stream_flash_ctx *ctx) return ctx->bytes_written; } +#ifdef CONFIG_STREAM_FLASH_INSPECT struct _inspect_flash { size_t buf_len; size_t total_size; @@ -267,42 +268,63 @@ static bool find_flash_total_size(const struct flash_pages_info *info, return true; } +/* Internal function make sure *ctx is not NULL, no redundant check here */ +static inline int inspect_device(const struct stream_flash_ctx *ctx) +{ + struct _inspect_flash inspect_flash_ctx = { + .buf_len = ctx->buf_len, + .total_size = 0 + }; + + /* Calculate the total size of the flash device, and inspect pages + * while doing so. + */ + flash_page_foreach(ctx->fdev, find_flash_total_size, &inspect_flash_ctx); + + if (inspect_flash_ctx.total_size == 0) { + LOG_ERR("Device seems to have 0 size"); + return -EFAULT; + } else if (inspect_flash_ctx.total_size < (ctx->offset + ctx->available)) { + LOG_ERR("Requested range overflows device size"); + return -EFAULT; + } + + return 0; +} +#else +static inline int inspect_device(const struct stream_flash_ctx *ctx) +{ + ARG_UNUSED(ctx); + return 0; +} +#endif + int stream_flash_init(struct stream_flash_ctx *ctx, const struct device *fdev, uint8_t *buf, size_t buf_len, size_t offset, size_t size, stream_flash_callback_t cb) { const struct flash_parameters *params; + if (!ctx || !fdev || !buf) { return -EFAULT; } - struct _inspect_flash inspect_flash_ctx = { - .buf_len = buf_len, - .total_size = 0 - }; - params = flash_get_parameters(fdev); - ctx->write_block_size = params->write_block_size; - if (buf_len % ctx->write_block_size) { + if (buf_len % params->write_block_size) { LOG_ERR("Buffer size is not aligned to minimal write-block-size"); return -EFAULT; } - /* Calculate the total size of the flash device */ - flash_page_foreach(fdev, find_flash_total_size, &inspect_flash_ctx); - - /* The flash size counted should never be equal zero */ - if (inspect_flash_ctx.total_size == 0) { - return -EFAULT; - } - - if ((offset + size) > inspect_flash_ctx.total_size || - offset % ctx->write_block_size) { + if (offset % params->write_block_size) { LOG_ERR("Incorrect parameter"); return -EFAULT; } + if (size == 0 || size % params->write_block_size) { + LOG_ERR("Size is incorrect"); + return -EFAULT; + } ctx->fdev = fdev; ctx->buf = buf; @@ -310,8 +332,8 @@ int stream_flash_init(struct stream_flash_ctx *ctx, const struct device *fdev, ctx->bytes_written = 0; ctx->buf_bytes = 0U; ctx->offset = offset; - ctx->available = (size == 0 ? inspect_flash_ctx.total_size - offset : - size); + ctx->available = size; + ctx->write_block_size = params->write_block_size; #if !defined(CONFIG_STREAM_FLASH_POST_WRITE_CALLBACK) ARG_UNUSED(cb); @@ -319,11 +341,23 @@ int stream_flash_init(struct stream_flash_ctx *ctx, const struct device *fdev, ctx->callback = cb; #endif + #ifdef CONFIG_STREAM_FLASH_ERASE ctx->last_erased_page_start_offset = -1; #endif ctx->erase_value = params->erase_value; + /* Inspection is deliberately done once context has been filled in */ + if (IS_ENABLED(CONFIG_STREAM_FLASH_INSPECT)) { + int ret = inspect_device(ctx); + + if (ret != 0) { + /* No log here, the inspect_device already does logging */ + return ret; + } + } + + return 0; } diff --git a/tests/subsys/storage/stream/stream_flash/src/main.c b/tests/subsys/storage/stream/stream_flash/src/main.c index 52d412053dd..88d2c7788ec 100644 --- a/tests/subsys/storage/stream/stream_flash/src/main.c +++ b/tests/subsys/storage/stream/stream_flash/src/main.c @@ -108,7 +108,7 @@ static void init_target(void) erase_flash(); - rc = stream_flash_init(&ctx, fdev, generic_buf, BUF_LEN, FLASH_BASE, 0, + rc = stream_flash_init(&ctx, fdev, generic_buf, BUF_LEN, FLASH_BASE, FLASH_AVAILABLE, stream_flash_callback); zassert_equal(rc, 0, "expected success"); } @@ -133,10 +133,13 @@ ZTEST(lib_stream_flash, test_stream_flash_init) rc = stream_flash_init(&ctx, fdev, NULL, BUF_LEN, FLASH_BASE, 0, NULL); zassert_true(rc < 0, "should fail as buffer is NULL"); - /* Entering '0' as flash size uses rest of flash. */ rc = stream_flash_init(&ctx, fdev, generic_buf, BUF_LEN, FLASH_BASE, 0, NULL); - zassert_equal(rc, 0, "should succeed"); - zassert_equal(FLASH_AVAILABLE, ctx.available, "Wrong size"); + zassert_equal(rc, -EFAULT, "should fail as size 0"); + + rc = stream_flash_init(&ctx, fdev, generic_buf, BUF_LEN, + flash_get_write_block_size(ctx.fdev) - 1, 0, NULL); + zassert_equal(rc, -EFAULT, "should fail as size is not aligned to write block size "); + flash_get_write_block_size(ctx.fdev); } ZTEST(lib_stream_flash, test_stream_flash_buffered_write) @@ -207,7 +210,7 @@ ZTEST(lib_stream_flash, test_stream_flash_buffered_write_unaligned) VERIFY_WRITTEN(0, 1); rc = stream_flash_init(&ctx, fdev, generic_buf, BUF_LEN, FLASH_BASE + BUF_LEN, - 0, stream_flash_callback); + FLASH_AVAILABLE - BUF_LEN, stream_flash_callback); zassert_equal(rc, 0, "expected success"); /* Trigger verification in callback */ @@ -278,11 +281,11 @@ ZTEST(lib_stream_flash, test_stream_flash_buf_size_greater_than_page_size) int rc; /* To illustrate that other params does not trigger error */ - rc = stream_flash_init(&ctx, fdev, generic_buf, 0x10, 0, 0, NULL); + rc = stream_flash_init(&ctx, fdev, generic_buf, 0x10, 0, FLASH_AVAILABLE, NULL); zassert_equal(rc, 0, "expected success"); /* Only change buf_len param */ - rc = stream_flash_init(&ctx, fdev, generic_buf, 0x10000, 0, 0, NULL); + rc = stream_flash_init(&ctx, fdev, generic_buf, 0x10000, 0, FLASH_AVAILABLE, NULL); zassert_true(rc < 0, "expected failure"); } @@ -410,7 +413,7 @@ ZTEST(lib_stream_flash, test_stream_flash_buffered_write_whole_page) /* Reset stream_flash context */ memset(&ctx, 0, sizeof(ctx)); memset(generic_buf, 0, BUF_LEN); - rc = stream_flash_init(&ctx, fdev, generic_buf, BUF_LEN, FLASH_BASE, 0, + rc = stream_flash_init(&ctx, fdev, generic_buf, BUF_LEN, FLASH_BASE, FLASH_AVAILABLE, stream_flash_callback); zassert_equal(rc, 0, "expected success");