[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20210405230322.GA249231@roeck-us.net>
Date: Mon, 5 Apr 2021 16:03:22 -0700
From: Guenter Roeck <linux@...ck-us.net>
To: Frank Rowand <frowand.list@...il.com>
Cc: Rob Herring <robh+dt@...nel.org>,
Pantelis Antoniou <pantelis.antoniou@...sulko.com>,
devicetree@...r.kernel.org,
Geert Uytterhoeven <geert+renesas@...der.be>,
linux-kernel@...r.kernel.org
Subject: Re: [PATCH 1/1] of: properly check for error returned by
fdt_get_name()
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@roeck-us.net/
>
> 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@...ck-us.net>
> Thanks,
>
> Frank
>
>
> On 4/4/21 10:28 PM, frowand.list@...il.com wrote:
> > From: Frank Rowand <frank.rowand@...y.com>
> >
> > 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@roeck-us.net/
> >
> > Fixes: commit 79edff12060f ("scripts/dtc: Update to upstream version v1.6.0-51-g183df9e9c2b9")
> > Reported-by: Guenter Roeck <linux@...ck-us.net>
> > Signed-off-by: Frank Rowand <frank.rowand@...y.com>
> >
> > ---
> >
> > 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@roeck-us.net/
> >
> > 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;
> > }
> >
>
Powered by blists - more mailing lists