[PATCH v2 2/2] of: overlay: rework overlay apply and remove kfree()s

From: frowand . list
Date: Sun Apr 10 2022 - 17:16:27 EST

From: Frank Rowand <frank.rowand@xxxxxxxx>

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@xxxxxxxxx>

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

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@xxxxxxxx>