Message-Id: <20220408172103.371637-1-frowand.list@gmail.com>
Date:   Fri,  8 Apr 2022 12:21:03 -0500
From:   frowand.list@...il.com
To:     Rob Herring <robh+dt@...nel.org>, pantelis.antoniou@...sulko.com,
        Slawomir Stepien <slawomir.stepien@...ia.com>
Cc:     devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
        Slawomir Stepien <sst@...zta.fm>,
        Geert Uytterhoeven <geert+renesas@...der.be>,
        Alan Tull <atull@...nel.org>
Subject: [PATCH 1/1] of: overlay: of_overlay_apply() kfree() errors

From: Frank Rowand <frank.rowand@...y.com>

Fix various kfree() issues related to of_overlay_apply().
  - Double kfree() of fdt and tree when init_overlay_changeset()
    returns an error.
  - free_overlay_changeset() free of the root of the unflattened
    overlay (variable tree) instead of the memory that contains
    the unflattened overlay.
  - Move similar kfree()s from multiple error locations to a
    common error path (err_free_tree_unlocked:).

Double kfree()
Reported-by: Slawomir Stepien <slawomir.stepien@...ia.com>

Signed-off-by: Frank Rowand <frank.rowand@...y.com>
---
 drivers/of/overlay.c | 64 ++++++++++++++++++++++++++++----------------
 1 file changed, 41 insertions(+), 23 deletions(-)

