Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
From:   frowand.list@gmail.com
To:     Rob Herring <robh+dt@kernel.org>, pantelis.antoniou@konsulko.com,
        Slawomir Stepien <slawomir.stepien@nokia.com>
Cc:     devicetree@vger.kernel.org, linux-kernel@vger.kernel.org,
        Slawomir Stepien <sst@poczta.fm>,
        Geert Uytterhoeven <geert+renesas@glider.be>,
        Alan Tull <atull@kernel.org>
Subject: [PATCH v2 2/2] of: overlay: rework overlay apply and remove kfree()s
Date:   Sun, 10 Apr 2022 16:08:33 -0500
Message-Id: <20220410210833.441504-3-frowand.list@gmail.com>
In-Reply-To: <20220410210833.441504-1-frowand.list@gmail.com>
References: <20220410210833.441504-1-frowand.list@gmail.com>
MIME-Version: 1.0
Content-Transfer-Encoding: 8bit
Precedence: bulk

From: Frank Rowand <frank.rowand@sony.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 the root of the unflattened
    overlay (variable tree) instead of the memory that contains
    the unflattened overlay.
  - For the case of a failure during applying an overlay, move kfree()
    of new_fdt and overlay_mem into the function that allocated them.
    For the case of removing an overlay, the kfree() remains in
    free_overlay_changeset().
  - Check return value of of_fdt_unflatten_tree() for error instead
    of checking the returnded value of overlay_root.

More clearly document policy related to lifetime of pointers into
overlay memory.

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

Signed-off-by: Frank Rowand <frank.rowand@sony.com>
---

Changes since v1:
  - Move kfree()s from init_overlay_changeset() to of_overlay_fdt_apply()
  - Better document lifetime of pointers into overlay, both in overlay.c
    and Documentation/devicetree/overlay-notes.rst

 Documentation/devicetree/overlay-notes.rst |  23 +++-
 drivers/of/overlay.c                       | 127 ++++++++++++---------
 2 files changed, 91 insertions(+), 59 deletions(-)

diff --git a/Documentation/devicetree/overlay-notes.rst b/Documentation/devicetree/overlay-notes.rst
index b2b8db765b8c..7a6e85f75567 100644
--- a/Documentation/devicetree/overlay-notes.rst
+++ b/Documentation/devicetree/overlay-notes.rst
@@ -119,10 +119,25 @@ Finally, if you need to remove all overlays in one-go, just call
 of_overlay_remove_all() which will remove every single one in the correct
 order.
 
-In addition, there is the option to register notifiers that get called on
+There is the option to register notifiers that get called on
 overlay operations. See of_overlay_notifier_register/unregister and
 enum of_overlay_notify_action for details.
 
-Note that a notifier callback is not supposed to store pointers to a device
-tree node or its content beyond OF_OVERLAY_POST_REMOVE corresponding to the
-respective node it received.
+A notifier callback for OF_OVERLAY_PRE_APPLY, OF_OVERLAY_POST_APPLY, or
+OF_OVERLAY_PRE_REMOVE may store pointers to a device tree node in the overlay
+or its content but these pointers must not persist past the notifier callback
+for OF_OVERLAY_POST_REMOVE.  The memory containing the overlay will be
+kfree()ed after OF_OVERLAY_POST_REMOVE notifiers are called.  Note that the
+memory will be kfree()ed even if the notifier for OF_OVERLAY_POST_REMOVE
+returns an error.
+
+The changeset notifiers in drivers/of/dynamic.c are a second type of notifier
+that could be triggered by applying or removing an overlay.  These notifiers
+are not allowed to store pointers to a device tree node in the overlay
+or its content.  The overlay code does not protect against such pointers
+remaining active when the memory containing the overlay is freed as a result
+of removing the overlay.
+
+Any other code that retains a pointer to the overlay nodes or data is
+considered to be a bug because after removing the overlay the pointer
+will refer to freed memory.
diff --git a/drivers/of/overlay.c b/drivers/of/overlay.c
index f74aa9ff67aa..c8e999518f2f 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
  * @new_fdt:		Memory allocated to hold unflattened aligned FDT
+ * @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 *new_fdt;
+	const void *overlay_mem;
 	struct device_node *overlay_tree;
 	int count;
 	struct fragment *fragments;
@@ -720,18 +722,20 @@ 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
  * @new_fdt:		Memory allocated to hold unflattened aligned FDT
+ * @tree_mem:		Memory that contains @overlay_tree
  * @overlay_tree:	Contains the overlay fragments and overlay fixup nodes
  *
  * Initialize @ovcs.  Populate @ovcs->fragments with node information from
  * the top level of @overlay_tree.  The relevant top level nodes are the
  * fragment nodes and the __symbols__ node.  Any other top level node will
