Message-ID: <b3000300-0efe-64b9-8bba-716045b17090@gmail.com>
Date:   Mon, 29 Mar 2021 14:12:22 -0500
From:   Frank Rowand <frowand.list@...il.com>
To:     Rob Herring <robh+dt@...nel.org>,
        Guenter Roeck <linux@...ck-us.net>
Cc:     Pantelis Antoniou <pantelis.antoniou@...sulko.com>,
        devicetree@...r.kernel.org,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] of/fdt: Improve error checking

On 3/29/21 1:21 PM, Rob Herring wrote:
> On Sat, Mar 27, 2021 at 5:41 PM Guenter Roeck <linux@...ck-us.net> wrote:
>>
>> If an unaligned pointer is passed to of_fdt_unflatten_tree(),
>> populate_node() as called from unflatten_dt_nodes() will fail.
>> unflatten_dt_nodes() will return 0 and set *nodepp to NULL.
>> This is not expected to happen in __unflatten_device_tree(),
>> which then tries to write into the NULL pointer, causing a crash
>> on openrisc if CONFIG_OF_UNITTEST=y.
>>
>>  ### dt-test ### start of unittest - you will see error messages
>> Unable to handle kernel NULL pointer dereference
>>  at virtual address 0x00000064
>>
>> Oops#: 0000
>> CPU #: 0
>>    PC: c03a25d4    SR: 0000807f    SP: c102dd50
>> GPR00: 00000000 GPR01: c102dd50 GPR02: c102dd78 GPR03: c1704004
>> GPR04: 00000000 GPR05: c102dc18 GPR06: c102ddc8 GPR07: c102dcf7
>> GPR08: 00000001 GPR09: c03a25a0 GPR10: c102c000 GPR11: c16fd75c
>> GPR12: 0000ffb7 GPR13: 00000000 GPR14: 00000008 GPR15: 00000000
>> GPR16: c16fd75c GPR17: 00000064 GPR18: c1704004 GPR19: 00000004
>> GPR20: 00000000 GPR21: 00000000 GPR22: c102ddc8 GPR23: 00000018
>> GPR24: 00000001 GPR25: 00000010 GPR26: c16fd75c GPR27: 00000008
>> GPR28: deadbeef GPR29: 00000000 GPR30: c0720128 GPR31: 00060000
>>   RES: c16fd75c oGPR11: ffffffff
>> Process swapper (pid: 1, stackpage=c1028ba0)
>>
>> Stack:
>> Call trace:
>> [<(ptrval)>] __unflatten_device_tree+0xe0/0x184
>> [<(ptrval)>] of_fdt_unflatten_tree+0x60/0x90
>> [<(ptrval)>] of_unittest+0xb4/0x3614
>> [<(ptrval)>] ? __kernfs_create_file+0x130/0x188
>> [<(ptrval)>] ? sysfs_add_file_mode_ns+0x13c/0x288
>> [<(ptrval)>] ? of_unittest+0x0/0x3614
>> [<(ptrval)>] ? lock_is_held_type+0x160/0x20c
>> [<(ptrval)>] ? of_unittest+0x0/0x3614
>> [<(ptrval)>] ? ignore_unknown_bootoption+0x0/0x24
>> [<(ptrval)>] do_one_initcall+0x98/0x340
>> [<(ptrval)>] ? parse_args+0x220/0x4e4
>> [<(ptrval)>] ? lock_is_held_type+0x160/0x20c
>> [<(ptrval)>] ? ignore_unknown_bootoption+0x0/0x24
>> [<(ptrval)>] ? rcu_read_lock_sched_held+0x34/0x88
>> [<(ptrval)>] kernel_init_freeable+0x1c0/0x240
>> [<(ptrval)>] ? ignore_unknown_bootoption+0x0/0x24
>> [<(ptrval)>] ? kernel_init+0x0/0x154
>> [<(ptrval)>] kernel_init+0x1c/0x154
>> [<(ptrval)>] ? calculate_sigpending+0x54/0x64
>> [<(ptrval)>] ret_from_fork+0x1c/0x150
>>
>> This problem affects all architectures with a 4-byte memory alignment.
>> Since commit 79edff12060f ("scripts/dtc: Update to upstream version
>> v1.6.0-51-g183df9e9c2b9"), devicetree code in the Linux kernel mandates
>> an 8-byte memory alignment of devicetree pointers, but it does not take
>> into account that functions such as kmalloc() or kmemdup() may not return
>> accordingly aligned pointers.
> 
> AFAICT, openrisc would get:
> 
> #define ARCH_KMALLOC_MINALIGN __alignof__(unsigned long long)
> 
> Is that not 8 bytes?
> 
> Specifically, the problem is here is the unittest DT is copied with
> kmemdup(). I don't think there are other allocations which could be a
> problem.
> 
>> To fix the immediate crash, check if *mynodes is NULL in
>> __unflatten_device_tree() before writing into it.
>>
>> Also affected by this problem is the code calling of_fdt_unflatten_tree().
>> That code checks for errors using the content of the mynodes pointer,
>> which is not set by the devicetree code if the alignment problem is
>> observed. Result is that the callers of of_fdt_unflatten_tree() check
>> if an uninitialized pointer is set to NULL. Preinitialize that pointer
>> to avoid the problem.
>>
>> With this code in place, devicetree code on openrisc (and any other
> 
> "devicetree unittest code"
> 
> The only other dtb copy is unflatten_and_copy_device_tree() which
> should be fine since it gives memblock the alignment requirement.

Plus overlays does at least one other copy.

I'll create a patch this week to properly align copies and to add some
of the checks suggested below based on the principal that a test should
not crash the kernel.

-Frank

> 
>> architecture with 4-byte memory alignment) will still not work properly,
>> but at least it won't crash anymore. The resulting log message is
>>
>>  ### dt-test ### start of unittest - you will see error messages
>>  ### dt-test ### unittest_data_add: No tree to attach; not running tests
>>
>> when trying to run devicetree unittests.
>>
>> Fixes: 79edff12060f ("scripts/dtc: Update to upstream version v1.6.0-51-g183df9e9c2b9")
>> Signed-off-by: Guenter Roeck <linux@...ck-us.net>
>> ---
>>  drivers/of/fdt.c      | 2 +-
>>  drivers/of/overlay.c  | 2 +-
>>  drivers/of/unittest.c | 2 +-
>>  3 files changed, 3 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/of/fdt.c b/drivers/of/fdt.c
>> index dcc1dd96911a..ab95fdb4461d 100644
>> --- a/drivers/of/fdt.c
>> +++ b/drivers/of/fdt.c
>> @@ -415,7 +415,7 @@ void *__unflatten_device_tree(const void *blob,
>>                 pr_warn("End of tree marker overwritten: %08x\n",
>>                         be32_to_cpup(mem + size));
>>
>> -       if (detached && mynodes) {
>> +       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..5dc4d2378bfd 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
>> --
>> 2.17.1
>>

Powered by blists - more mailing lists