[PATCH] firewire: Fix out-of-bounds access in CSR iterator

[PATCH] firewire: Fix out-of-bounds access in CSR iterator

[Sreeraj S Kurup]

The fw_csr_iterator_next() function reads ci->p before
checking if it has reached the end.

This can cause an out-of-bounds read when the iterator
has no more entries.

Check the iterator position before reading the entry.
Move the pointer forward only after reading a valid entry.

Signed-off-by: Sreeraj S Kurup <sreekuttan2156239@gmail.com>
---
 drivers/firewire/core-device.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/firewire/core-device.c b/drivers/firewire/core-device.c
index a68100d3da0b..36667841ca3c 100644
--- a/drivers/firewire/core-device.c
+++ b/drivers/firewire/core-device.c
@@ -40,10 +40,14 @@ EXPORT_SYMBOL(fw_csr_iterator_init);
 
 int fw_csr_iterator_next(struct fw_csr_iterator *ci, int *key, int *value)
 {
+	if (ci->p >= ci->end)
+		return 0;
+
 	*key = *ci->p >> 24;
 	*value = *ci->p & 0xffffff;
 
-	return ci->p++ < ci->end;
+	ci->p++;
+	return 1;
 }
 EXPORT_SYMBOL(fw_csr_iterator_next);
 
-- 
2.55.0

Re: [PATCH] firewire: Fix out-of-bounds access in CSR iterator

[Takashi Sakamoto]

Hi,

Thank you for proposing this fix, however I have mixed feelings about
apply it, since the iterator is designed to traverse the cache data of the
actual configuration ROM, which has been generated by the parser.

If the configuration ROM contains any malformed entries, they should
already be detected by the parser. Therefore I don't think robustness of
the iterator is a critical concern in this case.

For the v7.4 kernel, I've merged KUnit tests for the parser
implementation. If you are still interested in discussion about this fix,
please check them at first then try to add tests specific to the iterator
itself.

On Mon, Nov 02, 2026 at 06:53:34AM +0000, Sreeraj S Kurup wrote:
> The fw_csr_iterator_next() function reads ci->p before
> checking if it has reached the end.
> 
> This can cause an out-of-bounds read when the iterator
> has no more entries.
> 
> Check the iterator position before reading the entry.
> Move the pointer forward only after reading a valid entry.
> 
> Signed-off-by: Sreeraj S Kurup <sreekuttan2156239@gmail.com>
> ---
>  drivers/firewire/core-device.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/firewire/core-device.c b/drivers/firewire/core-device.c
> index a68100d3da0b..36667841ca3c 100644
> --- a/drivers/firewire/core-device.c
> +++ b/drivers/firewire/core-device.c
> @@ -40,10 +40,14 @@ EXPORT_SYMBOL(fw_csr_iterator_init);
>  
>  int fw_csr_iterator_next(struct fw_csr_iterator *ci, int *key, int *value)
>  {
> +	if (ci->p >= ci->end)
> +		return 0;
> +
>  	*key = *ci->p >> 24;
>  	*value = *ci->p & 0xffffff;
>  
> -	return ci->p++ < ci->end;
> +	ci->p++;
> +	return 1;
>  }
>  EXPORT_SYMBOL(fw_csr_iterator_next);
>  
> -- 
> 2.55.0


Thanks

Takashi Sakamoto

[PATCH v2 0/2] firewire: Fix out-of-bounds access in CSR iterator

[Sreeraj S Kurup]

Hi Takashi,

This series fixes an out-of-bounds read in fw_csr_iterator_next() and
adds KUnit tests for the CSR iterator.

The first patch fixes the iterator by checking its position before
dereferencing the current entry.

The second patch adds a separate KUnit test suite covering:

- Iteration over valid entries.
- Calling the iterator after reaching the end.
- Iteration over an empty directory.

It also adds a separate Kconfig option for the iterator tests.

Testing:

- Ran the FireWire KUnit tests using:
  python3 tools/testing/kunit/kunit.py run
  --kunitconfig=drivers/firewire/.kunitconfig

- All tests passed.

- Results:
  [14:47:14] ======== firewire-config-rom-iterator (3 subtests) =========
  [14:47:14] [PASSED] test_csr_iterator_with_valid_entries
  [14:47:14] [PASSED] test_csr_iterator_at_end
  [14:47:14] [PASSED] test_csr_iterator_empty_directory
  [14:47:14] ========== [PASSED] firewire-config-rom-iterator ===========

Please review the patches.

Regards,
Sreeraj

