diff mbox series

of: overlay: set 'overlay_tree' and 'fdt' fields only on success

Message ID YkV60TQ+d3sltkNU@t480s.localdomain
State New
Headers show
Series of: overlay: set 'overlay_tree' and 'fdt' fields only on success | expand

Commit Message

Slawomir Stepien March 31, 2022, 9:56 a.m. UTC 
From: Slawomir Stepien <slawomir.stepien@nokia.com>

Before this change, the memory pointed by fields 'overlay_tree' and
'fdt' will be double freed by a call to free_overlay_changeset() from
of_overlay_apply(), when the init_overlay_changeset() fails.

The first free will happen under 'err_free_tree' label and for the
second time under 'err_free_overlay_changeset' label, where we call
free_overlay_changeset().

This could happen for example, when you are applying an overlay to a
target path that does not exists.

By setting the pointers only when we are sure that
init_overlay_changeset() will not fail, will prevent this double free.

Signed-off-by: Slawomir Stepien <slawomir.stepien@nokia.com>
---
 drivers/of/overlay.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

Comments

Rob Herring April 4, 2022, 8:38 p.m. UTC | #1 
On Thu, Mar 31, 2022 at 11:56:33AM +0200, Slawomir Stepien wrote:
> From: Slawomir Stepien <slawomir.stepien@nokia.com>
> 
> Before this change, the memory pointed by fields 'overlay_tree' and
> 'fdt' will be double freed by a call to free_overlay_changeset() from
> of_overlay_apply(), when the init_overlay_changeset() fails.
> 
> The first free will happen under 'err_free_tree' label and for the
> second time under 'err_free_overlay_changeset' label, where we call
> free_overlay_changeset().
> 
> This could happen for example, when you are applying an overlay to a
> target path that does not exists.
> 
> By setting the pointers only when we are sure that
> init_overlay_changeset() will not fail, will prevent this double free.

It looks to me like the free should just be moved from 
init_overlay_changeset() to of_overlay_fdt_apply() where the allocation 
is done. Frank?

Also, I believe there's a bug that of_overlay_apply() should be passed 
new_fdt_align rather than new_fdt. It's only a bug if we expect 
overlay_changeset.fdt to point to a valid fdt rather than the memory 
allocation.

Rob
diff mbox series

Patch

diff --git a/drivers/of/overlay.c b/drivers/of/overlay.c
index d80160cf34bb..a72a9a415f8f 100644
--- a/drivers/of/overlay.c
+++ b/drivers/of/overlay.c
@@ -750,9 +750,6 @@  static int init_overlay_changeset(struct overlay_changeset *ovcs,
 	if (!of_node_is_root(tree))
 		pr_debug("%s() tree is not root\n", __func__);
 
-	ovcs->overlay_tree = tree;
-	ovcs->fdt = fdt;
-
 	INIT_LIST_HEAD(&ovcs->ovcs_list);
 
 	of_changeset_init(&ovcs->cset);
@@ -829,6 +826,8 @@  static int init_overlay_changeset(struct overlay_changeset *ovcs,
 		goto err_free_fragments;
 	}
 
+	ovcs->overlay_tree = tree;
+	ovcs->fdt = fdt;
 	ovcs->id = id;
 	ovcs->count = cnt;
 	ovcs->fragments = fragments;