Message ID | YkV60TQ+d3sltkNU@t480s.localdomain |
---|---|
State | Changes Requested, archived |
Headers | show |
Series | of: overlay: set 'overlay_tree' and 'fdt' fields only on success | expand |
Context | Check | Description |
---|---|---|
robh/checkpatch | success | |
robh/patch-applied | fail | build log |
robh/checkpatch | success | |
robh/patch-applied | fail | build log |
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;
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
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
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 >
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 > > >
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
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;