- * be ignored.
+ * be ignored.  Populate other @ovcs fields.
  *
  * Return: 0 on success, -ENOMEM if memory allocation failure, -EINVAL if error
  * detected in @overlay_tree, or -ENOSPC if idr_alloc() error.
  */
 static int init_overlay_changeset(struct overlay_changeset *ovcs,
-		const void *new_fdt, struct device_node *overlay_tree)
+		const void *new_fdt, const void *tree_mem,
+		struct device_node *overlay_tree)
 {
 	struct device_node *node, *overlay_node;
 	struct fragment *fragment;
@@ -751,9 +755,6 @@ static int init_overlay_changeset(struct overlay_changeset *ovcs,
 	if (!of_node_is_root(overlay_tree))
 		pr_debug("%s() overlay_tree is not root\n", __func__);
 
-	ovcs->overlay_tree = overlay_tree;
-	ovcs->new_fdt = new_fdt;
-
 	INIT_LIST_HEAD(&ovcs->ovcs_list);
 
 	of_changeset_init(&ovcs->cset);
@@ -832,6 +833,9 @@ static int init_overlay_changeset(struct overlay_changeset *ovcs,
 
 	ovcs->id = id;
 	ovcs->count = cnt;
+	ovcs->new_fdt = new_fdt;
+	ovcs->overlay_mem = tree_mem;
+	ovcs->overlay_tree = overlay_tree;
 	ovcs->fragments = fragments;
 
 	return 0;
@@ -846,7 +850,7 @@ static int init_overlay_changeset(struct overlay_changeset *ovcs,
 	return ret;
 }
 
-static void free_overlay_changeset(struct overlay_changeset *ovcs)
+static void free_overlay_changeset_contents(struct overlay_changeset *ovcs)
 {
 	int i;
 
@@ -861,12 +865,20 @@ static void free_overlay_changeset(struct overlay_changeset *ovcs)
 		of_node_put(ovcs->fragments[i].overlay);
 	}
 	kfree(ovcs->fragments);
+}
+static void free_overlay_changeset(struct overlay_changeset *ovcs)
+{
+
+	free_overlay_changeset_contents(ovcs);
+
 	/*
-	 * There should be no live pointers into ovcs->overlay_tree and
+	 * There should be no live pointers into ovcs->overlay_mem and
 	 * ovcs->new_fdt due to the policy that overlay notifiers are not
-	 * allowed to retain pointers into the overlay devicetree.
+	 * allowed to retain pointers into the overlay devicetree other
+	 * than the window between OF_OVERLAY_PRE_APPLY overlay notifiers
+	 * and the OF_OVERLAY_POST_REMOVE overlay notifiers.
 	 */
-	kfree(ovcs->overlay_tree);
+	kfree(ovcs->overlay_mem);
 	kfree(ovcs->new_fdt);
 	kfree(ovcs);
 }
@@ -876,8 +888,10 @@ static void free_overlay_changeset(struct overlay_changeset *ovcs)
  *
  * of_overlay_apply() - Create and apply an overlay changeset
  * @new_fdt:		Memory allocated to hold the aligned FDT
+ * @tree_mem:		Memory that contains @overlay_tree
  * @overlay_tree:	Expanded overlay device tree
  * @ovcs_id:		Pointer to overlay changeset id
+ * @kfree_unsafe:	Pointer to flag to not kfree() @new_fdt and @overlay_tree
  *
  * Creates and applies an overlay changeset.
  *
@@ -910,34 +924,25 @@ static void free_overlay_changeset(struct overlay_changeset *ovcs)
  * refused.
  *
  * Returns 0 on success, or a negative error number.  Overlay changeset
- * id is returned to *ovcs_id.
+ * id is returned to *ovcs_id.  When references to @new_fdt and @overlay_tree
+ * may exist, *kfree_unsafe is set to true.
  */
 
-static int of_overlay_apply(const void *new_fdt,
-		struct device_node *overlay_tree, int *ovcs_id)
+static int of_overlay_apply(const void *new_fdt, void *tree_mem,
+		struct device_node *overlay_tree, int *ovcs_id,
+		bool *kfree_unsafe)
 {
 	struct overlay_changeset *ovcs;
 	int ret = 0, ret_revert, ret_tmp;
 
-	/*
-	 * As of this point, new_fdt and overlay_tree 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(new_fdt);
-		kfree(overlay_tree);
-		ret = -EBUSY;
-		goto out;
+		return -EBUSY;
 	}
 
 	ovcs = kzalloc(sizeof(*ovcs), GFP_KERNEL);
 	if (!ovcs) {
-		kfree(new_fdt);
-		kfree(overlay_tree);
-		ret = -ENOMEM;
-		goto out;
+		return -ENOMEM;
 	}
 
 	of_overlay_mutex_lock();
@@ -945,28 +950,27 @@ static int of_overlay_apply(const void *new_fdt,
 
 	ret = of_resolve_phandles(overlay_tree);
 	if (ret)
-		goto err_free_overlay_tree;
+		goto err_free_ovcs;
 
-	ret = init_overlay_changeset(ovcs, new_fdt, overlay_tree);
+	ret = init_overlay_changeset(ovcs, new_fdt, tree_mem, overlay_tree);
 	if (ret)
-		goto err_free_overlay_tree;
+		goto err_free_ovcs_contents;
 
 	/*
-	 * after overlay_notify(), ovcs->overlay_tree related pointers may have
-	 * leaked to drivers, so can not kfree() overlay_tree,
-	 * aka ovcs->overlay_tree; and can not free memory containing aligned
-	 * fdt.  The aligned fdt is contained within the memory at
-	 * ovcs->new_fdt, possibly at an offset from ovcs->new_fdt.
+	 * After overlay_notify(), ovcs->overlay_tree related pointers may have
+	 * leaked to drivers, so can not kfree() ovcs->overlay_mem and
+	 * ovcs->new_fdt until after OF_OVERLAY_POST_REMOVE notifiers.
 	 */
+	*kfree_unsafe = true;
 	ret = overlay_notify(ovcs, OF_OVERLAY_PRE_APPLY);
 	if (ret) {
 		pr_err("overlay changeset pre-apply notify error %d\n", ret);
-		goto err_free_overlay_changeset;
+		goto err_free_ovcs_contents;
 	}
 
 	ret = build_changeset(ovcs);
 	if (ret)
-		goto err_free_overlay_changeset;
+		goto err_free_ovcs_contents;
 
 	ret_revert = 0;
 	ret = __of_changeset_apply_entries(&ovcs->cset, &ret_revert);
@@ -976,7 +980,7 @@ static int of_overlay_apply(const void *new_fdt,
 				 ret_revert);
 			devicetree_state_flags |= DTSF_APPLY_FAIL;
 		}
-		goto err_free_overlay_changeset;
+		goto err_free_ovcs_contents;
 	}
 
 	ret = __of_changeset_apply_notify(&ovcs->cset);
@@ -997,18 +1001,16 @@ static int of_overlay_apply(const void *new_fdt,
 
 	goto out_unlock;
 
-err_free_overlay_tree:
-	kfree(new_fdt);
-	kfree(overlay_tree);
+err_free_ovcs_contents:
+	free_overlay_changeset_contents(ovcs);
 
-err_free_overlay_changeset:
-	free_overlay_changeset(ovcs);
+err_free_ovcs:
+	kfree(ovcs);
 
 out_unlock:
 	mutex_unlock(&of_mutex);
 	of_overlay_mutex_unlock();
 
-out:
 	pr_debug("%s() err=%d\n", __func__, ret);
 
 	return ret;
@@ -1019,11 +1021,14 @@ int of_overlay_fdt_apply(const void *overlay_fdt, u32 overlay_fdt_size,
 {
 	void *new_fdt;
 	void *new_fdt_align;
+	void *overlay_mem;
+	bool kfree_unsafe;
 	int ret;
 	u32 size;
 	struct device_node *overlay_root = NULL;
 
 	*ovcs_id = 0;
+	kfree_unsafe = false;
 
 	if (overlay_fdt_size < sizeof(struct fdt_header) ||
 	    fdt_check_header(overlay_fdt)) {
@@ -1046,30 +1051,37 @@ 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);
-	if (ret < 0) {
-		/*
-		 * new_fdt and overlay_root now belong to the overlay
-		 * changeset.
-		 * overlay changeset code is responsible for freeing them.
-		 */
-		goto out;
-	}
+	ret = of_overlay_apply(new_fdt, overlay_mem, overlay_root, ovcs_id,
+			&kfree_unsafe);
+	if (ret < 0)
+		goto out_free_overlay_mem;
 
+	/*
+	 * new_fdt and overlay_mem now belong to the overlay changeset.
+	 * free_overlay_changeset() is responsible for freeing them.
+	 */
 	return 0;
 
+	/*
+	 * After overlay_notify(), ovcs->overlay_tree related pointers may have
+	 * leaked to drivers, so can not kfree() overlay_mem and new_fdt.  This
+	 * will result in a memory leak.
+	 */
+out_free_overlay_mem:
+	if (!kfree_unsafe)
+		kfree(overlay_mem);
 
 out_free_new_fdt:
-	kfree(new_fdt);
+	if (!kfree_unsafe)
+		kfree(new_fdt);
 
-out:
 	return ret;
 }
 EXPORT_SYMBOL_GPL(of_overlay_fdt_apply);
@@ -1237,6 +1249,11 @@ int of_overlay_remove(int *ovcs_id)
 
 	*ovcs_id = 0;
 
+	/*
+	 * Note that the overlay memory will be kfree()ed by
+	 * free_overlay_changeset() even if the notifier for
+	 * OF_OVERLAY_POST_REMOVE returns an error.
+	 */
 	ret_tmp = overlay_notify(ovcs, OF_OVERLAY_POST_REMOVE);
 	if (ret_tmp) {
 		pr_err("overlay changeset post-remove notify error %d\n",
-- 
Frank Rowand <frank.rowand@sony.com>