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]>
---
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__);
--
Frank Rowand <[email protected]>
Hi Guenter,
On 4/7/21 3:51 PM, [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]/
Can you please test this patch?
The most complete coverage will be a config with:
OF_UNITTEST=y
OF_OVERLAY=y
OF_DYNAMIC=y
One path will be missed if you test with:
#OF_OVERLAY=n
#OF_DYNAMIC=n
Thanks!
-Frank
>
> Reported-by: Guenter Roeck <[email protected]>
> Signed-off-by: Frank Rowand <[email protected]>
> ---
> 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__);
>
On Wed, Apr 7, 2021 at 3:51 PM <[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]>
> ---
> 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.
The error path is easy to fix. Freeing the memory later on, not so
much... 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
On 4/7/21 1:59 PM, Frank Rowand wrote:
> Hi Guenter,
>
> On 4/7/21 3:51 PM, [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]/
>
> Can you please test this patch?
>
Sure, will do, after you fixed the problem pointed out by Rob.
Sorry, I should have mentioned it - that problem was the reason
why I didn't propose a fix myself.
Guenter
On 4/7/21 4:34 PM, Rob Herring wrote:
> On Wed, Apr 7, 2021 at 3:51 PM <[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]>
>> ---
>> 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
> .
>
On 4/7/21 5:01 PM, Guenter Roeck wrote:
> On 4/7/21 1:59 PM, Frank Rowand wrote:
>> Hi Guenter,
>>
>> On 4/7/21 3:51 PM, [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]/
>>
>> Can you please test this patch?
>>
>
> Sure, will do, after you fixed the problem pointed out by Rob.
>
> Sorry, I should have mentioned it - that problem was the reason
> why I didn't propose a fix myself.
No problem, I was aware of the issue but then spaced on actually
dealing with it.
- Space Cadet Frank
>
> Guenter
>
_______________________________________________
kbuild mailing list -- [email protected]
To unsubscribe send an email to [email protected]