From: Frank Rowand <[email protected]>
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/[email protected]/
Reported-by: Guenter Roeck <[email protected]>
Signed-off-by: Frank Rowand <[email protected]>
---
changes since version 1:
- use pointer from kmalloc() for kfree() instead of using pointer that
has been modified for FDT alignment
changes since version 2:
- version 1 was a work in progress version, I failed to commit the following
final changes
- reorder first two arguments of of_overlay_apply()
drivers/of/of_private.h | 2 ++
drivers/of/overlay.c | 28 +++++++++++++++++-----------
drivers/of/unittest.c | 12 +++++++++---
3 files changed, 28 insertions(+), 14 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..cf770452e1e5 100644
--- a/drivers/of/overlay.c
+++ b/drivers/of/overlay.c
@@ -57,7 +57,7 @@ struct fragment {
* struct overlay_changeset
* @id: changeset identifier
* @ovcs_list: list on which we are located
- * @fdt: FDT that was unflattened to create @overlay_tree
+ * @fdt: base of memory allocated to hold aligned FDT that was unflattened to create @overlay_tree
* @overlay_tree: expanded device tree that contains the fragment nodes
* @count: count of fragment structures
* @fragments: fragment nodes in the overlay expanded device tree
@@ -719,8 +719,8 @@ static struct device_node *find_target(struct device_node *info_node)
/**
* init_overlay_changeset() - initialize overlay changeset from overlay tree
* @ovcs: Overlay changeset to build
- * @fdt: the FDT that was unflattened to create @tree
- * @tree: Contains all the overlay fragments and overlay fixup nodes
+ * @fdt: base of memory allocated to hold aligned FDT that was unflattened to create @tree
+ * @tree: Contains the overlay fragments and overlay fixup nodes
*
* Initialize @ovcs. Populate @ovcs->fragments with node information from
* the top level of @tree. The relevant top level nodes are the fragment
@@ -873,7 +873,8 @@ static void free_overlay_changeset(struct overlay_changeset *ovcs)
* internal documentation
*
* of_overlay_apply() - Create and apply an overlay changeset
- * @fdt: the FDT that was unflattened to create @tree
+ * @fdt: base of memory allocated to hold *@fdt_align
+ * @fdt_align: the FDT that was unflattened to create @tree, aligned
* @tree: Expanded overlay device tree
* @ovcs_id: Pointer to overlay changeset id
*
@@ -912,8 +913,8 @@ static void free_overlay_changeset(struct overlay_changeset *ovcs)
* id is returned to *ovcs_id.
*/
-static int of_overlay_apply(const void *fdt, struct device_node *tree,
- int *ovcs_id)
+static int of_overlay_apply(const void *fdt, const void *fdt_align,
+ struct device_node *tree, int *ovcs_id)
{
struct overlay_changeset *ovcs;
int ret = 0, ret_revert, ret_tmp;
@@ -953,7 +954,7 @@ static int of_overlay_apply(const void *fdt, struct device_node *tree,
/*
* after overlay_notify(), ovcs->overlay_tree related pointers may have
* leaked to drivers, so can not kfree() tree, aka ovcs->overlay_tree;
- * and can not free fdt, aka ovcs->fdt
+ * and can not free memory containing aligned fdt, aka ovcs->fdt
*/
ret = overlay_notify(ovcs, OF_OVERLAY_PRE_APPLY);
if (ret) {
@@ -1014,7 +1015,8 @@ 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;
+ void *new_fdt_align;
int ret;
u32 size;
struct device_node *overlay_root;
@@ -1036,18 +1038,22 @@ 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;
- of_fdt_unflatten_tree(new_fdt, NULL, &overlay_root);
+ new_fdt_align = PTR_ALIGN(new_fdt, FDT_ALIGN_SIZE);
+ memcpy(new_fdt_align, overlay_fdt, size);
+
+ of_fdt_unflatten_tree(new_fdt_align, NULL, &overlay_root);
if (!overlay_root) {
pr_err("unable to unflatten overlay_fdt\n");
ret = -EINVAL;
goto out_free_new_fdt;
}
- ret = of_overlay_apply(new_fdt, overlay_root, ovcs_id);
+ ret = of_overlay_apply(new_fdt, new_fdt_align, overlay_root, ovcs_id);
if (ret < 0) {
/*
* new_fdt and overlay_root now belong to the overlay
diff --git a/drivers/of/unittest.c b/drivers/of/unittest.c
index eb100627c186..29081a8b32e6 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>
@@ -1408,6 +1409,7 @@ static void attach_node_and_children(struct device_node *np)
static int __init unittest_data_add(void)
{
void *unittest_data;
+ void *unittest_data_align;
struct device_node *unittest_data_node, *np;
/*
* __dtb_testcases_begin[] and __dtb_testcases_end[] are magically
@@ -1415,7 +1417,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,11 +1427,15 @@ 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;
- of_fdt_unflatten_tree(unittest_data, NULL, &unittest_data_node);
+ unittest_data_align = PTR_ALIGN(unittest_data, FDT_ALIGN_SIZE);
+ memcpy(unittest_data_align, __dtb_testcases_begin, size);
+
+ of_fdt_unflatten_tree(unittest_data_align, NULL, &unittest_data_node);
if (!unittest_data_node) {
pr_warn("%s: No tree to attach; not running tests\n", __func__);
kfree(unittest_data);
--
Frank Rowand <[email protected]>
On 4/8/21 8:17 AM, [email protected] wrote:
> From: Frank Rowand <[email protected]>
>
> 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/[email protected]/
>
> Reported-by: Guenter Roeck <[email protected]>
> Signed-off-by: Frank Rowand <[email protected]>
> ---
>
> changes since version 1:
> - use pointer from kmalloc() for kfree() instead of using pointer that
> has been modified for FDT alignment
>
> changes since version 2:
> - version 1 was a work in progress version, I failed to commit the following
> final changes
> - reorder first two arguments of of_overlay_apply()
>
> drivers/of/of_private.h | 2 ++
> drivers/of/overlay.c | 28 +++++++++++++++++-----------
> drivers/of/unittest.c | 12 +++++++++---
> 3 files changed, 28 insertions(+), 14 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
> +
Use existing define ? Or was that local in libfdt ?
> /**
> * 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..cf770452e1e5 100644
> --- a/drivers/of/overlay.c
> +++ b/drivers/of/overlay.c
> @@ -57,7 +57,7 @@ struct fragment {
> * struct overlay_changeset
> * @id: changeset identifier
> * @ovcs_list: list on which we are located
> - * @fdt: FDT that was unflattened to create @overlay_tree
> + * @fdt: base of memory allocated to hold aligned FDT that was unflattened to create @overlay_tree
> * @overlay_tree: expanded device tree that contains the fragment nodes
> * @count: count of fragment structures
> * @fragments: fragment nodes in the overlay expanded device tree
> @@ -719,8 +719,8 @@ static struct device_node *find_target(struct device_node *info_node)
> /**
> * init_overlay_changeset() - initialize overlay changeset from overlay tree
> * @ovcs: Overlay changeset to build
> - * @fdt: the FDT that was unflattened to create @tree
> - * @tree: Contains all the overlay fragments and overlay fixup nodes
> + * @fdt: base of memory allocated to hold aligned FDT that was unflattened to create @tree
> + * @tree: Contains the overlay fragments and overlay fixup nodes
> *
> * Initialize @ovcs. Populate @ovcs->fragments with node information from
> * the top level of @tree. The relevant top level nodes are the fragment
> @@ -873,7 +873,8 @@ static void free_overlay_changeset(struct overlay_changeset *ovcs)
> * internal documentation
> *
> * of_overlay_apply() - Create and apply an overlay changeset
> - * @fdt: the FDT that was unflattened to create @tree
> + * @fdt: base of memory allocated to hold *@fdt_align
> + * @fdt_align: the FDT that was unflattened to create @tree, aligned
> * @tree: Expanded overlay device tree
> * @ovcs_id: Pointer to overlay changeset id
> *
> @@ -912,8 +913,8 @@ static void free_overlay_changeset(struct overlay_changeset *ovcs)
> * id is returned to *ovcs_id.
> */
>
> -static int of_overlay_apply(const void *fdt, struct device_node *tree,
> - int *ovcs_id)
> +static int of_overlay_apply(const void *fdt, const void *fdt_align,
fdt_align is not used in this function.
> + struct device_node *tree, int *ovcs_id)
> {
> struct overlay_changeset *ovcs;
> int ret = 0, ret_revert, ret_tmp;
> @@ -953,7 +954,7 @@ static int of_overlay_apply(const void *fdt, struct device_node *tree,
> /*
> * after overlay_notify(), ovcs->overlay_tree related pointers may have
> * leaked to drivers, so can not kfree() tree, aka ovcs->overlay_tree;
> - * and can not free fdt, aka ovcs->fdt
> + * and can not free memory containing aligned fdt, aka ovcs->fdt
ovcs->fdt does not point to the aligned fdt, but to the allocated fdt.
> */
> ret = overlay_notify(ovcs, OF_OVERLAY_PRE_APPLY);
> if (ret) {
> @@ -1014,7 +1015,8 @@ 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;
> + void *new_fdt_align;
> int ret;
> u32 size;
> struct device_node *overlay_root;
> @@ -1036,18 +1038,22 @@ 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;
>
> - of_fdt_unflatten_tree(new_fdt, NULL, &overlay_root);
> + new_fdt_align = PTR_ALIGN(new_fdt, FDT_ALIGN_SIZE);
> + memcpy(new_fdt_align, overlay_fdt, size);
Still copies beyond end of buffer.
> +
> + of_fdt_unflatten_tree(new_fdt_align, NULL, &overlay_root);
> if (!overlay_root) {
> pr_err("unable to unflatten overlay_fdt\n");
> ret = -EINVAL;
> goto out_free_new_fdt;
> }
>
> - ret = of_overlay_apply(new_fdt, overlay_root, ovcs_id);
> + ret = of_overlay_apply(new_fdt, new_fdt_align, overlay_root, ovcs_id);
> if (ret < 0) {
> /*
> * new_fdt and overlay_root now belong to the overlay
> diff --git a/drivers/of/unittest.c b/drivers/of/unittest.c
> index eb100627c186..29081a8b32e6 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>
> @@ -1408,6 +1409,7 @@ static void attach_node_and_children(struct device_node *np)
> static int __init unittest_data_add(void)
> {
> void *unittest_data;
> + void *unittest_data_align;
> struct device_node *unittest_data_node, *np;
> /*
> * __dtb_testcases_begin[] and __dtb_testcases_end[] are magically
> @@ -1415,7 +1417,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,11 +1427,15 @@ 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;
>
> - of_fdt_unflatten_tree(unittest_data, NULL, &unittest_data_node);
> + unittest_data_align = PTR_ALIGN(unittest_data, FDT_ALIGN_SIZE);
> + memcpy(unittest_data_align, __dtb_testcases_begin, size);
Same as above.
> +
> + of_fdt_unflatten_tree(unittest_data_align, NULL, &unittest_data_node);
> if (!unittest_data_node) {
> pr_warn("%s: No tree to attach; not running tests\n", __func__);
> kfree(unittest_data);
>
On Thu, Apr 8, 2021 at 10:17 AM <[email protected]> wrote:
>
> From: Frank Rowand <[email protected]>
>
> 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/[email protected]/
>
> Reported-by: Guenter Roeck <[email protected]>
> Signed-off-by: Frank Rowand <[email protected]>
> ---
>
> changes since version 1:
> - use pointer from kmalloc() for kfree() instead of using pointer that
> has been modified for FDT alignment
>
> changes since version 2:
> - version 1 was a work in progress version, I failed to commit the following
> final changes
> - reorder first two arguments of of_overlay_apply()
>
> drivers/of/of_private.h | 2 ++
> drivers/of/overlay.c | 28 +++++++++++++++++-----------
> drivers/of/unittest.c | 12 +++++++++---
> 3 files changed, 28 insertions(+), 14 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..cf770452e1e5 100644
> --- a/drivers/of/overlay.c
> +++ b/drivers/of/overlay.c
> @@ -57,7 +57,7 @@ struct fragment {
> * struct overlay_changeset
> * @id: changeset identifier
> * @ovcs_list: list on which we are located
> - * @fdt: FDT that was unflattened to create @overlay_tree
> + * @fdt: base of memory allocated to hold aligned FDT that was unflattened to create @overlay_tree
> * @overlay_tree: expanded device tree that contains the fragment nodes
> * @count: count of fragment structures
> * @fragments: fragment nodes in the overlay expanded device tree
> @@ -719,8 +719,8 @@ static struct device_node *find_target(struct device_node *info_node)
> /**
> * init_overlay_changeset() - initialize overlay changeset from overlay tree
> * @ovcs: Overlay changeset to build
> - * @fdt: the FDT that was unflattened to create @tree
> - * @tree: Contains all the overlay fragments and overlay fixup nodes
> + * @fdt: base of memory allocated to hold aligned FDT that was unflattened to create @tree
> + * @tree: Contains the overlay fragments and overlay fixup nodes
> *
> * Initialize @ovcs. Populate @ovcs->fragments with node information from
> * the top level of @tree. The relevant top level nodes are the fragment
> @@ -873,7 +873,8 @@ static void free_overlay_changeset(struct overlay_changeset *ovcs)
> * internal documentation
> *
> * of_overlay_apply() - Create and apply an overlay changeset
> - * @fdt: the FDT that was unflattened to create @tree
> + * @fdt: base of memory allocated to hold *@fdt_align
> + * @fdt_align: the FDT that was unflattened to create @tree, aligned
> * @tree: Expanded overlay device tree
> * @ovcs_id: Pointer to overlay changeset id
> *
> @@ -912,8 +913,8 @@ static void free_overlay_changeset(struct overlay_changeset *ovcs)
> * id is returned to *ovcs_id.
> */
>
> -static int of_overlay_apply(const void *fdt, struct device_node *tree,
> - int *ovcs_id)
> +static int of_overlay_apply(const void *fdt, const void *fdt_align,
> + struct device_node *tree, int *ovcs_id)
I think it's better if you move the kfree's out of this function. It
would be a broken design if this function was public because you'd
have no idea if 'fdt' could be freed or not. No reason to have that
bad design just because it's static. If a function returns an error,
then it should undo everything it did, but nothing more.
Rob
Hi Guenter,
Thanks for the review!
On 4/8/21 10:32 AM, Guenter Roeck wrote:
> On 4/8/21 8:17 AM, [email protected] wrote:
>> From: Frank Rowand <[email protected]>
>>
>> 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/[email protected]/
>>
>> Reported-by: Guenter Roeck <[email protected]>
>> Signed-off-by: Frank Rowand <[email protected]>
>> ---
>>
>> changes since version 1:
>> - use pointer from kmalloc() for kfree() instead of using pointer that
>> has been modified for FDT alignment
>>
>> changes since version 2:
>> - version 1 was a work in progress version, I failed to commit the following
>> final changes
>> - reorder first two arguments of of_overlay_apply()
>>
>> drivers/of/of_private.h | 2 ++
>> drivers/of/overlay.c | 28 +++++++++++++++++-----------
>> drivers/of/unittest.c | 12 +++++++++---
>> 3 files changed, 28 insertions(+), 14 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
>> +
>
> Use existing define ? Or was that local in libfdt ?
I don't see a define in libfdt. If anyone finds one,
I'll switch to it.
>
>> /**
>> * 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..cf770452e1e5 100644
>> --- a/drivers/of/overlay.c
>> +++ b/drivers/of/overlay.c
>> @@ -57,7 +57,7 @@ struct fragment {
>> * struct overlay_changeset
>> * @id: changeset identifier
>> * @ovcs_list: list on which we are located
>> - * @fdt: FDT that was unflattened to create @overlay_tree
>> + * @fdt: base of memory allocated to hold aligned FDT that was unflattened to create @overlay_tree
>> * @overlay_tree: expanded device tree that contains the fragment nodes
>> * @count: count of fragment structures
>> * @fragments: fragment nodes in the overlay expanded device tree
>> @@ -719,8 +719,8 @@ static struct device_node *find_target(struct device_node *info_node)
>> /**
>> * init_overlay_changeset() - initialize overlay changeset from overlay tree
>> * @ovcs: Overlay changeset to build
>> - * @fdt: the FDT that was unflattened to create @tree
>> - * @tree: Contains all the overlay fragments and overlay fixup nodes
>> + * @fdt: base of memory allocated to hold aligned FDT that was unflattened to create @tree
>> + * @tree: Contains the overlay fragments and overlay fixup nodes
>> *
>> * Initialize @ovcs. Populate @ovcs->fragments with node information from
>> * the top level of @tree. The relevant top level nodes are the fragment
>> @@ -873,7 +873,8 @@ static void free_overlay_changeset(struct overlay_changeset *ovcs)
>> * internal documentation
>> *
>> * of_overlay_apply() - Create and apply an overlay changeset
>> - * @fdt: the FDT that was unflattened to create @tree
>> + * @fdt: base of memory allocated to hold *@fdt_align
>> + * @fdt_align: the FDT that was unflattened to create @tree, aligned
>> * @tree: Expanded overlay device tree
>> * @ovcs_id: Pointer to overlay changeset id
>> *
>> @@ -912,8 +913,8 @@ static void free_overlay_changeset(struct overlay_changeset *ovcs)
>> * id is returned to *ovcs_id.
>> */
>>
>> -static int of_overlay_apply(const void *fdt, struct device_node *tree,
>> - int *ovcs_id)
>> +static int of_overlay_apply(const void *fdt, const void *fdt_align,
>
> fdt_align is not used in this function.
Thanks for catching that. Left over from earlier version where I
saved both aligned and unaligned addresses in init_overlay_changeset().
Will remove.
>
>> + struct device_node *tree, int *ovcs_id)
>> {
>> struct overlay_changeset *ovcs;
>> int ret = 0, ret_revert, ret_tmp;
>> @@ -953,7 +954,7 @@ static int of_overlay_apply(const void *fdt, struct device_node *tree,
>> /*
>> * after overlay_notify(), ovcs->overlay_tree related pointers may have
>> * leaked to drivers, so can not kfree() tree, aka ovcs->overlay_tree;
>> - * and can not free fdt, aka ovcs->fdt
>> + * and can not free memory containing aligned fdt, aka ovcs->fdt
>
> ovcs->fdt does not point to the aligned fdt, but to the allocated fdt.
Yes. The comment is not clear enough about that, I will modify.
>
>> */
>> ret = overlay_notify(ovcs, OF_OVERLAY_PRE_APPLY);
>> if (ret) {
>> @@ -1014,7 +1015,8 @@ 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;
>> + void *new_fdt_align;
>> int ret;
>> u32 size;
>> struct device_node *overlay_root;
>> @@ -1036,18 +1038,22 @@ 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;
>>
>> - of_fdt_unflatten_tree(new_fdt, NULL, &overlay_root);
>> + new_fdt_align = PTR_ALIGN(new_fdt, FDT_ALIGN_SIZE);
>> + memcpy(new_fdt_align, overlay_fdt, size);
>
> Still copies beyond end of buffer.
Will fix with separate variables "size" and "size_alloc".
>
>> +
>> + of_fdt_unflatten_tree(new_fdt_align, NULL, &overlay_root);
>> if (!overlay_root) {
>> pr_err("unable to unflatten overlay_fdt\n");
>> ret = -EINVAL;
>> goto out_free_new_fdt;
>> }
>>
>> - ret = of_overlay_apply(new_fdt, overlay_root, ovcs_id);
>> + ret = of_overlay_apply(new_fdt, new_fdt_align, overlay_root, ovcs_id);
>> if (ret < 0) {
>> /*
>> * new_fdt and overlay_root now belong to the overlay
>> diff --git a/drivers/of/unittest.c b/drivers/of/unittest.c
>> index eb100627c186..29081a8b32e6 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>
>> @@ -1408,6 +1409,7 @@ static void attach_node_and_children(struct device_node *np)
>> static int __init unittest_data_add(void)
>> {
>> void *unittest_data;
>> + void *unittest_data_align;
>> struct device_node *unittest_data_node, *np;
>> /*
>> * __dtb_testcases_begin[] and __dtb_testcases_end[] are magically
>> @@ -1415,7 +1417,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,11 +1427,15 @@ 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;
>>
>> - of_fdt_unflatten_tree(unittest_data, NULL, &unittest_data_node);
>> + unittest_data_align = PTR_ALIGN(unittest_data, FDT_ALIGN_SIZE);
>> + memcpy(unittest_data_align, __dtb_testcases_begin, size);
>
> Same as above.
Will fix with separate variables "size" and "size_alloc".
>
>> +
>> + of_fdt_unflatten_tree(unittest_data_align, NULL, &unittest_data_node);
>> if (!unittest_data_node) {
>> pr_warn("%s: No tree to attach; not running tests\n", __func__);
>> kfree(unittest_data);
>>
>
On 4/8/21 1:27 PM, Rob Herring wrote:
> On Thu, Apr 8, 2021 at 10:17 AM <[email protected]> wrote:
>>
>> From: Frank Rowand <[email protected]>
>>
>> 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/[email protected]/
>>
>> Reported-by: Guenter Roeck <[email protected]>
>> Signed-off-by: Frank Rowand <[email protected]>
>> ---
>>
>> changes since version 1:
>> - use pointer from kmalloc() for kfree() instead of using pointer that
>> has been modified for FDT alignment
>>
>> changes since version 2:
>> - version 1 was a work in progress version, I failed to commit the following
>> final changes
>> - reorder first two arguments of of_overlay_apply()
>>
>> drivers/of/of_private.h | 2 ++
>> drivers/of/overlay.c | 28 +++++++++++++++++-----------
>> drivers/of/unittest.c | 12 +++++++++---
>> 3 files changed, 28 insertions(+), 14 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..cf770452e1e5 100644
>> --- a/drivers/of/overlay.c
>> +++ b/drivers/of/overlay.c
>> @@ -57,7 +57,7 @@ struct fragment {
>> * struct overlay_changeset
>> * @id: changeset identifier
>> * @ovcs_list: list on which we are located
>> - * @fdt: FDT that was unflattened to create @overlay_tree
>> + * @fdt: base of memory allocated to hold aligned FDT that was unflattened to create @overlay_tree
>> * @overlay_tree: expanded device tree that contains the fragment nodes
>> * @count: count of fragment structures
>> * @fragments: fragment nodes in the overlay expanded device tree
>> @@ -719,8 +719,8 @@ static struct device_node *find_target(struct device_node *info_node)
>> /**
>> * init_overlay_changeset() - initialize overlay changeset from overlay tree
>> * @ovcs: Overlay changeset to build
>> - * @fdt: the FDT that was unflattened to create @tree
>> - * @tree: Contains all the overlay fragments and overlay fixup nodes
>> + * @fdt: base of memory allocated to hold aligned FDT that was unflattened to create @tree
>> + * @tree: Contains the overlay fragments and overlay fixup nodes
>> *
>> * Initialize @ovcs. Populate @ovcs->fragments with node information from
>> * the top level of @tree. The relevant top level nodes are the fragment
>> @@ -873,7 +873,8 @@ static void free_overlay_changeset(struct overlay_changeset *ovcs)
>> * internal documentation
>> *
>> * of_overlay_apply() - Create and apply an overlay changeset
>> - * @fdt: the FDT that was unflattened to create @tree
>> + * @fdt: base of memory allocated to hold *@fdt_align
>> + * @fdt_align: the FDT that was unflattened to create @tree, aligned
>> * @tree: Expanded overlay device tree
>> * @ovcs_id: Pointer to overlay changeset id
>> *
>> @@ -912,8 +913,8 @@ static void free_overlay_changeset(struct overlay_changeset *ovcs)
>> * id is returned to *ovcs_id.
>> */
>>
>> -static int of_overlay_apply(const void *fdt, struct device_node *tree,
>> - int *ovcs_id)
>> +static int of_overlay_apply(const void *fdt, const void *fdt_align,
>> + struct device_node *tree, int *ovcs_id)
>
> I think it's better if you move the kfree's out of this function. It
> would be a broken design if this function was public because you'd
> have no idea if 'fdt' could be freed or not. No reason to have that
> bad design just because it's static. If a function returns an error,
> then it should undo everything it did, but nothing more.
>
> Rob
>
The pattern of "If a function returns an error, then it should undo
everything it did, but nothing more" is usually what I would expect,
but overlays have more than a bit of bizarro land in their genes.
Once an overlay has been applied, the devicetree subsystem owns and
is responsible for the freeing of the related FDT.
This is noted just after calling of_overlay_apply():
ret = of_overlay_apply(new_fdt, overlay_root, ovcs_id);
if (ret < 0) {
/*
* new_fdt and overlay_root now belong to the overlay
* changeset.
* overlay changeset code is responsible for freeing them.
*/
and is also noted inside of_overlay_apply():
/*
* As of this point, fdt and tree belong to the overlay changeset.
* overlay changeset code is responsible for freeing them.
*/
of_overlay_apply() is not public on purpose. of_overlay_fdt_apply() was
created to be the public function and to provide the encapsulation of
the copy of the FDT for which memory is allocated in of_overlay_fdt_apply().
-Frank
On 4/8/21 1:06 PM, Frank Rowand wrote:
>>> +#define FDT_ALIGN_SIZE 8
>>> +
>>
>> Use existing define ? Or was that local in libfdt ?
>
> I don't see a define in libfdt. If anyone finds one,
> I'll switch to it.
>
Turns out that was hardcoded in scripts/dtc/libfdt/fdt.c
+ /* The device tree must be at an 8-byte aligned address */
+ if ((uintptr_t)fdt & 7)
+ return -FDT_ERR_ALIGNMENT;
+
Guenter