On Sun, Apr 04, 2021 at 10:37:35PM -0500, Frank Rowand wrote: > 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. > Trying again: With this patch applied, the kernel no longer crashes, and the log message is as expected: ### dt-test ### start of unittest - you will see error messages ### dt-test ### unittest_data_add: unflatten testcases tree failed Tested-by: Guenter Roeck <linux@xxxxxxxxxxxx> > 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; > > } > > >
