From e8e659f79178c3c30bbfbfac7a310a2a27c9694b Mon Sep 17 00:00:00 2001 From: Sreeraj S Kurup Date: Sat, 25 Jul 2026 15:52:54 +0000 Subject: [PATCH 1/8] firewire: core: validate overall descriptor length in fw_core_add_descriptor() In fw_core_add_descriptor(), incoming descriptor structures are processed without checking whether the descriptor's specified length falls within valid boundaries. An empty descriptor (length 0) or an oversized descriptor exceeding the IEEE 1394 Config ROM capacity can lead to invalid processing. Add bounds checking at the start of fw_core_add_descriptor() using the in_range() helper macro to reject descriptors with length 0 or length exceeding 256 quadlets (the standard maximum Configuration ROM size). Signed-off-by: Sreeraj S Kurup Link: https://lore.kernel.org/r/20260725155255.3054-2-sreekuttan2156239@gmail.com Signed-off-by: Takashi Sakamoto --- drivers/firewire/core-card.c | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/drivers/firewire/core-card.c b/drivers/firewire/core-card.c index a754c6366b97..eaec54ea287a 100644 --- a/drivers/firewire/core-card.c +++ b/drivers/firewire/core-card.c @@ -16,6 +16,7 @@ #include #include #include +#include #include #include @@ -167,11 +168,10 @@ int fw_core_add_descriptor(struct fw_descriptor *desc) { size_t i; - /* - * Check descriptor is valid; the length of all blocks in the - * descriptor has to add up to exactly the length of the - * block. - */ + /* Reject empty descriptors or those exceeding max Config ROM size (256 quadlets) */ + if (!in_range(desc->length, 1, 256)) + return -EINVAL; + i = 0; while (i < desc->length) i += (desc->data[i] >> 16) + 1; From a5367912ba81dfb3180fce89b545b009e57f17a5 Mon Sep 17 00:00:00 2001 From: Sreeraj S Kurup Date: Sat, 25 Jul 2026 15:52:55 +0000 Subject: [PATCH 2/8] firewire: core: validate sub-block lengths in fw_core_add_descriptor() When traversing internal block structures of a descriptor in fw_core_add_descriptor(), each sub-block header specifies its own length in the upper 16 bits of its header quadlet. If a malformed or corrupted descriptor provides a sub-block length that exceeds the remaining total length of the descriptor buffer, the parsing loop advances past the allocated boundary of desc->data, leading to an out-of-bounds read access. Validate each sub-block's length against the remaining descriptor size before advancing the offset pointer to ensure loop bounds safety. Signed-off-by: Sreeraj S Kurup Link: https://lore.kernel.org/r/20260725155255.3054-3-sreekuttan2156239@gmail.com Signed-off-by: Takashi Sakamoto --- drivers/firewire/core-card.c | 20 ++++++++++++++++++-- 1 file changed, 18 insertions(+), 2 deletions(-) diff --git a/drivers/firewire/core-card.c b/drivers/firewire/core-card.c index eaec54ea287a..94992791f02e 100644 --- a/drivers/firewire/core-card.c +++ b/drivers/firewire/core-card.c @@ -173,9 +173,25 @@ int fw_core_add_descriptor(struct fw_descriptor *desc) return -EINVAL; i = 0; - while (i < desc->length) - i += (desc->data[i] >> 16) + 1; + /* + * Validate internal block structures within the descriptor. Each sub-block + * encodes its length in the top 16 bits of its header quadlet. + */ + while (i < desc->length) { + u16 block_len = desc->data[i] >> 16; + /* + * Guard against corrupted descriptors where an individual block length + * claims to extend past the allocated end of desc->data, avoiding + * out-of-bounds reads. + */ + if (block_len >= desc->length - i) + return -EINVAL; + + i += block_len + 1; + } + + /* The sum of sub-block lengths must match total descriptor length */ if (i != desc->length) return -EINVAL; From 1f9b65cf57081035547b4d3ca1805df20892e9db Mon Sep 17 00:00:00 2001 From: Takashi Sakamoto Date: Mon, 10 Aug 2026 15:41:17 +0900 Subject: [PATCH 3/8] firewire: core: add KUnit test skeleton for node tree Some issues have been reported in node tree management. Refactoring the topology-related code in the core is required. Adding unit tests would help ensure that the refactoring does not introduce regressions. This commit adds a KUnit test skeleton for this purpose. Link: https://lore.kernel.org/r/20260810064119.410324-2-o-takashi@sakamocchi.jp Signed-off-by: Takashi Sakamoto --- drivers/firewire/.kunitconfig | 1 + drivers/firewire/Kconfig | 15 +++++++++++++++ drivers/firewire/core-topology.c | 4 ++++ drivers/firewire/node-tree-test.c | 19 +++++++++++++++++++ 4 files changed, 39 insertions(+) create mode 100644 drivers/firewire/node-tree-test.c diff --git a/drivers/firewire/.kunitconfig b/drivers/firewire/.kunitconfig index 21b7e9eef63d..7406acb00478 100644 --- a/drivers/firewire/.kunitconfig +++ b/drivers/firewire/.kunitconfig @@ -6,3 +6,4 @@ CONFIG_FIREWIRE_KUNIT_DEVICE_ATTRIBUTE_TEST=y CONFIG_FIREWIRE_KUNIT_PACKET_SERDES_TEST=y CONFIG_FIREWIRE_KUNIT_SELF_ID_SEQUENCE_HELPER_TEST=y CONFIG_FIREWIRE_KUNIT_OHCI_SERDES_TEST=y +CONFIG_FIREWIRE_KUNIT_NODE_TREE_TEST=y diff --git a/drivers/firewire/Kconfig b/drivers/firewire/Kconfig index a5f5e250223a..b5abe00accc9 100644 --- a/drivers/firewire/Kconfig +++ b/drivers/firewire/Kconfig @@ -81,6 +81,21 @@ config FIREWIRE_KUNIT_SELF_ID_SEQUENCE_HELPER_TEST For more information on KUnit and unit tests in general, refer to the KUnit documentation in Documentation/dev-tools/kunit/. +config FIREWIRE_KUNIT_NODE_TREE_TEST + tristate "KUnit tests for node tree" if !KUNIT_ALL_TESTS + depends on FIREWIRE && KUNIT + default KUNIT_ALL_TESTS + help + This builds the KUnit tests for node tree. + + KUnit tests run during boot and output the results to the debug + log in TAP format (https://testanything.org/). Only useful for + kernel devs running KUnit test harness and are not for inclusion + into a production build. + + For more information on KUnit and unit tests in general, refer + to the KUnit documentation in Documentation/dev-tools/kunit/. + config FIREWIRE_OHCI tristate "OHCI-1394 controllers" depends on PCI && FIREWIRE diff --git a/drivers/firewire/core-topology.c b/drivers/firewire/core-topology.c index df2ac0dab106..1d3a4419f554 100644 --- a/drivers/firewire/core-topology.c +++ b/drivers/firewire/core-topology.c @@ -507,3 +507,7 @@ void fw_core_handle_bus_reset(struct fw_card *card, int node_id, int generation, } } EXPORT_SYMBOL(fw_core_handle_bus_reset); + +#ifdef CONFIG_FIREWIRE_KUNIT_NODE_TREE_TEST +#include "node-tree-test.c" +#endif diff --git a/drivers/firewire/node-tree-test.c b/drivers/firewire/node-tree-test.c new file mode 100644 index 000000000000..fca71dd99b25 --- /dev/null +++ b/drivers/firewire/node-tree-test.c @@ -0,0 +1,19 @@ +// SPDX-License-Identifier: GPL-2.0-only +// +// node-tree-test.c - An application of Kunit to test node tree. +// +// Copyright (c) 2026 Takashi Sakamoto +// +// This file can not be built independently since it is intentionally included in core-topology.c. + +#include + +static struct kunit_case node_tree_test_cases[] = { + {} +}; + +static struct kunit_suite node_tree_test_suite = { + .name = "firewire-node-tree", + .test_cases = node_tree_test_cases, +}; +kunit_test_suite(node_tree_test_suite); From 7b763ea17152cf61ede5289c3c66bf133c8aafa7 Mon Sep 17 00:00:00 2001 From: Takashi Sakamoto Date: Mon, 10 Aug 2026 15:41:18 +0900 Subject: [PATCH 4/8] firewire: core: add KUnit tests for successful tree building After a bus reset, self ID sequence is captured by 1394 OHCI hardware and passed to software through SelfID DMA context. The core parses the sequence to build an internal cache of the node tree for the current generation of the bus. This is the first step in managing resources on the bus. The tree is build by the build_tree() function. This commit adds KUnit tests for the function, covering several successful scenarios. Link: https://lore.kernel.org/r/20260810064119.410324-3-o-takashi@sakamocchi.jp Signed-off-by: Takashi Sakamoto --- drivers/firewire/node-tree-test.c | 508 ++++++++++++++++++++++++++++++ 1 file changed, 508 insertions(+) diff --git a/drivers/firewire/node-tree-test.c b/drivers/firewire/node-tree-test.c index fca71dd99b25..6f404df3d50e 100644 --- a/drivers/firewire/node-tree-test.c +++ b/drivers/firewire/node-tree-test.c @@ -7,13 +7,521 @@ // This file can not be built independently since it is intentionally included in core-topology.c. #include +#include +#include + +struct private_data { + struct fw_card *card; + unsigned int release_count; +}; + +static int node_tree_test_init(struct kunit *test) +{ + struct private_data *data; + + data = kunit_kzalloc(test, sizeof(*data), GFP_KERNEL); + KUNIT_ASSERT_NOT_NULL(test, data); + + data->card = kunit_kzalloc(test, sizeof(struct fw_card), GFP_KERNEL); + KUNIT_ASSERT_NOT_NULL(test, data->card); + + data->card->device = kunit_device_register(test, "dummy-device"); + KUNIT_ASSERT_NOT_ERR_OR_NULL(test, data->card->device); + + test->priv = data; + + return 0; +} + +static void node_tree_test_exit(struct kunit *test) +{ + struct private_data *data = test->priv; + + kunit_device_unregister(test, data->card->device); + kunit_kfree(test, data->card); + kunit_kfree(test, data); +} + +static void release_fw_node(struct fw_card *card, struct fw_node *node, struct fw_node *parent) +{ + struct private_data *data = kunit_get_current_test()->priv; + + fw_node_put(node); + ++data->release_count; +} + +static void node_tree_test_two_nodes(struct kunit *test) +{ + // root + // ++============++ + // || phy 1 || + // || P0 P1 P2 || + // ++===|==|==|==++ + // | + // +-----+ + // | + // ++===|==x==x==++ + // || P0 P1 P2 || + // || phy 0 || + // ++============++ + // + static const u32 self_id_sequence[] = { + 0x80000080, + 0x8100005e, + }; + struct private_data *data = test->priv; + struct fw_card *card = data->card; + + card->node_id = LOCAL_BUS | 0x01; + + card->local_node = build_tree(card, self_id_sequence, ARRAY_SIZE(self_id_sequence), 123); + KUNIT_EXPECT_NOT_NULL(test, card->local_node); + KUNIT_EXPECT_PTR_EQ(test, card->local_node, card->root_node); + + struct fw_node *node = card->root_node; + KUNIT_EXPECT_EQ(test, node->node_id, LOCAL_BUS | 0x01); + KUNIT_EXPECT_EQ(test, node->port_count, 3); + KUNIT_EXPECT_NULL(test, node->ports[0]); + KUNIT_EXPECT_NULL(test, node->ports[1]); + KUNIT_EXPECT_NOT_NULL(test, node->ports[2]); + + struct fw_node *parent = node; + node = parent->ports[2]; + KUNIT_EXPECT_EQ(test, node->node_id, LOCAL_BUS | 0x00); + KUNIT_EXPECT_EQ(test, node->port_count, 1); + KUNIT_EXPECT_PTR_EQ(test, node->ports[0], parent); + + ++card->color; + for_each_fw_node(card, card->root_node, release_fw_node); + KUNIT_EXPECT_EQ(test, data->release_count, 2); +} + +static void node_tree_test_two_nodes_1394a(struct kunit *test) +{ + // root + // ++===============++ + // || phy 0 || + // || P0 P1 P2 P3 || + // ++===|==|==|==|==++ + // | + // +--+ + // | + // ++===|==|==|==|==|==++ + // || P0 P1 P2 P3 P4 || + // || phy 1 || + // ++==================++ + // + // NOTE: Just for Self-ID Packets Zero and One. + static const u32 self_id_sequence[] = { + 0x80000065, 0x80814000, + 0x8100005d, 0x81810000, + }; + struct private_data *data = test->priv; + struct fw_card *card = data->card; + + card->node_id = LOCAL_BUS | 0x01; + + card->local_node = build_tree(card, self_id_sequence, ARRAY_SIZE(self_id_sequence), 123); + KUNIT_EXPECT_NOT_NULL(test, card->local_node); + KUNIT_EXPECT_PTR_EQ(test, card->local_node, card->root_node); + + struct fw_node *node = card->root_node; + KUNIT_EXPECT_EQ(test, node->node_id, LOCAL_BUS | 0x01); + KUNIT_EXPECT_EQ(test, node->port_count, 4); + KUNIT_EXPECT_NULL(test, node->ports[0]); + KUNIT_EXPECT_NULL(test, node->ports[1]); + KUNIT_EXPECT_NOT_NULL(test, node->ports[2]); + KUNIT_EXPECT_NULL(test, node->ports[3]); + + struct fw_node *parent = node; + node = parent->ports[2]; + KUNIT_EXPECT_EQ(test, node->node_id, LOCAL_BUS | 0x00); + KUNIT_EXPECT_EQ(test, node->port_count, 5); + KUNIT_EXPECT_NULL(test, node->ports[0]); + KUNIT_EXPECT_PTR_EQ(test, node->ports[1], parent); + KUNIT_EXPECT_NULL(test, node->ports[2]); + KUNIT_EXPECT_NULL(test, node->ports[3]); + KUNIT_EXPECT_NULL(test, node->ports[4]); + + ++card->color; + for_each_fw_node(card, card->root_node, release_fw_node); + KUNIT_EXPECT_EQ(test, data->release_count, 2); +} + +static void node_tree_test_three_nodes_case0(struct kunit *test) +{ + // root + // ++============++ + // || phy 2 || + // || P0 P1 P2 || + // ++===|==|==|==++ + // | | + // +--+ +-----------------+ + // | | + // ++===|==|==x==++ ++===|==|==|==++ + // || P0 P1 P2 || || P0 P1 P2 || + // || phy 0 || || phy 1 || + // ++============++ ++============++ + // + static const u32 self_id_sequence[] = { + 0x80000060, + 0x81000058, + 0x820000dc, + }; + struct private_data *data = test->priv; + struct fw_card *card = data->card; + + card->node_id = LOCAL_BUS | 0x02; + + card->local_node = build_tree(card, self_id_sequence, ARRAY_SIZE(self_id_sequence), 123); + KUNIT_EXPECT_NOT_NULL(test, card->local_node); + KUNIT_EXPECT_PTR_EQ(test, card->local_node, card->root_node); + + struct fw_node *node = card->root_node; + KUNIT_EXPECT_EQ(test, node->node_id, LOCAL_BUS | 0x02); + KUNIT_EXPECT_EQ(test, node->port_count, 3); + KUNIT_EXPECT_NOT_NULL(test, node->ports[0]); + KUNIT_EXPECT_NULL(test, node->ports[1]); + KUNIT_EXPECT_NOT_NULL(test, node->ports[2]); + + struct fw_node *parent = node; + node = parent->ports[0]; + KUNIT_EXPECT_EQ(test, node->node_id, LOCAL_BUS | 0x00); + KUNIT_EXPECT_EQ(test, node->port_count, 2); + KUNIT_EXPECT_NULL(test, node->ports[0]); + KUNIT_EXPECT_PTR_EQ(test, node->ports[1], parent); + + node = parent->ports[2]; + KUNIT_EXPECT_EQ(test, node->node_id, LOCAL_BUS | 0x01); + KUNIT_EXPECT_EQ(test, node->port_count, 3); + KUNIT_EXPECT_NULL(test, node->ports[0]); + KUNIT_EXPECT_NULL(test, node->ports[1]); + KUNIT_EXPECT_PTR_EQ(test, node->ports[2], parent); + + ++card->color; + for_each_fw_node(card, card->root_node, release_fw_node); + KUNIT_EXPECT_EQ(test, data->release_count, 3); +} + +static void node_tree_test_three_nodes_case1(struct kunit *test) +{ + // root + // ++============++ + // || phy 2 || + // || P0 P1 P2 || + // ++===|==|==x==++ + // | + // | +-----------+ + // | | | + // ++===|==|==|==++ ++===|==x==x==++ + // || P0 P1 P2 || || P0 P1 P2 || + // || phy 1 || || phy 0 || + // ++============++ ++============++ + // + static const u32 self_id_sequence[] = { + 0x80000080, + 0x8100006c, + 0x82000070, + }; + struct private_data *data = test->priv; + struct fw_card *card = data->card; + + card->node_id = LOCAL_BUS | 0x02; + + card->local_node = build_tree(card, self_id_sequence, ARRAY_SIZE(self_id_sequence), 123); + KUNIT_EXPECT_NOT_NULL(test, card->local_node); + KUNIT_EXPECT_PTR_EQ(test, card->local_node, card->root_node); + + struct fw_node *node = card->root_node; + KUNIT_EXPECT_EQ(test, node->node_id, LOCAL_BUS | 0x02); + KUNIT_EXPECT_EQ(test, node->port_count, 2); + KUNIT_EXPECT_NULL(test, node->ports[0]); + KUNIT_EXPECT_NOT_NULL(test, node->ports[1]); + + struct fw_node *parent = node; + node = parent->ports[1]; + KUNIT_EXPECT_EQ(test, node->node_id, LOCAL_BUS | 0x01); + KUNIT_EXPECT_EQ(test, node->port_count, 3); + KUNIT_EXPECT_NULL(test, node->ports[0]); + KUNIT_EXPECT_PTR_EQ(test, node->ports[1], parent); + KUNIT_EXPECT_NOT_NULL(test, node->ports[2]); + + parent = node; + node = parent->ports[2]; + KUNIT_EXPECT_EQ(test, node->node_id, LOCAL_BUS | 0x00); + KUNIT_EXPECT_EQ(test, node->port_count, 1); + KUNIT_EXPECT_PTR_EQ(test, node->ports[0], parent); + + ++card->color; + for_each_fw_node(card, card->root_node, release_fw_node); + KUNIT_EXPECT_EQ(test, data->release_count, 3); +} + +static void node_tree_test_four_nodes_case0(struct kunit *test) +{ + // root + // ++============++ + // || phy 3 || + // || P0 P1 P2 || + // ++===|==|==|==++ + // | + // | +-----------+ +--------------+ + // | | | | | + // ++===|==|==|==++ ++===|==|==x==++ ++===|==x==x==++ + // || P0 P1 P2 || || P0 P1 P2 || || P0 P1 P2 || + // || phy 2 || || phy 1 || || phy 0 || + // ++============++ ++============++ ++============++ + // + static const u32 self_id_sequence[] = { + 0x80000080, + 0x810000b0, + 0x8200006c, + 0x83000074, + }; + struct private_data *data = test->priv; + struct fw_card *card = data->card; + + card->node_id = LOCAL_BUS | 0x03; + + card->local_node = build_tree(card, self_id_sequence, ARRAY_SIZE(self_id_sequence), 123); + KUNIT_EXPECT_NOT_NULL(test, card->local_node); + KUNIT_EXPECT_PTR_EQ(test, card->local_node, card->root_node); + + struct fw_node *node = card->root_node; + KUNIT_EXPECT_EQ(test, node->node_id, LOCAL_BUS | 0x03); + KUNIT_EXPECT_EQ(test, node->port_count, 3); + KUNIT_EXPECT_NULL(test, node->ports[0]); + KUNIT_EXPECT_NOT_NULL(test, node->ports[1]); + KUNIT_EXPECT_NULL(test, node->ports[2]); + + struct fw_node *parent = node; + node = parent->ports[1]; + KUNIT_EXPECT_EQ(test, node->node_id, LOCAL_BUS | 0x02); + KUNIT_EXPECT_EQ(test, node->port_count, 3); + KUNIT_EXPECT_NULL(test, node->ports[0]); + KUNIT_EXPECT_PTR_EQ(test, node->ports[1], parent); + KUNIT_EXPECT_NOT_NULL(test, node->ports[2]); + + parent = node; + node = parent->ports[2]; + KUNIT_EXPECT_EQ(test, node->node_id, LOCAL_BUS | 0x01); + KUNIT_EXPECT_EQ(test, node->port_count, 2); + KUNIT_EXPECT_PTR_EQ(test, node->ports[0], parent); + KUNIT_EXPECT_NOT_NULL(test, node->ports[1]); + + parent = node; + node = parent->ports[1]; + KUNIT_EXPECT_EQ(test, node->node_id, LOCAL_BUS | 0x00); + KUNIT_EXPECT_EQ(test, node->port_count, 1); + KUNIT_EXPECT_PTR_EQ(test, node->ports[0], parent); + + ++card->color; + for_each_fw_node(card, card->root_node, release_fw_node); + KUNIT_EXPECT_EQ(test, data->release_count, 4); +} + +static void node_tree_test_four_nodes_case1(struct kunit *test) +{ + // root + // ++============++ + // || phy 3 || + // || P0 P1 P2 || + // ++===|==|==x==++ + // | + // | +--------------------------------+ + // | | +-----------+ | + // ++===|==|==|==++ ++===|==x==x==++ ++===|==|==|==++ + // || P0 P1 P2 || || P0 P1 P2 || || P0 P1 P2 || + // || phy 2 || || phy 1 || || phy 0 || + // ++============++ ++============++ ++============++ + // + static const u32 self_id_sequence[] = { + 0x80000094, + 0x81000080, + 0x820000bc, + 0x830000d0, + }; + struct private_data *data = test->priv; + struct fw_card *card = data->card; + + card->node_id = LOCAL_BUS | 0x03; + + card->local_node = build_tree(card, self_id_sequence, ARRAY_SIZE(self_id_sequence), 123); + KUNIT_EXPECT_NOT_NULL(test, card->local_node); + KUNIT_EXPECT_PTR_EQ(test, card->local_node, card->root_node); + + struct fw_node *node = card->root_node; + KUNIT_EXPECT_EQ(test, node->node_id, LOCAL_BUS | 0x03); + KUNIT_EXPECT_EQ(test, node->port_count, 2); + KUNIT_EXPECT_NOT_NULL(test, node->ports[0]); + KUNIT_EXPECT_NULL(test, node->ports[1]); + + struct fw_node *parent = node; + node = parent->ports[0]; + KUNIT_EXPECT_EQ(test, node->node_id, LOCAL_BUS | 0x02); + KUNIT_EXPECT_EQ(test, node->port_count, 3); + KUNIT_EXPECT_PTR_EQ(test, node->ports[0], parent); + KUNIT_EXPECT_NOT_NULL(test, node->ports[1]); + KUNIT_EXPECT_NOT_NULL(test, node->ports[2]); + + parent = node; + node = parent->ports[2]; + KUNIT_EXPECT_EQ(test, node->node_id, LOCAL_BUS | 0x01); + KUNIT_EXPECT_EQ(test, node->port_count, 1); + KUNIT_EXPECT_PTR_EQ(test, node->ports[0], parent); + + node = parent->ports[1]; + KUNIT_EXPECT_EQ(test, node->node_id, LOCAL_BUS | 0x00); + KUNIT_EXPECT_EQ(test, node->port_count, 3); + KUNIT_EXPECT_PTR_EQ(test, node->ports[0], parent); + KUNIT_EXPECT_NULL(test, node->ports[1]); + KUNIT_EXPECT_NULL(test, node->ports[2]); + + ++card->color; + for_each_fw_node(card, card->root_node, release_fw_node); + KUNIT_EXPECT_EQ(test, data->release_count, 4); +} + +static void node_tree_test_four_nodes_case2(struct kunit *test) +{ + // root + // ++============++ + // || phy 3 || + // || P0 P1 P2 || + // ++===|==|==|==++ + // | | + // | +-----------------------------+ + // | +--------------+ | + // ++===|==|==x==++ ++===|==|==|==++ ++===|==x==x==++ + // || P0 P1 P2 || || P0 P1 P2 || || P0 P1 P2 || + // || phy 1 || || phy 0 || || phy 2 || + // ++============++ ++============++ ++============++ + // + static const u32 self_id_sequence[] = { + 0x80000094, + 0x810000b0, + 0x82000080, + 0x830000dc, + }; + struct private_data *data = test->priv; + struct fw_card *card = data->card; + + card->node_id = LOCAL_BUS | 0x03; + + card->local_node = build_tree(card, self_id_sequence, ARRAY_SIZE(self_id_sequence), 123); + KUNIT_EXPECT_NOT_NULL(test, card->local_node); + KUNIT_EXPECT_PTR_EQ(test, card->local_node, card->root_node); + + struct fw_node *node = card->root_node; + KUNIT_EXPECT_EQ(test, node->node_id, LOCAL_BUS | 0x03); + KUNIT_EXPECT_EQ(test, node->port_count, 3); + KUNIT_EXPECT_NOT_NULL(test, node->ports[0]); + KUNIT_EXPECT_NULL(test, node->ports[1]); + KUNIT_EXPECT_NOT_NULL(test, node->ports[2]); + + struct fw_node *parent = node; + node = parent->ports[2]; + KUNIT_EXPECT_EQ(test, node->node_id, LOCAL_BUS | 0x02); + KUNIT_EXPECT_EQ(test, node->port_count, 1); + KUNIT_EXPECT_PTR_EQ(test, node->ports[0], parent); + + node = parent->ports[0]; + KUNIT_EXPECT_EQ(test, node->node_id, LOCAL_BUS | 0x01); + KUNIT_EXPECT_EQ(test, node->port_count, 2); + KUNIT_EXPECT_PTR_EQ(test, node->ports[0], parent); + KUNIT_EXPECT_NOT_NULL(test, node->ports[1]); + + parent = node; + node = parent->ports[1]; + KUNIT_EXPECT_EQ(test, node->node_id, LOCAL_BUS | 0x00); + KUNIT_EXPECT_EQ(test, node->port_count, 3); + KUNIT_EXPECT_PTR_EQ(test, node->ports[0], parent); + KUNIT_EXPECT_NULL(test, node->ports[1]); + KUNIT_EXPECT_NULL(test, node->ports[2]); + + ++card->color; + for_each_fw_node(card, card->root_node, release_fw_node); + KUNIT_EXPECT_EQ(test, data->release_count, 4); +} + +static void node_tree_test_four_nodes_case3(struct kunit *test) +{ + // root + // ++============++ + // || phy 3 || + // || P0 P1 P2 || + // ++===|==|==|==++ + // | | +--------------------------------+ + // | +--------------------+ | + // | | | + // ++===|==|==x==++ ++===|==|==|==++ ++===|==|==x==++ + // || P0 P1 P2 || || P0 P1 P2 || || P0 P1 P2 || + // || phy 0 || || phy 1 || || phy 2 || + // ++============++ ++============++ ++============++ + // + static const u32 self_id_sequence[] = { + 0x80000090, + 0x81000058, + 0x82000060, + 0x830000fc, + }; + struct private_data *data = test->priv; + struct fw_card *card = data->card; + + card->node_id = LOCAL_BUS | 0x03; + + card->local_node = build_tree(card, self_id_sequence, ARRAY_SIZE(self_id_sequence), 123); + KUNIT_EXPECT_NOT_NULL(test, card->local_node); + KUNIT_EXPECT_PTR_EQ(test, card->local_node, card->root_node); + + struct fw_node *node = card->root_node; + KUNIT_EXPECT_EQ(test, node->node_id, LOCAL_BUS | 0x03); + KUNIT_EXPECT_EQ(test, node->port_count, 3); + KUNIT_EXPECT_NOT_NULL(test, node->ports[0]); + KUNIT_EXPECT_NOT_NULL(test, node->ports[1]); + KUNIT_EXPECT_NOT_NULL(test, node->ports[2]); + + struct fw_node *parent = node; + node = parent->ports[2]; + KUNIT_EXPECT_EQ(test, node->node_id, LOCAL_BUS | 0x02); + KUNIT_EXPECT_EQ(test, node->port_count, 2); + KUNIT_EXPECT_NULL(test, node->ports[0]); + KUNIT_EXPECT_PTR_EQ(test, node->ports[1], parent); + + node = parent->ports[1]; + KUNIT_EXPECT_EQ(test, node->node_id, LOCAL_BUS | 0x01); + KUNIT_EXPECT_EQ(test, node->port_count, 3); + KUNIT_EXPECT_NULL(test, node->ports[0]); + KUNIT_EXPECT_NULL(test, node->ports[1]); + KUNIT_EXPECT_PTR_EQ(test, node->ports[2], parent); + + node = parent->ports[0]; + KUNIT_EXPECT_EQ(test, node->node_id, LOCAL_BUS | 0x00); + KUNIT_EXPECT_EQ(test, node->port_count, 2); + KUNIT_EXPECT_PTR_EQ(test, node->ports[0], parent); + KUNIT_EXPECT_NULL(test, node->ports[1]); + + ++card->color; + for_each_fw_node(card, card->root_node, release_fw_node); + KUNIT_EXPECT_EQ(test, data->release_count, 4); +} static struct kunit_case node_tree_test_cases[] = { + KUNIT_CASE(node_tree_test_two_nodes), + KUNIT_CASE(node_tree_test_two_nodes_1394a), + KUNIT_CASE(node_tree_test_three_nodes_case0), + KUNIT_CASE(node_tree_test_three_nodes_case1), + KUNIT_CASE(node_tree_test_four_nodes_case0), + KUNIT_CASE(node_tree_test_four_nodes_case1), + KUNIT_CASE(node_tree_test_four_nodes_case2), + KUNIT_CASE(node_tree_test_four_nodes_case3), {} }; static struct kunit_suite node_tree_test_suite = { .name = "firewire-node-tree", + .init = node_tree_test_init, + .exit = node_tree_test_exit, .test_cases = node_tree_test_cases, }; kunit_test_suite(node_tree_test_suite); From f744022705b7eb479a1931ddd95aae4e9a4be221 Mon Sep 17 00:00:00 2001 From: Takashi Sakamoto Date: Mon, 10 Aug 2026 15:41:19 +0900 Subject: [PATCH 5/8] firewire: core: add KUnit tests for failure of tree building Abdun Nihaal has reported a memory leak when tree building fails in the middle of self ID sequence enumeration. This is caused by an invalid self ID sequence and is not a common occurrence. This commit is intended to assist in fixing the issue by adding KUnit tests to show the cases in which the memory leak is triggered. The leak occurs internally in the build_tree() function, therefore it cannot be detected directly by the tests. Link: https://lore.kernel.org/r/20260810064119.410324-4-o-takashi@sakamocchi.jp Signed-off-by: Takashi Sakamoto --- drivers/firewire/node-tree-test.c | 84 +++++++++++++++++++++++++++++++ 1 file changed, 84 insertions(+) diff --git a/drivers/firewire/node-tree-test.c b/drivers/firewire/node-tree-test.c index 6f404df3d50e..251c0113639f 100644 --- a/drivers/firewire/node-tree-test.c +++ b/drivers/firewire/node-tree-test.c @@ -506,6 +506,86 @@ static void node_tree_test_four_nodes_case3(struct kunit *test) KUNIT_EXPECT_EQ(test, data->release_count, 4); } +static void node_tree_test_invalid_extended_self_id_sequence(struct kunit *test) +{ + // Use the same node tree as node_tree_test_four_nodes_case1, except for the invalid + // content of self ID packet for the phy 3. + static const u32 self_id_sequence[] = { + 0x80000094, + 0x81000080, + 0x820000bc, + 0x830000d1, // Invalid. + }; + struct private_data *data = test->priv; + struct fw_card *card = data->card; + + card->node_id = LOCAL_BUS | 0x03; + + // TODO: Memory leak. + card->local_node = build_tree(card, self_id_sequence, ARRAY_SIZE(self_id_sequence), 123); + KUNIT_EXPECT_NULL(test, card->local_node); +} + +static void node_tree_test_invalid_phy_id(struct kunit *test) +{ + // Use the same node tree as node_tree_test_four_nodes_case1, except for the invalid + // phy ID for phy 3. + static const u32 self_id_sequence[] = { + 0x80000094, + 0x81000080, + 0x820000bc, + 0x8f0000d0, // Invalid. + }; + struct private_data *data = test->priv; + struct fw_card *card = data->card; + + card->node_id = LOCAL_BUS | 0x03; + + // TODO: Memory leak. + card->local_node = build_tree(card, self_id_sequence, ARRAY_SIZE(self_id_sequence), 123); + KUNIT_EXPECT_NULL(test, card->local_node); +} + +static void node_tree_test_invalid_child_port_count(struct kunit *test) +{ + // Use the same node tree as node_tree_test_four_nodes_case1, except for the invalid + // count of child ports for phy 3. + static const u32 self_id_sequence[] = { + 0x80000094, + 0x81000080, + 0x820000bc, + 0x830000fc, // Invalid. + }; + struct private_data *data = test->priv; + struct fw_card *card = data->card; + + card->node_id = LOCAL_BUS | 0x03; + + // TODO: Memory leak. + card->local_node = build_tree(card, self_id_sequence, ARRAY_SIZE(self_id_sequence), 123); + KUNIT_EXPECT_NULL(test, card->local_node); +} + +static void node_tree_test_invalid_parent_port_count(struct kunit *test) +{ + // Use the same node tree as node_tree_test_four_nodes_case1, except for the invalid + // count of parent ports for phy 3. + static const u32 self_id_sequence[] = { + 0x80000094, + 0x81000080, + 0x820000bc, + 0x830000e8, // Invalid. + }; + struct private_data *data = test->priv; + struct fw_card *card = data->card; + + card->node_id = LOCAL_BUS | 0x03; + + // TODO: Memory leak. + card->local_node = build_tree(card, self_id_sequence, ARRAY_SIZE(self_id_sequence), 123); + KUNIT_EXPECT_NULL(test, card->local_node); +} + static struct kunit_case node_tree_test_cases[] = { KUNIT_CASE(node_tree_test_two_nodes), KUNIT_CASE(node_tree_test_two_nodes_1394a), @@ -515,6 +595,10 @@ static struct kunit_case node_tree_test_cases[] = { KUNIT_CASE(node_tree_test_four_nodes_case1), KUNIT_CASE(node_tree_test_four_nodes_case2), KUNIT_CASE(node_tree_test_four_nodes_case3), + KUNIT_CASE(node_tree_test_invalid_extended_self_id_sequence), + KUNIT_CASE(node_tree_test_invalid_phy_id), + KUNIT_CASE(node_tree_test_invalid_child_port_count), + KUNIT_CASE(node_tree_test_invalid_parent_port_count), {} }; From 87c48e54f28d410af0c12b5dd339307bb43abeea Mon Sep 17 00:00:00 2001 From: Takashi Sakamoto Date: Tue, 11 Aug 2026 21:09:26 +0900 Subject: [PATCH 6/8] firewire: core: consolidate port counting in build_tree() The self ID sequence describes the state of each port for each PHY. Currently, build_tree() counts the ports in two separate places. Consolidate the port counting in one place. Link: https://lore.kernel.org/r/20260811120928.700577-2-o-takashi@sakamocchi.jp Signed-off-by: Takashi Sakamoto --- drivers/firewire/core-topology.c | 18 ++++++++++-------- 1 file changed, 10 insertions(+), 8 deletions(-) diff --git a/drivers/firewire/core-topology.c b/drivers/firewire/core-topology.c index 1d3a4419f554..4f610205576c 100644 --- a/drivers/firewire/core-topology.c +++ b/drivers/firewire/core-topology.c @@ -119,8 +119,8 @@ static struct fw_node *build_tree(struct fw_card *card, const u32 *sid, int self while (enumerator.quadlet_count > 0) { unsigned int child_port_count = 0; + unsigned int parent_port_count = 0; unsigned int total_port_count = 0; - unsigned int parent_count = 0; unsigned int quadlet_count; const u32 *self_id_sequence; unsigned int port_capacity; @@ -148,16 +148,19 @@ static struct fw_node *build_tree(struct fw_card *card, const u32 *sid, int self switch (port_status) { case PHY_PACKET_SELF_ID_PORT_STATUS_CHILD: ++child_port_count; - fallthrough; + break; case PHY_PACKET_SELF_ID_PORT_STATUS_PARENT: + ++parent_port_count; + break; case PHY_PACKET_SELF_ID_PORT_STATUS_NCONN: ++total_port_count; - fallthrough; + break; case PHY_PACKET_SELF_ID_PORT_STATUS_NONE: default: break; } } + total_port_count += child_port_count + parent_port_count; if (phy_id != phy_packet_self_id_get_phy_id(self_id_sequence[0])) { fw_err(card, "PHY ID mismatch in self ID: %d != %d\n", @@ -203,7 +206,6 @@ static struct fw_node *build_tree(struct fw_card *card, const u32 *sid, int self // we temporarily abuse node->color for remembering the entry in // the node->ports array where the parent node should be. Later, // when we handle the parent node, we fix up the reference. - ++parent_count; node->color = port_index; break; @@ -223,10 +225,10 @@ static struct fw_node *build_tree(struct fw_card *card, const u32 *sid, int self // Check that the node reports exactly one parent port, except for the root, which // of course should have no parents. - if ((enumerator.quadlet_count == 0 && parent_count != 0) || - (enumerator.quadlet_count > 0 && parent_count != 1)) { + if ((enumerator.quadlet_count == 0 && parent_port_count != 0) || + (enumerator.quadlet_count > 0 && parent_port_count != 1)) { fw_err(card, "parent port inconsistency for node %d: " - "parent_count=%d\n", phy_id, parent_count); + "parent_count=%d\n", phy_id, parent_port_count); return NULL; } @@ -235,7 +237,7 @@ static struct fw_node *build_tree(struct fw_card *card, const u32 *sid, int self list_add_tail(&node->link, &stack); stack_depth += 1 - child_port_count; - if (node->phy_speed == SCODE_BETA && parent_count + child_port_count > 1) + if (node->phy_speed == SCODE_BETA && parent_port_count + child_port_count > 1) beta_repeaters_present = true; // If PHYs report different gap counts, set an invalid count which will force a gap From a563a7cb645a79a3a7e31bed8cbf95980ef5feb1 Mon Sep 17 00:00:00 2001 From: Takashi Sakamoto Date: Tue, 11 Aug 2026 21:09:27 +0900 Subject: [PATCH 7/8] firewire: core: validate parent port count before allocating nodes in build_tree() The node tree requires each child node to have exactly one port connected to a parent node, while the root node must have no such port. This can be validated by comparing the parent port count for a PHY with the rest of the self ID sequence. Currently, this validation is done after the node has been allocated. Move it before the allocation so that an invalid self ID sequence can cause an error without having to clean up the newly allocated node. Link: https://lore.kernel.org/r/20260811120928.700577-3-o-takashi@sakamocchi.jp Signed-off-by: Takashi Sakamoto --- drivers/firewire/core-topology.c | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/drivers/firewire/core-topology.c b/drivers/firewire/core-topology.c index 4f610205576c..e032497b2594 100644 --- a/drivers/firewire/core-topology.c +++ b/drivers/firewire/core-topology.c @@ -162,6 +162,15 @@ static struct fw_node *build_tree(struct fw_card *card, const u32 *sid, int self } total_port_count += child_port_count + parent_port_count; + // Check that the node reports exactly one parent port, except for the root, which + // of course should have no parents. + if ((enumerator.quadlet_count == 0 && parent_port_count != 0) || + (enumerator.quadlet_count > 0 && parent_port_count != 1)) { + fw_err(card, "parent port inconsistency for node %d: parent_count=%d\n", + phy_id, parent_port_count); + return NULL; + } + if (phy_id != phy_packet_self_id_get_phy_id(self_id_sequence[0])) { fw_err(card, "PHY ID mismatch in self ID: %d != %d\n", phy_id, phy_packet_self_id_get_phy_id(self_id_sequence[0])); @@ -223,15 +232,6 @@ static struct fw_node *build_tree(struct fw_card *card, const u32 *sid, int self } } - // Check that the node reports exactly one parent port, except for the root, which - // of course should have no parents. - if ((enumerator.quadlet_count == 0 && parent_port_count != 0) || - (enumerator.quadlet_count > 0 && parent_port_count != 1)) { - fw_err(card, "parent port inconsistency for node %d: " - "parent_count=%d\n", phy_id, parent_port_count); - return NULL; - } - /* Pop the child nodes off the stack and push the new node. */ __list_del(h->prev, &stack); list_add_tail(&node->link, &stack); From 05bfb1327dc5fb61528bab31cd8f0c1e4bddec23 Mon Sep 17 00:00:00 2001 From: Takashi Sakamoto Date: Tue, 11 Aug 2026 21:09:28 +0900 Subject: [PATCH 8/8] firewire: core: fix memory leak in error path of build_tree() In the error path of build_tree(), node instances can remain in the local linked list when the function returns. Whenever an invalid value is detected in the self ID sequence, each allocated node instance is either an entry in the linked list or an entry in the ports array of its parent node. Therefore, the allocate node instances can be safely released by traversing the linked list from its head. Release the remaining node instances with for_each_fw_node() before returning to the caller. Fixes: 3038e353cfaf ("firewire: Add core firewire stack.") Reported-by: Abdun Nihaal Link: https://lore.kernel.org/all/20260727095955.104972-1-nihaal@cse.iitm.ac.in/ Link: https://lore.kernel.org/r/20260811120928.700577-4-o-takashi@sakamocchi.jp Signed-off-by: Takashi Sakamoto --- drivers/firewire/core-topology.c | 30 +++++++++++++++++++++--------- drivers/firewire/node-tree-test.c | 4 ---- 2 files changed, 21 insertions(+), 13 deletions(-) diff --git a/drivers/firewire/core-topology.c b/drivers/firewire/core-topology.c index e032497b2594..ee6b54f89859 100644 --- a/drivers/firewire/core-topology.c +++ b/drivers/firewire/core-topology.c @@ -88,6 +88,17 @@ static inline struct fw_node *fw_node(struct list_head *l) return list_entry(l, struct fw_node, link); } +typedef void (*fw_node_callback_t)(struct fw_card *card, struct fw_node *node, + struct fw_node *parent); + +static void for_each_fw_node(struct fw_card *card, struct fw_node *root, + fw_node_callback_t callback); + +static void free_fw_node(struct fw_card *card, struct fw_node *node, struct fw_node *parent) +{ + kfree(node); +} + /* * This function builds the tree representation of the topology given * by the self IDs from the latest bus reset. During the construction @@ -134,7 +145,7 @@ static struct fw_node *build_tree(struct fw_card *card, const u32 *sid, int self if (PTR_ERR(self_id_sequence) != -ENODATA) { fw_err(card, "inconsistent extended self IDs: %ld\n", PTR_ERR(self_id_sequence)); - return NULL; + goto error; } break; } @@ -168,18 +179,18 @@ static struct fw_node *build_tree(struct fw_card *card, const u32 *sid, int self (enumerator.quadlet_count > 0 && parent_port_count != 1)) { fw_err(card, "parent port inconsistency for node %d: parent_count=%d\n", phy_id, parent_port_count); - return NULL; + goto error; } if (phy_id != phy_packet_self_id_get_phy_id(self_id_sequence[0])) { fw_err(card, "PHY ID mismatch in self ID: %d != %d\n", phy_id, phy_packet_self_id_get_phy_id(self_id_sequence[0])); - return NULL; + goto error; } if (child_port_count > stack_depth) { fw_err(card, "topology stack underflow\n"); - return NULL; + goto error; } /* @@ -197,7 +208,7 @@ static struct fw_node *build_tree(struct fw_card *card, const u32 *sid, int self node = fw_node_create(self_id_sequence[0], total_port_count, card->color); if (node == NULL) { fw_err(card, "out of memory while building topology\n"); - return NULL; + goto error; } if (phy_id == (card->node_id & 0x3f)) @@ -256,12 +267,13 @@ static struct fw_node *build_tree(struct fw_card *card, const u32 *sid, int self card->beta_repeaters_present = beta_repeaters_present; return local_node; +error: + ++card->color; + list_for_each_entry_safe(node, child, &stack, link) + for_each_fw_node(card, node, free_fw_node); + return NULL; } -typedef void (*fw_node_callback_t)(struct fw_card * card, - struct fw_node * node, - struct fw_node * parent); - static void for_each_fw_node(struct fw_card *card, struct fw_node *root, fw_node_callback_t callback) { diff --git a/drivers/firewire/node-tree-test.c b/drivers/firewire/node-tree-test.c index 251c0113639f..5e6bb58537e9 100644 --- a/drivers/firewire/node-tree-test.c +++ b/drivers/firewire/node-tree-test.c @@ -521,7 +521,6 @@ static void node_tree_test_invalid_extended_self_id_sequence(struct kunit *test) card->node_id = LOCAL_BUS | 0x03; - // TODO: Memory leak. card->local_node = build_tree(card, self_id_sequence, ARRAY_SIZE(self_id_sequence), 123); KUNIT_EXPECT_NULL(test, card->local_node); } @@ -541,7 +540,6 @@ static void node_tree_test_invalid_phy_id(struct kunit *test) card->node_id = LOCAL_BUS | 0x03; - // TODO: Memory leak. card->local_node = build_tree(card, self_id_sequence, ARRAY_SIZE(self_id_sequence), 123); KUNIT_EXPECT_NULL(test, card->local_node); } @@ -561,7 +559,6 @@ static void node_tree_test_invalid_child_port_count(struct kunit *test) card->node_id = LOCAL_BUS | 0x03; - // TODO: Memory leak. card->local_node = build_tree(card, self_id_sequence, ARRAY_SIZE(self_id_sequence), 123); KUNIT_EXPECT_NULL(test, card->local_node); } @@ -581,7 +578,6 @@ static void node_tree_test_invalid_parent_port_count(struct kunit *test) card->node_id = LOCAL_BUS | 0x03; - // TODO: Memory leak. card->local_node = build_tree(card, self_id_sequence, ARRAY_SIZE(self_id_sequence), 123); KUNIT_EXPECT_NULL(test, card->local_node); }