Sreeraj S Kurup (2):
  firewire: Fix out-of-bounds access in CSR iterator
  firewire: Add KUnit tests for CSR iterator

 drivers/firewire/.kunitconfig               |  1 +
 drivers/firewire/Kconfig                    | 12 +++
 drivers/firewire/config-rom-iterator-test.c | 93 +++++++++++++++++++++
 drivers/firewire/core-device.c              | 10 ++-
 4 files changed, 115 insertions(+), 1 deletion(-)
 create mode 100644 drivers/firewire/config-rom-iterator-test.c

-- 
2.55.0

[PATCH v2 1/2] firewire: Fix out-of-bounds access in CSR iterator

[Sreeraj S Kurup]

The fw_csr_iterator_next() function reads ci->p before
checking if it has reached the end.

This can cause an out-of-bounds read when the iterator
has no more entries.

Check the iterator position before reading the entry.
Move the pointer forward only after reading a valid entry.

Signed-off-by: Sreeraj S Kurup <sreekuttan2156239@gmail.com>
---
 drivers/firewire/core-device.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/firewire/core-device.c b/drivers/firewire/core-device.c
index a68100d3da0b..36667841ca3c 100644
--- a/drivers/firewire/core-device.c
+++ b/drivers/firewire/core-device.c
@@ -40,10 +40,14 @@ EXPORT_SYMBOL(fw_csr_iterator_init);
 
 int fw_csr_iterator_next(struct fw_csr_iterator *ci, int *key, int *value)
 {
+	if (ci->p >= ci->end)
+		return 0;
+
 	*key = *ci->p >> 24;
 	*value = *ci->p & 0xffffff;
 
-	return ci->p++ < ci->end;
+	ci->p++;
+	return 1;
 }
 EXPORT_SYMBOL(fw_csr_iterator_next);
 
-- 
2.55.0

[PATCH v2 2/2] firewire: Add KUnit tests for CSR iterator

[Sreeraj S Kurup]

Add a separate KUnit test suite for the CSR iterator.

Test iteration over valid entries, an empty directory,
and calling the iterator after reaching the end.

Add a separate Kconfig option to enable the iterator tests
and update the KUnit configuration.

Signed-off-by: Sreeraj S Kurup <sreekuttan2156239@gmail.com>
---
 drivers/firewire/.kunitconfig               |  1 +
 drivers/firewire/Kconfig                    | 12 +++
 drivers/firewire/config-rom-iterator-test.c | 93 +++++++++++++++++++++
 drivers/firewire/core-device.c              |  4 +
 4 files changed, 110 insertions(+)
 create mode 100644 drivers/firewire/config-rom-iterator-test.c

diff --git a/drivers/firewire/.kunitconfig b/drivers/firewire/.kunitconfig
index ee9129081cef..e36dd34fd344 100644
--- a/drivers/firewire/.kunitconfig
+++ b/drivers/firewire/.kunitconfig
@@ -8,3 +8,4 @@ CONFIG_FIREWIRE_KUNIT_SELF_ID_SEQUENCE_HELPER_TEST=y
 CONFIG_FIREWIRE_KUNIT_OHCI_SERDES_TEST=y
 CONFIG_FIREWIRE_KUNIT_NODE_TREE_TEST=y
 CONFIG_FIREWIRE_KUNIT_CONFIG_ROM_PARSER_AND_GENERATOR_TEST=y
+CONFIG_FIREWIRE_KUNIT_CONFIG_ROM_ITERATOR_TEST=y
diff --git a/drivers/firewire/Kconfig b/drivers/firewire/Kconfig
index 7e1d4b2c440b..e67c2524c799 100644
--- a/drivers/firewire/Kconfig
+++ b/drivers/firewire/Kconfig
@@ -112,6 +112,18 @@ config FIREWIRE_KUNIT_CONFIG_ROM_PARSER_AND_GENERATOR_TEST
 	  For more information on KUnit and unit tests in general, refer
 	  to the KUnit documentation in Documentation/dev-tools/kunit/.
 
+config FIREWIRE_KUNIT_CONFIG_ROM_ITERATOR_TEST
+	tristate "KUnit tests for config ROM iterator" if !KUNIT_ALL_TESTS
+	depends on FIREWIRE && KUNIT
+	default KUNIT_ALL_TESTS
+	help
+	  This builds KUnit tests for the configuration ROM iterator used
+          by the FireWire subsystem.
+
+	  The tests check iteration over valid entries, handling empty
+	  directories, and calls to the iterator after reaching the end of
+          a directory
+
 config FIREWIRE_OHCI
 	tristate "OHCI-1394 controllers"
 	depends on PCI && FIREWIRE
