Hi Guenter, Can you please test this patch to see if it prevents the crash on openrisc that you reported in https://lore.kernel.org/lkml/20210327224116.69309-1-linux@xxxxxxxxxxxx/ Just after start of unittest you should see a warning about testcases. Thanks, Frank On 4/4/21 10:28 PM, frowand.list@xxxxxxxxx wrote: > From: Frank Rowand <frank.rowand@xxxxxxxx> > > fdt_get_name() returns error values via a parameter pointer > instead of in function return. Fix check for this error value > in populate_node() and callers of populate_node(). > > Chasing up the caller tree showed callers of various functions > failing to initialize the value of pointer parameters that > can return error values. Initialize those values to NULL. > > The bug was introduced by > commit e6a6928c3ea1 ("of/fdt: Convert FDT functions to use libfdt") > but this patch can not be backported directly to that commit > because the relevant code has further been restructured by > commit dfbd4c6eff35 ("drivers/of: Split unflatten_dt_node()") > > The bug became visible by triggering a crash on openrisc with: > commit 79edff12060f ("scripts/dtc: Update to upstream version v1.6.0-51-g183df9e9c2b9") > as reported in: > https://lore.kernel.org/lkml/20210327224116.69309-1-linux@xxxxxxxxxxxx/ > > Fixes: commit 79edff12060f ("scripts/dtc: Update to upstream version v1.6.0-51-g183df9e9c2b9") > Reported-by: Guenter Roeck <linux@xxxxxxxxxxxx> > Signed-off-by: Frank Rowand <frank.rowand@xxxxxxxx> > > --- > > This patch papers over the unaligned pointer passed to > of_fdt_unflatten_tree() bug that Guenter reported in > https://lore.kernel.org/lkml/20210327224116.69309-1-linux@xxxxxxxxxxxx/ > > I will create a separate patch to fix that problem. > > drivers/of/fdt.c | 36 +++++++++++++++++++++++------------- > drivers/of/overlay.c | 2 +- > drivers/of/unittest.c | 15 ++++++++++----- > 3 files changed, 34 insertions(+), 19 deletions(-) > > diff --git a/drivers/of/fdt.c b/drivers/of/fdt.c > index dcc1dd96911a..adb26aff481d 100644 > --- a/drivers/of/fdt.c > +++ b/drivers/of/fdt.c > @@ -205,7 +205,7 @@ static void populate_properties(const void *blob, > *pprev = NULL; > } > > -static bool populate_node(const void *blob, > +static int populate_node(const void *blob, > int offset, > void **mem, > struct device_node *dad, > @@ -214,24 +214,24 @@ static bool populate_node(const void *blob, > { > struct device_node *np; > const char *pathp; > - unsigned int l, allocl; > + int len; > > - pathp = fdt_get_name(blob, offset, &l); > + pathp = fdt_get_name(blob, offset, &len); > if (!pathp) { > *pnp = NULL; > - return false; > + return len; > } > > - allocl = ++l; > + len++; > > - np = unflatten_dt_alloc(mem, sizeof(struct device_node) + allocl, > + np = unflatten_dt_alloc(mem, sizeof(struct device_node) + len, > __alignof__(struct device_node)); > if (!dryrun) { > char *fn; > of_node_init(np); > np->full_name = fn = ((char *)np) + sizeof(*np); > > - memcpy(fn, pathp, l); > + memcpy(fn, pathp, len); > > if (dad != NULL) { > np->parent = dad; > @@ -295,6 +295,7 @@ static int unflatten_dt_nodes(const void *blob, > struct device_node *nps[FDT_MAX_DEPTH]; > void *base = mem; > bool dryrun = !base; > + int ret; > > if (nodepp) > *nodepp = NULL; > @@ -322,9 +323,10 @@ static int unflatten_dt_nodes(const void *blob, > !of_fdt_device_is_available(blob, offset)) > continue; > > - if (!populate_node(blob, offset, &mem, nps[depth], > - &nps[depth+1], dryrun)) > - return mem - base; > + ret = populate_node(blob, offset, &mem, nps[depth], > + &nps[depth+1], dryrun); > + if (ret < 0) > + return ret; > > if (!dryrun && nodepp && !*nodepp) > *nodepp = nps[depth+1]; > @@ -372,6 +374,10 @@ void *__unflatten_device_tree(const void *blob, > { > int size; > void *mem; > + int ret; > + > + if (mynodes) > + *mynodes = NULL; > > pr_debug(" -> unflatten_device_tree()\n"); > > @@ -392,7 +398,7 @@ void *__unflatten_device_tree(const void *blob, > > /* First pass, scan for size */ > size = unflatten_dt_nodes(blob, NULL, dad, NULL); > - if (size < 0) > + if (size <= 0) > return NULL; > > size = ALIGN(size, 4); > @@ -410,12 +416,16 @@ void *__unflatten_device_tree(const void *blob, > pr_debug(" unflattening %p...\n", mem); > > /* Second pass, do actual unflattening */ > - unflatten_dt_nodes(blob, mem, dad, mynodes); > + ret = unflatten_dt_nodes(blob, mem, dad, mynodes); > + > if (be32_to_cpup(mem + size) != 0xdeadbeef) > pr_warn("End of tree marker overwritten: %08x\n", > be32_to_cpup(mem + size)); > > - if (detached && mynodes) { > + if (ret <= 0) > + return NULL; > + > + if (detached && mynodes && *mynodes) { > of_node_set_flag(*mynodes, OF_DETACHED); > pr_debug("unflattened tree is detached\n"); > } > diff --git a/drivers/of/overlay.c b/drivers/of/overlay.c > index 50bbe0edf538..e12c643b6ba8 100644 > --- a/drivers/of/overlay.c > +++ b/drivers/of/overlay.c > @@ -1017,7 +1017,7 @@ int of_overlay_fdt_apply(const void *overlay_fdt, u32 overlay_fdt_size, > const void *new_fdt; > int ret; > u32 size; > - struct device_node *overlay_root; > + struct device_node *overlay_root = NULL; > > *ovcs_id = 0; > ret = 0; > diff --git a/drivers/of/unittest.c b/drivers/of/unittest.c > index eb100627c186..f9b5b698249f 100644 > --- a/drivers/of/unittest.c > +++ b/drivers/of/unittest.c > @@ -1408,7 +1408,7 @@ static void attach_node_and_children(struct device_node *np) > static int __init unittest_data_add(void) > { > void *unittest_data; > - struct device_node *unittest_data_node, *np; > + struct device_node *unittest_data_node = NULL, *np; > /* > * __dtb_testcases_begin[] and __dtb_testcases_end[] are magically > * created by cmd_dt_S_dtb in scripts/Makefile.lib > @@ -1417,10 +1417,10 @@ static int __init unittest_data_add(void) > extern uint8_t __dtb_testcases_end[]; > const int size = __dtb_testcases_end - __dtb_testcases_begin; > int rc; > + void *ret; > > if (!size) { > - pr_warn("%s: No testcase data to attach; not running tests\n", > - __func__); > + pr_warn("%s: testcases is empty\n", __func__); > return -ENODATA; > } > > @@ -1429,9 +1429,14 @@ static int __init unittest_data_add(void) > if (!unittest_data) > return -ENOMEM; > > - of_fdt_unflatten_tree(unittest_data, NULL, &unittest_data_node); > + ret = of_fdt_unflatten_tree(unittest_data, NULL, &unittest_data_node); > + if (!ret) { > + pr_warn("%s: unflatten testcases tree failed\n", __func__); > + kfree(unittest_data); > + return -ENODATA; > + } > if (!unittest_data_node) { > - pr_warn("%s: No tree to attach; not running tests\n", __func__); > + pr_warn("%s: testcases tree is empty\n", __func__); > kfree(unittest_data); > return -ENODATA; > } >
