Re: [PATCH] of/fdt: Improve error checking
On 3/29/21 11:21 AM, Rob Herring wrote:
> On Sat, Mar 27, 2021 at 5:41 PM Guenter Roeck 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 0x0064
>>
>> Oops#:
>> CPU #: 0
>>PC: c03a25d4SR: 807fSP: c102dd50
>> GPR00: GPR01: c102dd50 GPR02: c102dd78 GPR03: c1704004
>> GPR04: GPR05: c102dc18 GPR06: c102ddc8 GPR07: c102dcf7
>> GPR08: 0001 GPR09: c03a25a0 GPR10: c102c000 GPR11: c16fd75c
>> GPR12: ffb7 GPR13: GPR14: 0008 GPR15:
>> GPR16: c16fd75c GPR17: 0064 GPR18: c1704004 GPR19: 0004
>> GPR20: GPR21: GPR22: c102ddc8 GPR23: 0018
>> GPR24: 0001 GPR25: 0010 GPR26: c16fd75c GPR27: 0008
>> GPR28: deadbeef GPR29: GPR30: c0720128 GPR31: 0006
>> RES: c16fd75c oGPR11:
>> 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
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 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 0x0064
>>
>> Oops#:
>> CPU #: 0
>>PC: c03a25d4SR: 807fSP: c102dd50
>> GPR00: GPR01: c102dd50 GPR02: c102dd78 GPR03: c1704004
>> GPR04: GPR05: c102dc18 GPR06: c102ddc8 GPR07: c102dcf7
>> GPR08: 0001 GPR09: c03a25a0 GPR10: c102c000 GPR11: c16fd75c
>> GPR12: ffb7 GPR13: GPR14: 0008 GPR15:
>> GPR16: c16fd75c GPR17: 0064 GPR18: c1704004 GPR19: 0004
>> GPR20: GPR21: GPR22: c102ddc8 GPR23: 0018
>> GPR24: 0001 GPR25: 0010 GPR26: c16fd75c GPR27: 0008
>> GPR28: deadbeef GPR29: GPR30: c0720128 GPR31: 0006
>> RES: c16fd75c oGPR11:
>> 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
>> ---
>> 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 && my
Re: [PATCH] of/fdt: Improve error checking
On Sat, Mar 27, 2021 at 5:41 PM Guenter Roeck 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 0x0064
>
> Oops#:
> CPU #: 0
>PC: c03a25d4SR: 807fSP: c102dd50
> GPR00: GPR01: c102dd50 GPR02: c102dd78 GPR03: c1704004
> GPR04: GPR05: c102dc18 GPR06: c102ddc8 GPR07: c102dcf7
> GPR08: 0001 GPR09: c03a25a0 GPR10: c102c000 GPR11: c16fd75c
> GPR12: ffb7 GPR13: GPR14: 0008 GPR15:
> GPR16: c16fd75c GPR17: 0064 GPR18: c1704004 GPR19: 0004
> GPR20: GPR21: GPR22: c102ddc8 GPR23: 0018
> GPR24: 0001 GPR25: 0010 GPR26: c16fd75c GPR27: 0008
> GPR28: deadbeef GPR29: GPR30: c0720128 GPR31: 0006
> RES: c16fd75c oGPR11:
> 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.
> 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
> ---
> 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, u
[PATCH] of/fdt: Improve error checking
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 0x0064
Oops#:
CPU #: 0
PC: c03a25d4SR: 807fSP: c102dd50
GPR00: GPR01: c102dd50 GPR02: c102dd78 GPR03: c1704004
GPR04: GPR05: c102dc18 GPR06: c102ddc8 GPR07: c102dcf7
GPR08: 0001 GPR09: c03a25a0 GPR10: c102c000 GPR11: c16fd75c
GPR12: ffb7 GPR13: GPR14: 0008 GPR15:
GPR16: c16fd75c GPR17: 0064 GPR18: c1704004 GPR19: 0004
GPR20: GPR21: GPR22: c102ddc8 GPR23: 0018
GPR24: 0001 GPR25: 0010 GPR26: c16fd75c GPR27: 0008
GPR28: deadbeef GPR29: GPR30: c0720128 GPR31: 0006
RES: c16fd75c oGPR11:
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.
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
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
---
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;
/*
* _
