| 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: <272758bf-5fbe-c4e2-79dc-7242d4a3a776@gmail.com>
Date: Wed, 20 Apr 2022 00:33:36 -0500
From: Frank Rowand <frowand.list@...il.com>
To: Rob Herring <robh@...nel.org>
Cc: pantelis.antoniou@...sulko.com,
Slawomir Stepien <slawomir.stepien@...ia.com>,
devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
Slawomir Stepien <sst@...zta.fm>,
Jan Kiszka <jan.kiszka@...mens.com>,
Geert Uytterhoeven <geert+renesas@...der.be>,
Alan Tull <atull@...nel.org>
Subject: Re: [PATCH v3 2/2] of: overlay: rework overlay apply and remove
kfree()s
On 4/19/22 13:56, Rob Herring wrote:
> On Mon, Apr 18, 2022 at 07:52:41PM -0500, 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 free_overlay_changeset(), which
>> is called by the function that allocated them.
>> - For the case of removing an overlay, the kfree() of new_fdt and
>> overlay_mem remains in free_overlay_changeset().
>
> You never set kfree_unsafe back to false anywhere, so after removing you
> still leak memory.
Embarrassing and ironic that I would leave that line out. There should
be an ovcs->kfree_unsafe = false immediately before
"overlay_notify(ovcs, OF_OVERLAY_POST_REMOVE)" in of_overlay_remove().
>
>> - Check return value of of_fdt_unflatten_tree() for error instead
>> of checking the returned value of overlay_root.
>> - When storing pointers to allocated objects in ovcs, do so as
>> near to the allocation as possible instead of in deeply layered
>> function.
>>
>> 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>
>> ---
>>
>> Changes since v2:
>> - A version 2 review comment correctly said "This screams hack".
>> Restructure as listed below in response to the comment.
>> - Quit passing kfree_unsafe in function parameters, move it to
>> be a field of ovcs
>
> What I meant was store the notifier state and from that imply when kfree
> is unsafe. Something like this patch on top of yours (untested and still
> some kfree_unsafe comments need to be updated):
Got it. I like the approach you show below. Patch v4 should be appearing
Tuesday.
-Frank
>
> diff --git a/drivers/of/overlay.c b/drivers/of/overlay.c
> index 3072dfeca8e8..53c616f576d2 100644
> --- a/drivers/of/overlay.c
> +++ b/drivers/of/overlay.c
> @@ -63,7 +63,7 @@ struct fragment {
> * @count: count of fragment structures
> * @fragments: fragment nodes in the overlay expanded device tree
> * @symbols_fragment: last element of @fragments[] is the __symbols__ node
> - * @kfree_unsafe: pointers into the @new_fdt or @overlay_mem may exist
> + * @notify_state: the last successful notifier called
> * @cset: changeset to apply fragments to live device tree
> */
> struct overlay_changeset {
> @@ -75,7 +75,7 @@ struct overlay_changeset {
> int count;
> struct fragment *fragments;
> bool symbols_fragment;
> - bool kfree_unsafe;
> + enum of_overlay_notify_action notify_state;
> struct of_changeset cset;
> };
>
> @@ -183,6 +183,8 @@ static int overlay_notify(struct overlay_changeset *ovcs,
> }
> }
>
> + ovcs->notify_state = action;
> +
> return 0;
> }
>
> @@ -831,6 +833,7 @@ static int init_overlay_changeset(struct overlay_changeset *ovcs)
> }
>
> ovcs->count = cnt;
> + ovcs->notify_state = OF_OVERLAY_INIT;
>
> return 0;
>
> @@ -866,15 +869,14 @@ static void free_overlay_changeset(struct overlay_changeset *ovcs)
> * allowed to retain pointers into the overlay devicetree other
> * than during the window between OF_OVERLAY_PRE_APPLY overlay
> * notifiers and the OF_OVERLAY_POST_REMOVE overlay notifiers.
> - * During the window, ovcs->kfree_unsafe will be true.
> *
> * A memory leak will occur here if ovcs->kfree_unsafe is true.
> */
>
> - if (!ovcs->kfree_unsafe)
> + if (ovcs->notify_state == OF_OVERLAY_INIT || ovcs->notify_state == OF_OVERLAY_POST_REMOVE) {
> kfree(ovcs->overlay_mem);
> - if (!ovcs->kfree_unsafe)
> kfree(ovcs->new_fdt);
> + }
> kfree(ovcs);
> }
>
> @@ -926,12 +928,6 @@ static int of_overlay_apply(struct overlay_changeset *ovcs)
> if (ret)
> goto out;
>
> - /*
> - * After overlay_notify(), ovcs->overlay_root 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.
> - */
> - ovcs->kfree_unsafe = true;
> ret = overlay_notify(ovcs, OF_OVERLAY_PRE_APPLY);
> if (ret) {
> pr_err("overlay changeset pre-apply notify error %d\n", ret);
> diff --git a/include/linux/of.h b/include/linux/of.h
> index 04971e85fbc9..b7b095593eec 100644
> --- a/include/linux/of.h
> +++ b/include/linux/of.h
> @@ -1543,6 +1543,7 @@ static inline bool of_device_is_system_power_controller(const struct device_node
> */
>
> enum of_overlay_notify_action {
> + OF_OVERLAY_INIT = -1,
> OF_OVERLAY_PRE_APPLY = 0,
> OF_OVERLAY_POST_APPLY,
> OF_OVERLAY_PRE_REMOVE,
Powered by blists - more mailing lists