* [PATCH] of: overlay: set 'overlay_tree' and 'fdt' fields only on success
@ 2022-03-31 9:56 Slawomir Stepien
2022-03-31 10:06 ` Alexander Sverdlin
2022-04-04 20:38 ` Rob Herring
0 siblings, 2 replies; 7+ messages in thread
From: Slawomir Stepien @ 2022-03-31 9:56 UTC (permalink / raw)
To: pantelis.antoniou, frowand.list, robh+dt, devicetree
Cc: krzysztof.adamski, tomasz.medrek, alexander.sverdlin
From: Slawomir Stepien <slawomir.stepien@nokia.com>
Before this change, the memory pointed by fields 'overlay_tree' and
'fdt' will be double freed by a call to free_overlay_changeset() from
of_overlay_apply(), when the init_overlay_changeset() fails.
The first free will happen under 'err_free_tree' label and for the
second time under 'err_free_overlay_changeset' label, where we call
free_overlay_changeset().
This could happen for example, when you are applying an overlay to a
target path that does not exists.
By setting the pointers only when we are sure that
init_overlay_changeset() will not fail, will prevent this double free.
Signed-off-by: Slawomir Stepien <slawomir.stepien@nokia.com>
---
drivers/of/overlay.c | 5 ++---
1 file changed, 2 insertions(+), 3 deletions(-)
diff --git a/drivers/of/overlay.c b/drivers/of/overlay.c
index d80160cf34bb..a72a9a415f8f 100644
--- a/drivers/of/overlay.c
+++ b/drivers/of/overlay.c
@@ -750,9 +750,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);
@@ -829,6 +826,8 @@ static int init_overlay_changeset(struct overlay_changeset *ovcs,
goto err_free_fragments;
}
+ ovcs->overlay_tree = tree;
+ ovcs->fdt = fdt;
ovcs->id = id;
ovcs->count = cnt;
ovcs->fragments = fragments;
--
Slawomir Stepien
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH] of: overlay: set 'overlay_tree' and 'fdt' fields only on success
2022-03-31 9:56 [PATCH] of: overlay: set 'overlay_tree' and 'fdt' fields only on success Slawomir Stepien
@ 2022-03-31 10:06 ` Alexander Sverdlin
2022-04-04 20:38 ` Rob Herring
1 sibling, 0 replies; 7+ messages in thread
From: Alexander Sverdlin @ 2022-03-31 10:06 UTC (permalink / raw)
To: Slawomir Stepien, pantelis.antoniou, frowand.list, robh+dt, devicetree
Cc: krzysztof.adamski, tomasz.medrek
Hi Slawomir,
Thank you for the patch!
On 31/03/2022 11:56, Slawomir Stepien wrote:
> From: Slawomir Stepien <slawomir.stepien@nokia.com>
>
> Before this change, the memory pointed by fields 'overlay_tree' and
> 'fdt' will be double freed by a call to free_overlay_changeset() from
> of_overlay_apply(), when the init_overlay_changeset() fails.
>
> The first free will happen under 'err_free_tree' label and for the
> second time under 'err_free_overlay_changeset' label, where we call
> free_overlay_changeset().
>
> This could happen for example, when you are applying an overlay to a
> target path that does not exists.
>
> By setting the pointers only when we are sure that
> init_overlay_changeset() will not fail, will prevent this double free.
>
> Signed-off-by: Slawomir Stepien <slawomir.stepien@nokia.com>
Reviewed-by: Alexander Sverdlin <alexander.sverdlin@nokia.com>
> ---
> drivers/of/overlay.c | 5 ++---
> 1 file changed, 2 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/of/overlay.c b/drivers/of/overlay.c
> index d80160cf34bb..a72a9a415f8f 100644
> --- a/drivers/of/overlay.c
> +++ b/drivers/of/overlay.c
> @@ -750,9 +750,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);
> @@ -829,6 +826,8 @@ static int init_overlay_changeset(struct overlay_changeset *ovcs,
> goto err_free_fragments;
> }
>
> + ovcs->overlay_tree = tree;
> + ovcs->fdt = fdt;
> ovcs->id = id;
> ovcs->count = cnt;
> ovcs->fragments = fragments;
--
Best regards,
Alexander Sverdlin.
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] of: overlay: set 'overlay_tree' and 'fdt' fields only on success
2022-03-31 9:56 [PATCH] of: overlay: set 'overlay_tree' and 'fdt' fields only on success Slawomir Stepien
2022-03-31 10:06 ` Alexander Sverdlin
@ 2022-04-04 20:38 ` Rob Herring
2022-04-04 21:02 ` Frank Rowand
2022-04-10 20:30 ` Frank Rowand
1 sibling, 2 replies; 7+ messages in thread
From: Rob Herring @ 2022-04-04 20:38 UTC (permalink / raw)
To: Slawomir Stepien
Cc: pantelis.antoniou, frowand.list, devicetree, krzysztof.adamski,
tomasz.medrek, alexander.sverdlin
On Thu, Mar 31, 2022 at 11:56:33AM +0200, Slawomir Stepien wrote:
> From: Slawomir Stepien <slawomir.stepien@nokia.com>
>
> Before this change, the memory pointed by fields 'overlay_tree' and
> 'fdt' will be double freed by a call to free_overlay_changeset() from
> of_overlay_apply(), when the init_overlay_changeset() fails.
>
> The first free will happen under 'err_free_tree' label and for the
> second time under 'err_free_overlay_changeset' label, where we call
> free_overlay_changeset().
>
> This could happen for example, when you are applying an overlay to a
> target path that does not exists.
>
> By setting the pointers only when we are sure that
> init_overlay_changeset() will not fail, will prevent this double free.
It looks to me like the free should just be moved from
init_overlay_changeset() to of_overlay_fdt_apply() where the allocation
is done. Frank?
Also, I believe there's a bug that of_overlay_apply() should be passed
new_fdt_align rather than new_fdt. It's only a bug if we expect
overlay_changeset.fdt to point to a valid fdt rather than the memory
allocation.
Rob
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] of: overlay: set 'overlay_tree' and 'fdt' fields only on success
2022-04-04 20:38 ` Rob Herring
@ 2022-04-04 21:02 ` Frank Rowand
2022-04-07 19:53 ` Frank Rowand
2022-04-10 20:30 ` Frank Rowand
1 sibling, 1 reply; 7+ messages in thread
From: Frank Rowand @ 2022-04-04 21:02 UTC (permalink / raw)
To: Rob Herring, Slawomir Stepien
Cc: pantelis.antoniou, devicetree, krzysztof.adamski, tomasz.medrek,
alexander.sverdlin
On 4/4/22 15:38, Rob Herring wrote:
> On Thu, Mar 31, 2022 at 11:56:33AM +0200, Slawomir Stepien wrote:
>> From: Slawomir Stepien <slawomir.stepien@nokia.com>
>>
>> Before this change, the memory pointed by fields 'overlay_tree' and
>> 'fdt' will be double freed by a call to free_overlay_changeset() from
>> of_overlay_apply(), when the init_overlay_changeset() fails.
>>
>> The first free will happen under 'err_free_tree' label and for the
>> second time under 'err_free_overlay_changeset' label, where we call
>> free_overlay_changeset().
>>
>> This could happen for example, when you are applying an overlay to a
>> target path that does not exists.
>>
>> By setting the pointers only when we are sure that
>> init_overlay_changeset() will not fail, will prevent this double free.
>
> It looks to me like the free should just be moved from
> init_overlay_changeset() to of_overlay_fdt_apply() where the allocation
> is done. Frank?
This patch is next on my list to look over.
-Frank
>
> Also, I believe there's a bug that of_overlay_apply() should be passed
> new_fdt_align rather than new_fdt. It's only a bug if we expect
> overlay_changeset.fdt to point to a valid fdt rather than the memory
> allocation.
>
> Rob
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] of: overlay: set 'overlay_tree' and 'fdt' fields only on success
2022-04-04 21:02 ` Frank Rowand
@ 2022-04-07 19:53 ` Frank Rowand
2022-04-08 6:05 ` Slawomir Stepien
0 siblings, 1 reply; 7+ messages in thread
From: Frank Rowand @ 2022-04-07 19:53 UTC (permalink / raw)
To: Rob Herring, Slawomir Stepien
Cc: pantelis.antoniou, devicetree, krzysztof.adamski, tomasz.medrek,
alexander.sverdlin
Hi Slawomir,
On 4/4/22 16:02, Frank Rowand wrote:
> On 4/4/22 15:38, Rob Herring wrote:
>> On Thu, Mar 31, 2022 at 11:56:33AM +0200, Slawomir Stepien wrote:
>>> From: Slawomir Stepien <slawomir.stepien@nokia.com>
>>>
>>> Before this change, the memory pointed by fields 'overlay_tree' and
>>> 'fdt' will be double freed by a call to free_overlay_changeset() from
>>> of_overlay_apply(), when the init_overlay_changeset() fails.
>>>
>>> The first free will happen under 'err_free_tree' label and for the
>>> second time under 'err_free_overlay_changeset' label, where we call
>>> free_overlay_changeset().
>>>
>>> This could happen for example, when you are applying an overlay to a
>>> target path that does not exists.
>>>
>>> By setting the pointers only when we are sure that
>>> init_overlay_changeset() will not fail, will prevent this double free.
>>
>> It looks to me like the free should just be moved from
>> init_overlay_changeset() to of_overlay_fdt_apply() where the allocation
>> is done. Frank?
>
> This patch is next on my list to look over.
Thanks for finding this problem. While investigating what you reported
I found that there are additional related issues. I am in the process
of testing a patch to fix all of the issues.
-Frank
>
> -Frank
>
>>
>> Also, I believe there's a bug that of_overlay_apply() should be passed
>> new_fdt_align rather than new_fdt. It's only a bug if we expect
>> overlay_changeset.fdt to point to a valid fdt rather than the memory
>> allocation.
>>
>> Rob
>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] of: overlay: set 'overlay_tree' and 'fdt' fields only on success
2022-04-07 19:53 ` Frank Rowand
@ 2022-04-08 6:05 ` Slawomir Stepien
0 siblings, 0 replies; 7+ messages in thread
From: Slawomir Stepien @ 2022-04-08 6:05 UTC (permalink / raw)
To: Frank Rowand
Cc: Rob Herring, pantelis.antoniou, devicetree, krzysztof.adamski,
tomasz.medrek, alexander.sverdlin
On kwi 07, 2022 14:53, Frank Rowand wrote:
> Hi Slawomir,
Hello
> On 4/4/22 16:02, Frank Rowand wrote:
> > On 4/4/22 15:38, Rob Herring wrote:
> >> On Thu, Mar 31, 2022 at 11:56:33AM +0200, Slawomir Stepien wrote:
> >>> From: Slawomir Stepien <slawomir.stepien@nokia.com>
> >>>
> >>> Before this change, the memory pointed by fields 'overlay_tree' and
> >>> 'fdt' will be double freed by a call to free_overlay_changeset() from
> >>> of_overlay_apply(), when the init_overlay_changeset() fails.
> >>>
> >>> The first free will happen under 'err_free_tree' label and for the
> >>> second time under 'err_free_overlay_changeset' label, where we call
> >>> free_overlay_changeset().
> >>>
> >>> This could happen for example, when you are applying an overlay to a
> >>> target path that does not exists.
> >>>
> >>> By setting the pointers only when we are sure that
> >>> init_overlay_changeset() will not fail, will prevent this double free.
> >>
> >> It looks to me like the free should just be moved from
> >> init_overlay_changeset() to of_overlay_fdt_apply() where the allocation
> >> is done. Frank?
> >
> > This patch is next on my list to look over.
>
> Thanks for finding this problem. While investigating what you reported
> I found that there are additional related issues. I am in the process
> of testing a patch to fix all of the issues.
That sounds great! Thank you!
> -Frank
>
> >
> > -Frank
> >
> >>
> >> Also, I believe there's a bug that of_overlay_apply() should be passed
> >> new_fdt_align rather than new_fdt. It's only a bug if we expect
> >> overlay_changeset.fdt to point to a valid fdt rather than the memory
> >> allocation.
> >>
> >> Rob
> >
>
--
Slawomir Stepien
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] of: overlay: set 'overlay_tree' and 'fdt' fields only on success
2022-04-04 20:38 ` Rob Herring
2022-04-04 21:02 ` Frank Rowand
@ 2022-04-10 20:30 ` Frank Rowand
1 sibling, 0 replies; 7+ messages in thread
From: Frank Rowand @ 2022-04-10 20:30 UTC (permalink / raw)
To: Rob Herring, Slawomir Stepien
Cc: pantelis.antoniou, devicetree, krzysztof.adamski, tomasz.medrek,
alexander.sverdlin
On 4/4/22 15:38, Rob Herring wrote:
> On Thu, Mar 31, 2022 at 11:56:33AM +0200, Slawomir Stepien wrote:
>> From: Slawomir Stepien <slawomir.stepien@nokia.com>
>>
>> Before this change, the memory pointed by fields 'overlay_tree' and
>> 'fdt' will be double freed by a call to free_overlay_changeset() from
>> of_overlay_apply(), when the init_overlay_changeset() fails.
>>
>> The first free will happen under 'err_free_tree' label and for the
>> second time under 'err_free_overlay_changeset' label, where we call
>> free_overlay_changeset().
>>
>> This could happen for example, when you are applying an overlay to a
>> target path that does not exists.
>>
>> By setting the pointers only when we are sure that
>> init_overlay_changeset() will not fail, will prevent this double free.
>
> It looks to me like the free should just be moved from
> init_overlay_changeset() to of_overlay_fdt_apply() where the allocation
> is done. Frank?
Yes, that would be much cleaner and proper. v2 of my follow on patch set
will do so.
>
> Also, I believe there's a bug that of_overlay_apply() should be passed
> new_fdt_align rather than new_fdt. It's only a bug if we expect
> overlay_changeset.fdt to point to a valid fdt rather than the memory
> allocation.
new_fdt is correct instead of new_fdt_align. It is only used by
of_overlay_apply() and children for the purpose of kfree(). The
only place that new_fdt_align is derefenced to access the data
in the FDT is in of_fdt_unflatten_tree() and children, which is
called by of_overlay_fdt_apply.
>
> Rob
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2022-04-10 20:30 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-03-31 9:56 [PATCH] of: overlay: set 'overlay_tree' and 'fdt' fields only on success Slawomir Stepien
2022-03-31 10:06 ` Alexander Sverdlin
2022-04-04 20:38 ` Rob Herring
2022-04-04 21:02 ` Frank Rowand
2022-04-07 19:53 ` Frank Rowand
2022-04-08 6:05 ` Slawomir Stepien
2022-04-10 20:30 ` Frank Rowand
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.