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 <dominik.ermel@nordicsemi.no>

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
 *************
 

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
 ============================
 

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.

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

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

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