Re: [PATCH] of/fdt: Improve error checking

From: Guenter Roeck <linux@roeck-us.net>
To: Rob Herring <robh+dt@kernel.org>
Cc: Frank Rowand <frowand.list@gmail.com>,
	Pantelis Antoniou <pantelis.antoniou@konsulko.com>,
	devicetree@vger.kernel.org,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH] of/fdt: Improve error checking
Date: Mon, 29 Mar 2021 12:33:12 -0700	[thread overview]
Message-ID: <9d720238-fd72-f38e-be3c-ec9f66d95ebb@roeck-us.net> (raw)
In-Reply-To: <CAL_JsqK6r=hfXdSZhsWb72O0QMDaOPnWTiYnOjp1fvztRj865A@mail.gmail.com>

On 3/29/21 11:21 AM, Rob Herring wrote:
> On Sat, Mar 27, 2021 at 5:41 PM Guenter Roeck <linux@roeck-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?
> 
No. I checked. Quite surprisingly, it is 4. sizeof(unsigned long long)
is 8, though.

> 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.
> 

Plus, as Frank points out, there is a copy in overlays, and I wasn't
sure if the other uses of kmalloc()/kmemdup() in devicetree code are safe.
That is why I didn't try to fix that.

>> 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.

Thanks,
Guenter