| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <YlVTE35t+BnXTEEm@t480s.localdomain>
Date: Tue, 12 Apr 2022 12:23:15 +0200
From: Slawomir Stepien <sst@...zta.fm>
To: Frank Rowand <frowand.list@...il.com>
Cc: Rob Herring <robh+dt@...nel.org>, pantelis.antoniou@...sulko.com,
Slawomir Stepien <slawomir.stepien@...ia.com>,
devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
Geert Uytterhoeven <geert+renesas@...der.be>,
Alan Tull <atull@...nel.org>,
Jan Kiszka <jan.kiszka@...mens.com>
Subject: Re: [PATCH v2 2/2] of: overlay: rework overlay apply and remove
kfree()s
On kwi 10, 2022 23:11, Frank Rowand wrote:
> adding cc: Jan Kiszka <jan.kiszka@...mens.com>
>
> On 4/10/22 16:08, frowand.list@...il.com wrote:
> > 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 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@...ia.com>
> >
> > Signed-off-by: Frank Rowand <frank.rowand@...y.com>
Hi Frank
It looks good to me.
Reviewed-by: Slawomir Stepien <slawomir.stepien@...ia.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",
>
--
Slawomir Stepien
Powered by blists - more mailing lists