diff --git a/drivers/firewire/config-rom-iterator-test.c b/drivers/firewire/config-rom-iterator-test.c
new file mode 100644
index 000000000000..8c846f2f2011
--- /dev/null
+++ b/drivers/firewire/config-rom-iterator-test.c
@@ -0,0 +1,93 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Copyright (C) 2026 Sreeraj S Kurup
+ *
+ * config-rom-iterator-test.c - KUnit tests for configuration ROM iterator.
+ */
+
+#include <kunit/test.h>
+
+static void test_csr_iterator_with_valid_entries(struct kunit *test)
+{
+	static const u32 directory[] = {
+		0x00020000,	/* Two entries. */
+		0x12001234,
+		0x34005678,
+	};
+	struct fw_csr_iterator ci;
+	int key, value;
+
+	fw_csr_iterator_init(&ci, directory);
+
+	KUNIT_EXPECT_TRUE(test,
+			  fw_csr_iterator_next(&ci, &key, &value));
+	KUNIT_EXPECT_EQ(test, key, 0x12);
+	KUNIT_EXPECT_EQ(test, value, 0x001234);
+
+	KUNIT_EXPECT_TRUE(test,
+			  fw_csr_iterator_next(&ci, &key, &value));
+	KUNIT_EXPECT_EQ(test, key, 0x34);
+	KUNIT_EXPECT_EQ(test, value, 0x005678);
+
+	KUNIT_EXPECT_FALSE(test,
+			   fw_csr_iterator_next(&ci, &key, &value));
+}
+
+static void test_csr_iterator_at_end(struct kunit *test)
+{
+	static const u32 directory[] = {
+		0x00010000,	/* One entry. */
+		0x12001234,
+	};
+	struct fw_csr_iterator ci;
+	int key, value;
+
+	fw_csr_iterator_init(&ci, directory);
+
+	KUNIT_ASSERT_TRUE(test,
+			  fw_csr_iterator_next(&ci, &key, &value));
+
+	KUNIT_EXPECT_EQ(test, key, 0x12);
+	KUNIT_EXPECT_EQ(test, value, 0x001234);
+
+	KUNIT_EXPECT_PTR_EQ(test, ci.p, ci.end);
+
+	/*
+	 * Calling next() after the last entry must not
+	 * advance the iterator beyond its end.
+	 */
+	KUNIT_EXPECT_FALSE(test,
+			   fw_csr_iterator_next(&ci, &key, &value));
+
+	KUNIT_EXPECT_PTR_EQ(test, ci.p, ci.end);
+}
+
+static void test_csr_iterator_empty_directory(struct kunit *test)
+{
+	static const u32 directory[] = {
+		0x00000000,
+	};
+	struct fw_csr_iterator ci;
+	int key, value;
+
+	fw_csr_iterator_init(&ci, directory);
+
+	KUNIT_EXPECT_FALSE(test,
+			   fw_csr_iterator_next(&ci, &key, &value));
+
+	KUNIT_EXPECT_PTR_EQ(test, ci.p, ci.end);
+}
+
+static struct kunit_case csr_iterator_test_cases[] = {
+	KUNIT_CASE(test_csr_iterator_with_valid_entries),
+	KUNIT_CASE(test_csr_iterator_at_end),
+	KUNIT_CASE(test_csr_iterator_empty_directory),
+	{}
+};
+
+static struct kunit_suite csr_iterator_test_suite = {
+	.name = "firewire-config-rom-iterator",
+	.test_cases = csr_iterator_test_cases,
+};
+
+kunit_test_suite(csr_iterator_test_suite);
diff --git a/drivers/firewire/core-device.c b/drivers/firewire/core-device.c
index 36667841ca3c..5d5ad4f41ab5 100644
--- a/drivers/firewire/core-device.c
+++ b/drivers/firewire/core-device.c
@@ -1450,3 +1450,7 @@ void fw_node_event(struct fw_card *card, struct fw_node *node, int event)
 #ifdef CONFIG_FIREWIRE_KUNIT_CONFIG_ROM_PARSER_AND_GENERATOR_TEST
 #include "config-rom-parser-test.c"
 #endif
+
+#ifdef CONFIG_FIREWIRE_KUNIT_CONFIG_ROM_ITERATOR_TEST
+#include "config-rom-iterator-test.c"
+#endif
-- 
2.55.0