diff --git a/drivers/of/overlay.c b/drivers/of/overlay.c
index d80160cf34bb..1b9a90d61496 100644
--- a/drivers/of/overlay.c
+++ b/drivers/of/overlay.c
@@ -58,6 +58,7 @@ struct fragment {
  * @id:			changeset identifier
  * @ovcs_list:		list on which we are located
  * @fdt:		base of memory allocated to hold aligned FDT that was unflattened to create @overlay_tree
+ * @overlay_mem:	the memory chunk that contains @overlay_tree
  * @overlay_tree:	expanded device tree that contains the fragment nodes
  * @count:		count of fragment structures
  * @fragments:		fragment nodes in the overlay expanded device tree
@@ -68,6 +69,7 @@ struct overlay_changeset {
 	int id;
 	struct list_head ovcs_list;
 	const void *fdt;
+	const void *overlay_mem;
 	struct device_node *overlay_tree;
 	int count;
 	struct fragment *fragments;
@@ -720,6 +722,7 @@ static struct device_node *find_target(struct device_node *info_node)
  * init_overlay_changeset() - initialize overlay changeset from overlay tree
  * @ovcs:	Overlay changeset to build
  * @fdt:	base of memory allocated to hold aligned FDT that was unflattened to create @tree
+ * @tree_mem:	Memory that contains @tree
  * @tree:	Contains the overlay fragments and overlay fixup nodes
  *
  * Initialize @ovcs.  Populate @ovcs->fragments with node information from
@@ -730,13 +733,23 @@ static struct device_node *find_target(struct device_node *info_node)
  * detected in @tree, or -ENOSPC if idr_alloc() error.
  */
 static int init_overlay_changeset(struct overlay_changeset *ovcs,
-		const void *fdt, struct device_node *tree)
+		const void *fdt, const void *tree_mem, struct device_node *tree)
 {
 	struct device_node *node, *overlay_node;
 	struct fragment *fragment;
 	struct fragment *fragments;
 	int cnt, id, ret;
 
+	/*
+	 * Must set these fields before any error return. fdt and tree_mem
+	 * will be freed by free_overlay_changeset(), which is called if
+	 * init_overlay_changeset() returns an error.
+	 */
+
+	ovcs->fdt = fdt;
+	ovcs->overlay_mem = tree_mem;
+	ovcs->overlay_tree = tree;
+
 	/*
 	 * Warn for some issues.  Can not return -EINVAL for these until
 	 * of_unittest_apply_overlay() is fixed to pass these checks.
@@ -750,9 +763,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);
@@ -865,7 +875,7 @@ static void free_overlay_changeset(struct overlay_changeset *ovcs)
 	 * ovcs->fdt due to the policy that overlay notifiers are not allowed
 	 * to retain pointers into the overlay devicetree.
 	 */
-	kfree(ovcs->overlay_tree);
+	kfree(ovcs->overlay_mem);
 	kfree(ovcs->fdt);
 	kfree(ovcs);
 }
@@ -875,6 +885,7 @@ static void free_overlay_changeset(struct overlay_changeset *ovcs)
  *
  * of_overlay_apply() - Create and apply an overlay changeset
  * @fdt:	base of memory allocated to hold the aligned FDT
+ * @tree_mem:	Memory that contains @tree
  * @tree:	Expanded overlay device tree
  * @ovcs_id:	Pointer to overlay changeset id
  *
@@ -913,31 +924,27 @@ static void free_overlay_changeset(struct overlay_changeset *ovcs)
  * id is returned to *ovcs_id.
  */
 
-static int of_overlay_apply(const void *fdt, struct device_node *tree,
-		int *ovcs_id)
+static int of_overlay_apply(const void *fdt, void *tree_mem,
+		struct device_node *tree, int *ovcs_id)
 {
 	struct overlay_changeset *ovcs;
 	int ret = 0, ret_revert, ret_tmp;
 
 	/*
-	 * As of this point, fdt and tree belong to the overlay changeset.
+	 * As of this point, fdt and tree_mem belong to the overlay changeset.
 	 * overlay changeset code is responsible for freeing them.
 	 */
 
 	if (devicetree_corrupt()) {
 		pr_err("devicetree state suspect, refuse to apply overlay\n");
-		kfree(fdt);
-		kfree(tree);
 		ret = -EBUSY;
-		goto out;
+		goto err_free_tree_unlocked;
 	}
 
 	ovcs = kzalloc(sizeof(*ovcs), GFP_KERNEL);
 	if (!ovcs) {
-		kfree(fdt);
-		kfree(tree);
 		ret = -ENOMEM;
-		goto out;
+		goto err_free_tree_unlocked;
 	}
 
 	of_overlay_mutex_lock();
@@ -947,9 +954,14 @@ static int of_overlay_apply(const void *fdt, struct device_node *tree,
 	if (ret)
 		goto err_free_tree;
 
-	ret = init_overlay_changeset(ovcs, fdt, tree);
+	/*
+	 * init_overlay_changeset() promises to add tree_mem and tree to ovcs
+	 * even in the case of an early error return, so they can be freed by
+	 * free_overlay_changeset().
+	 */
+	ret = init_overlay_changeset(ovcs, fdt, tree_mem, tree);
 	if (ret)
-		goto err_free_tree;
+		goto err_free_overlay_changeset;
 
 	/*
 	 * after overlay_notify(), ovcs->overlay_tree related pointers may have
@@ -999,7 +1011,7 @@ static int of_overlay_apply(const void *fdt, struct device_node *tree,
 
 err_free_tree:
 	kfree(fdt);
-	kfree(tree);
+	kfree(tree_mem);
 
 err_free_overlay_changeset:
 	free_overlay_changeset(ovcs);
@@ -1008,9 +1020,14 @@ static int of_overlay_apply(const void *fdt, struct device_node *tree,
 	mutex_unlock(&of_mutex);
 	of_overlay_mutex_unlock();
 
-out:
 	pr_debug("%s() err=%d\n", __func__, ret);
 
+	return ret;
+
+err_free_tree_unlocked:
+	kfree(fdt);
+	kfree(tree_mem);
+
 	return ret;
 }
 
@@ -1019,6 +1036,7 @@ int of_overlay_fdt_apply(const void *overlay_fdt, u32 overlay_fdt_size,
 {
 	void *new_fdt;
 	void *new_fdt_align;
+	void *overlay_mem;
 	int ret;
 	u32 size;
 	struct device_node *overlay_root = NULL;
@@ -1046,18 +1064,17 @@ int of_overlay_fdt_apply(const void *overlay_fdt, u32 overlay_fdt_size,
 	new_fdt_align = PTR_ALIGN(new_fdt, FDT_ALIGN_SIZE);
 	memcpy(new_fdt_align, overlay_fdt, size);
 
-	of_fdt_unflatten_tree(new_fdt_align, NULL, &overlay_root);
-	if (!overlay_root) {
+	overlay_mem = of_fdt_unflatten_tree(new_fdt_align, NULL, &overlay_root);
+	if (!overlay_mem) {
 		pr_err("unable to unflatten overlay_fdt\n");
 		ret = -EINVAL;
 		goto out_free_new_fdt;
 	}
 
-	ret = of_overlay_apply(new_fdt, overlay_root, ovcs_id);
+	ret = of_overlay_apply(new_fdt, overlay_mem, overlay_root, ovcs_id);
 	if (ret < 0) {
 		/*
-		 * new_fdt and overlay_root now belong to the overlay
-		 * changeset.
+		 * new_fdt and overlay_mem now belong to the overlay changeset.
 		 * overlay changeset code is responsible for freeing them.
 		 */
 		goto out;
@@ -1067,6 +1084,7 @@ int of_overlay_fdt_apply(const void *overlay_fdt, u32 overlay_fdt_size,
 
 
 out_free_new_fdt:
+	kfree(overlay_mem);
 	kfree(new_fdt);
 
 out:
-- 
Frank Rowand <frank.rowand@...y.com>

Powered by blists - more mailing lists