2022-04-08 21:12:37

by Frank Rowand

[permalink] [raw]
Subject: [PATCH 1/1] of: overlay: of_overlay_apply() kfree() errors

From: Frank Rowand <[email protected]>

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 <[email protected]>

Signed-off-by: Frank Rowand <[email protected]>
---
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 <[email protected]>


2022-04-11 04:11:11

by Rob Herring

[permalink] [raw]
Subject: Re: [PATCH 1/1] of: overlay: of_overlay_apply() kfree() errors

On Fri, Apr 8, 2022 at 12:21 PM <[email protected]> wrote:
>
> From: Frank Rowand <[email protected]>

The subject needs a verb.

>
> 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:).

What about my question/suggestion on the original patch from Slawomir?

>
> Double kfree()
> Reported-by: Slawomir Stepien <[email protected]>
>
> Signed-off-by: Frank Rowand <[email protected]>
> ---
> 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 <[email protected]>
>

2022-04-11 06:58:09

by Frank Rowand

[permalink] [raw]
Subject: Re: [PATCH 1/1] of: overlay: of_overlay_apply() kfree() errors

On 4/8/22 12:34, Rob Herring wrote:
> On Fri, Apr 8, 2022 at 12:21 PM <[email protected]> wrote:
>>
>> From: Frank Rowand <[email protected]>
>
> The subject needs a verb.
>
>>
>> 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:).
>
> What about my question/suggestion on the original patch from Slawomir?

Thanks for pointing those out. I have now replied in that email thread
and v2 of my patch will be much better due to your comments there.

-Frank

>
>>
>> Double kfree()
>> Reported-by: Slawomir Stepien <[email protected]>
>>
>> Signed-off-by: Frank Rowand <[email protected]>
>> ---
>> 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 <[email protected]>
>>

2022-04-11 09:02:11

by Slawomir Stepien

[permalink] [raw]
Subject: Re: [PATCH 1/1] of: overlay: of_overlay_apply() kfree() errors

On kwi 08, 2022 12:21, [email protected] wrote:
> From: Frank Rowand <[email protected]>

Hi Frank!

See my one comment below.

> 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 <[email protected]>
>
> Signed-off-by: Frank Rowand <[email protected]>
> ---
> 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);

There is no need to free this overlay_mem. You can end up here only when
overlay_mem is NULL.

> kfree(new_fdt);
>
> out:

--
Slawomir Stepien

2022-04-11 12:33:18

by Frank Rowand

[permalink] [raw]
Subject: Re: [PATCH 1/1] of: overlay: of_overlay_apply() kfree() errors

On 4/8/22 12:34, Rob Herring wrote:
> On Fri, Apr 8, 2022 at 12:21 PM <[email protected]> wrote:
>>
>> From: Frank Rowand <[email protected]>
>
> The subject needs a verb.
>
>>
>> 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:).
>
> What about my question/suggestion on the original patch from Slawomir?
>

I lost track of your email while investigating the problem. I'll go
back and look at your suggestion and question.

-Frank

2022-04-12 20:20:42

by Frank Rowand

[permalink] [raw]
Subject: Re: [PATCH 1/1] of: overlay: of_overlay_apply() kfree() errors

On 4/9/22 01:52, Slawomir Stepien wrote:
> On kwi 08, 2022 12:21, [email protected] wrote:
>> From: Frank Rowand <[email protected]>
>
> Hi Frank!
>
> See my one comment below.
>
>> 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 <[email protected]>
>>
>> Signed-off-by: Frank Rowand <[email protected]>
>> ---
>> 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);
>
> There is no need to free this overlay_mem. You can end up here only when
> overlay_mem is NULL.

Thanks for catching this.

There will be a new very different version of this patch reflecting Rob's
comments.

-Frank

>
>> kfree(new_fdt);
>>
>> out:
>