| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <20210405170220.GA52569@roeck-us.net>
Date: Mon, 5 Apr 2021 10:02:20 -0700
From: Guenter Roeck <linux@...ck-us.net>
To: 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:28:45PM -0500, 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>
>
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,
Guenter
> ---
>
> 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;
> }
> --
> Frank Rowand <frank.rowand@...y.com>
>
Powered by blists - more mailing lists