On 4/9/22 01:52, Slawomir Stepien wrote:
> On kwi 08, 2022 12:21, frowand.list@xxxxxxxxx wrote:
>> From: Frank Rowand <frank.rowand@xxxxxxxx>
>
> 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 <slawomir.stepien@xxxxxxxxx>
>>
>> Signed-off-by: Frank Rowand <frank.rowand@xxxxxxxx>
>> ---
>> 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:
>
