From: Frank Rowand <frowand.list@gmail.com>
To: Rob Herring <robh+dt@kernel.org>, Guenter Roeck <linux@roeck-us.net>
Cc: Pantelis Antoniou <pantelis.antoniou@konsulko.com>,
devicetree@vger.kernel.org,
Geert Uytterhoeven <geert+renesas@glider.be>,
linux-kernel@vger.kernel.org
Subject: Re: [PATCH 1/1] of: properly check for error returned by fdt_get_name()
Date: Sun, 4 Apr 2021 22:37:35 -0500 [thread overview]
Message-ID: <8b16b504-83b5-9d36-eade-f7b375c0535b@gmail.com> (raw)
In-Reply-To: <20210405032845.1942533-1-frowand.list@gmail.com>
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.
Thanks,
Frank
On 4/4/21 10:28 PM, frowand.list@gmail.com wrote:
> From: Frank Rowand <frank.rowand@sony.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@roeck-us.net>
> Signed-off-by: Frank Rowand <frank.rowand@sony.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;
> }
>
next prev parent reply other threads:[~2021-04-05 3:37 UTC|newest]
Thread overview: 8+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-04-05 3:28 [PATCH 1/1] of: properly check for error returned by fdt_get_name() frowand.list
2021-04-05 3:37 ` Frank Rowand [this message]
2021-04-05 23:03 ` Guenter Roeck
2021-04-06 19:21 ` Rob Herring
2021-04-06 20:30 ` Frank Rowand
2021-04-07 21:04 ` Frank Rowand
2021-04-06 19:21 ` Rob Herring
-- strict thread matches above, loose matches on Subject: below --
2021-04-05 17:02 Guenter Roeck
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=8b16b504-83b5-9d36-eade-f7b375c0535b@gmail.com \
--to=frowand.list@gmail.com \
--cc=devicetree@vger.kernel.org \
--cc=geert+renesas@glider.be \
--cc=linux-kernel@vger.kernel.org \
--cc=linux@roeck-us.net \
--cc=pantelis.antoniou@konsulko.com \
--cc=robh+dt@kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).