Re: [PATCH 1/1] of: unittest: overlay: ensure proper alignment of copied FDT

[Frank Rowand]

On 4/7/21 4:34 PM, Rob Herring wrote:
> On Wed, Apr 7, 2021 at 3:51 PM <frowand.list@xxxxxxxxx> wrote:
>>
>> From: Frank Rowand <frank.rowand@xxxxxxxx>
>>
>> The Devicetree standard specifies an 8 byte alignment of the FDT.
>> Code in libfdt expects this alignment for an FDT image in memory.
>> kmemdup() returns 4 byte alignment on openrisc.  Replace kmemdup()
>> with kmalloc(), align pointer, memcpy() to get proper alignment.
>>
>> The 4 byte alignment exposed a related bug which triggered 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@xxxxxxxxxxxx/
>>
>> Reported-by: Guenter Roeck <linux@xxxxxxxxxxxx>
>> Signed-off-by: Frank Rowand <frank.rowand@xxxxxxxx>
>> ---
>>  drivers/of/of_private.h | 2 ++
>>  drivers/of/overlay.c    | 8 ++++++--
>>  drivers/of/unittest.c   | 9 +++++++--
>>  3 files changed, 15 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/of/of_private.h b/drivers/of/of_private.h
>> index d9e6a324de0a..d717efbd637d 100644
>> --- a/drivers/of/of_private.h
>> +++ b/drivers/of/of_private.h
>> @@ -8,6 +8,8 @@
>>   * Copyright (C) 1996-2005 Paul Mackerras.
>>   */
>>
>> +#define FDT_ALIGN_SIZE 8
>> +
>>  /**
>>   * struct alias_prop - Alias property in 'aliases' node
>>   * @link:      List node to link the structure in aliases_lookup list
>> diff --git a/drivers/of/overlay.c b/drivers/of/overlay.c
>> index 50bbe0edf538..8b40711ed202 100644
>> --- a/drivers/of/overlay.c
>> +++ b/drivers/of/overlay.c
>> @@ -1014,7 +1014,7 @@ static int of_overlay_apply(const void *fdt, struct device_node *tree,
>>  int of_overlay_fdt_apply(const void *overlay_fdt, u32 overlay_fdt_size,
>>                          int *ovcs_id)
>>  {
>> -       const void *new_fdt;
>> +       void *new_fdt;
>>         int ret;
>>         u32 size;
>>         struct device_node *overlay_root;
>> @@ -1036,10 +1036,14 @@ int of_overlay_fdt_apply(const void *overlay_fdt, u32 overlay_fdt_size,
>>          * Must create permanent copy of FDT because of_fdt_unflatten_tree()
>>          * will create pointers to the passed in FDT in the unflattened tree.
>>          */
>> -       new_fdt = kmemdup(overlay_fdt, size, GFP_KERNEL);
>> +       size += FDT_ALIGN_SIZE;
>> +       new_fdt = kmalloc(size, GFP_KERNEL);
>>         if (!new_fdt)
>>                 return -ENOMEM;
>>
>> +       new_fdt = PTR_ALIGN(new_fdt, FDT_ALIGN_SIZE);
>> +       memcpy(new_fdt, overlay_fdt, size);
>> +
>>         of_fdt_unflatten_tree(new_fdt, NULL, &overlay_root);
>>         if (!overlay_root) {
>>                 pr_err("unable to unflatten overlay_fdt\n");
>> diff --git a/drivers/of/unittest.c b/drivers/of/unittest.c
>> index eb100627c186..edd6ce807691 100644
>> --- a/drivers/of/unittest.c
>> +++ b/drivers/of/unittest.c
>> @@ -22,6 +22,7 @@
>>  #include <linux/slab.h>
>>  #include <linux/device.h>
>>  #include <linux/platform_device.h>
>> +#include <linux/kernel.h>
>>
>>  #include <linux/i2c.h>
>>  #include <linux/i2c-mux.h>
>> @@ -1415,7 +1416,7 @@ static int __init unittest_data_add(void)
>>          */
>>         extern uint8_t __dtb_testcases_begin[];
>>         extern uint8_t __dtb_testcases_end[];
>> -       const int size = __dtb_testcases_end - __dtb_testcases_begin;
>> +       u32 size = __dtb_testcases_end - __dtb_testcases_begin;
>>         int rc;
>>
>>         if (!size) {
>> @@ -1425,10 +1426,14 @@ static int __init unittest_data_add(void)
>>         }
>>
>>         /* creating copy */
>> -       unittest_data = kmemdup(__dtb_testcases_begin, size, GFP_KERNEL);
>> +       size += FDT_ALIGN_SIZE;
>> +       unittest_data = kmalloc(size, GFP_KERNEL);
>>         if (!unittest_data)
>>                 return -ENOMEM;
>>
>> +       unittest_data = PTR_ALIGN(unittest_data, FDT_ALIGN_SIZE);
>> +       memcpy(unittest_data, __dtb_testcases_begin, size);
>> +
>>         of_fdt_unflatten_tree(unittest_data, NULL, &unittest_data_node);
>>         if (!unittest_data_node) {
>>                 pr_warn("%s: No tree to attach; not running tests\n", __func__);
> 
> The next line here is a kfree(unittest_data) which I assume will fail
> if the ptr address changed. Same issue in the overlay code.

Thanks for catching this.

> 
> The error path is easy to fix. Freeing the memory later on, not so
> much... 

The overlay subsystem retains ownership of the allocated memory and
responsibility for any subsequent kfree(), so actually not very
difficult.

New version of the patch should be out this morning.

-Frank

> One solution is always alloc a power of 2 size, that's
> guaranteed to be 'size' aligned:
> 
>  * The allocated object address is aligned to at least ARCH_KMALLOC_MINALIGN
>  * bytes. For @size of power of two bytes, the alignment is also guaranteed
>  * to be at least to the size.
> 
> Rob
> .
>