Re: [PATCH] firewire: core: fix possible memory leak in build_tree()

[Takashi Sakamoto]

Hi,

Thanks for the fix.

On Mon, Jul 27, 2026 at 03:29:53PM +0530, Abdun Nihaal wrote:
> The memory allocated for node in fw_node_create() is not freed when a
> parent port inconsistency error occurs. Fix that by adding a kfree().
> 
> Fixes: 3038e353cfaf ("firewire: Add core firewire stack.")
> Cc: stable@xxxxxxxxxxxxxxx
> Signed-off-by: Abdun Nihaal <nihaal@xxxxxxxxxxxxxx>
> ---
> Compile tested only. Issue found using static analysis.
> 
>  drivers/firewire/core-topology.c | 1 +
>  1 file changed, 1 insertion(+)
 
Indeed, there is a case to leak the memory object for the node found
newly. However, I have a slight concern that the simple call of kfree()
would cause another issue, perhaps.

> diff --git a/drivers/firewire/core-topology.c b/drivers/firewire/core-topology.c
> index df2ac0dab106..317f8f57e521 100644
> --- a/drivers/firewire/core-topology.c
> +++ b/drivers/firewire/core-topology.c
> @@ -227,6 +227,7 @@ static struct fw_node *build_tree(struct fw_card *card, const u32 *sid, int self
>              (enumerator.quadlet_count > 0 && parent_count != 1)) {
>              fw_err(card, "parent port inconsistency for node %d: "
>                     "parent_count=%d\n", phy_id, parent_count);
> +            kfree(node);
>              return NULL;
>          }

When arriving at the line, all of children nodes have the reference to
the issued node.

======== 8< --------
250                 for (i = 0; i < port_count; i++) {
251                         switch (get_port_type(sid, i)) {
252                         case SELFID_PORT_PARENT:
                                    ...
265                                 break;
266 
267                         case SELFID_PORT_CHILD:
                                    ...
273                                 child->ports[child->color] = node;
                                    ...
276                                 break;
277                         }
278                 }
======== 8< --------

I guess it required to invalidate the above references, since the
reference would be used to release the child node.

======== 8< --------
diff --git a/drivers/firewire/core-topology.c b/drivers/firewire/core-topology.c
index 75a6b0df670a..8bb6aa1b5d54 100644
--- a/drivers/firewire/core-topology.c
+++ b/drivers/firewire/core-topology.c
@@ -286,6 +286,11 @@ static struct fw_node *build_tree(struct fw_card *card,
                    (next_sid < end && parent_count != 1)) {
                        fw_err(card, "parent port inconsistency for node %d: "
                               "parent_count=%d\n", phy_id, parent_count);
+                       for (i = 0; i < child->port_count; ++i) {
+                               if (child->ports[i] == node)
+                                       child->ports[i] = NULL;
+                       }
+                       kfree(node);
                        return NULL;
                }
======== 8< --------

I'd like you to recheck the above point, since the above perspective comes
from my rough code review.


Thanks

Takashi Sakamoto