U-Boot Archive on lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] boot: Assure FDT is always at 8-byte aligned address
@ 2025-11-13 11:54 Marek Vasut
  2025-11-13 19:33 ` Simon Glass
  2025-11-28 17:41 ` Tom Rini
  0 siblings, 2 replies; 11+ messages in thread
From: Marek Vasut @ 2025-11-13 11:54 UTC (permalink / raw)
  To: u-boot
  Cc: Marek Vasut, Heinrich Schuchardt, Quentin Schulz, Simon Glass,
	Tom Rini, Wolfgang Wallner

The fitImage may contain FDT at 4-byte aligned address, because alignment
of DT tags is 4 bytes. However, libfdt and also Linux expects DT to be at
8-byte aligned address. Make sure that the DTs embedded in fitImages are
always used from 8-byte aligned addresses. In case the DT is decompressed,
make sure the target buffer is 8-byte aligned. In case the DT is only
loaded, make sure the target buffer is 8-byte aligned too.

Signed-off-by: Marek Vasut <marek.vasut+renesas@mailbox.org>
---
Cc: Heinrich Schuchardt <xypron.glpk@gmx.de>
Cc: Quentin Schulz <quentin.schulz@cherry.de>
Cc: Simon Glass <sjg@chromium.org>
Cc: Tom Rini <trini@konsulko.com>
Cc: Wolfgang Wallner <wolfgang.wallner@br-automation.com>
Cc: u-boot@lists.denx.de
---
 boot/image-fit.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/boot/image-fit.c b/boot/image-fit.c
index 2f2d3e9304d..cccaa48f683 100644
--- a/boot/image-fit.c
+++ b/boot/image-fit.c
@@ -23,7 +23,6 @@
 #include <log.h>
 #include <mapmem.h>
 #include <asm/io.h>
-#include <malloc.h>
 #include <memalign.h>
 #include <asm/global_data.h>
 #ifdef CONFIG_DM_HASH
@@ -36,6 +35,7 @@ DECLARE_GLOBAL_DATA_PTR;
 #include <bootm.h>
 #include <image.h>
 #include <bootstage.h>
+#include <malloc.h>
 #include <upl.h>
 #include <u-boot/crc.h>
 
@@ -2279,7 +2279,7 @@ int fit_image_load(struct bootm_headers *images, ulong addr,
 
 		log_debug("decompressing image\n");
 		if (load == data) {
-			loadbuf = malloc(max_decomp_len);
+			loadbuf = memalign(8, max_decomp_len);
 			load = map_to_sysmem(loadbuf);
 		} else {
 			loadbuf = map_sysmem(load, max_decomp_len);
@@ -2291,6 +2291,11 @@ int fit_image_load(struct bootm_headers *images, ulong addr,
 			return -ENOEXEC;
 		}
 		len = load_end - load;
+	} else if (load_op != FIT_LOAD_IGNORED && image_type == IH_TYPE_FLATDT &&
+		   ((uintptr_t)buf & 7)) {
+		loadbuf = memalign(8, len);
+		load = map_to_sysmem(loadbuf);
+		memcpy(loadbuf, buf, len);
 	} else if (load != data) {
 		log_debug("copying\n");
 		loadbuf = map_sysmem(load, len);
-- 
2.51.0


^ permalink raw reply related	[flat|nested] 11+ messages in thread

* Re: [PATCH] boot: Assure FDT is always at 8-byte aligned address
  2025-11-13 11:54 [PATCH] boot: Assure FDT is always at 8-byte aligned address Marek Vasut
@ 2025-11-13 19:33 ` Simon Glass
  2025-11-13 21:56   ` Marek Vasut
  2025-11-28 17:41 ` Tom Rini
  1 sibling, 1 reply; 11+ messages in thread
From: Simon Glass @ 2025-11-13 19:33 UTC (permalink / raw)
  To: Marek Vasut
  Cc: u-boot, Heinrich Schuchardt, Quentin Schulz, Tom Rini,
	Wolfgang Wallner

Hi Marek,

On Thu, 13 Nov 2025 at 04:55, Marek Vasut
<marek.vasut+renesas@mailbox.org> wrote:
>
> The fitImage may contain FDT at 4-byte aligned address, because alignment
> of DT tags is 4 bytes. However, libfdt and also Linux expects DT to be at
> 8-byte aligned address. Make sure that the DTs embedded in fitImages are
> always used from 8-byte aligned addresses. In case the DT is decompressed,
> make sure the target buffer is 8-byte aligned. In case the DT is only
> loaded, make sure the target buffer is 8-byte aligned too.
>
> Signed-off-by: Marek Vasut <marek.vasut+renesas@mailbox.org>
> ---
> Cc: Heinrich Schuchardt <xypron.glpk@gmx.de>
> Cc: Quentin Schulz <quentin.schulz@cherry.de>
> Cc: Simon Glass <sjg@chromium.org>
> Cc: Tom Rini <trini@konsulko.com>
> Cc: Wolfgang Wallner <wolfgang.wallner@br-automation.com>
> Cc: u-boot@lists.denx.de
> ---
>  boot/image-fit.c | 9 +++++++--
>  1 file changed, 7 insertions(+), 2 deletions(-)
>
> diff --git a/boot/image-fit.c b/boot/image-fit.c
> index 2f2d3e9304d..cccaa48f683 100644
> --- a/boot/image-fit.c
> +++ b/boot/image-fit.c
> @@ -23,7 +23,6 @@
>  #include <log.h>
>  #include <mapmem.h>
>  #include <asm/io.h>
> -#include <malloc.h>
>  #include <memalign.h>
>  #include <asm/global_data.h>
>  #ifdef CONFIG_DM_HASH
> @@ -36,6 +35,7 @@ DECLARE_GLOBAL_DATA_PTR;
>  #include <bootm.h>
>  #include <image.h>
>  #include <bootstage.h>
> +#include <malloc.h>
>  #include <upl.h>
>  #include <u-boot/crc.h>
>
> @@ -2279,7 +2279,7 @@ int fit_image_load(struct bootm_headers *images, ulong addr,
>
>                 log_debug("decompressing image\n");
>                 if (load == data) {
> -                       loadbuf = malloc(max_decomp_len);
> +                       loadbuf = memalign(8, max_decomp_len);
>                         load = map_to_sysmem(loadbuf);
>                 } else {
>                         loadbuf = map_sysmem(load, max_decomp_len);
> @@ -2291,6 +2291,11 @@ int fit_image_load(struct bootm_headers *images, ulong addr,
>                         return -ENOEXEC;
>                 }
>                 len = load_end - load;
> +       } else if (load_op != FIT_LOAD_IGNORED && image_type == IH_TYPE_FLATDT &&
> +                  ((uintptr_t)buf & 7)) {
> +               loadbuf = memalign(8, len);
> +               load = map_to_sysmem(loadbuf);
> +               memcpy(loadbuf, buf, len);
>         } else if (load != data) {
>                 log_debug("copying\n");
>                 loadbuf = map_sysmem(load, len);
> --
> 2.51.0
>

We really can't do a memory allocation in fit_image_load() !

The problem should be fixed higher up, in its callers, etc. For one
thing, the caller knows whether it is a DT or not, so putting this
logic here is messy. See boot_get_fdt_fit()

Regards,
Simon

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] boot: Assure FDT is always at 8-byte aligned address
  2025-11-13 19:33 ` Simon Glass
@ 2025-11-13 21:56   ` Marek Vasut
  2025-11-13 22:46     ` Simon Glass
  0 siblings, 1 reply; 11+ messages in thread
From: Marek Vasut @ 2025-11-13 21:56 UTC (permalink / raw)
  To: Simon Glass, Marek Vasut
  Cc: u-boot, Heinrich Schuchardt, Quentin Schulz, Tom Rini,
	Wolfgang Wallner

On 11/13/25 8:33 PM, Simon Glass wrote:

Hello Simon,

>> The fitImage may contain FDT at 4-byte aligned address, because alignment
>> of DT tags is 4 bytes. However, libfdt and also Linux expects DT to be at
>> 8-byte aligned address. Make sure that the DTs embedded in fitImages are
>> always used from 8-byte aligned addresses. In case the DT is decompressed,
>> make sure the target buffer is 8-byte aligned. In case the DT is only
>> loaded, make sure the target buffer is 8-byte aligned too.
>>
>> Signed-off-by: Marek Vasut <marek.vasut+renesas@mailbox.org>
>> ---
>> Cc: Heinrich Schuchardt <xypron.glpk@gmx.de>
>> Cc: Quentin Schulz <quentin.schulz@cherry.de>
>> Cc: Simon Glass <sjg@chromium.org>
>> Cc: Tom Rini <trini@konsulko.com>
>> Cc: Wolfgang Wallner <wolfgang.wallner@br-automation.com>
>> Cc: u-boot@lists.denx.de
>> ---
>>   boot/image-fit.c | 9 +++++++--
>>   1 file changed, 7 insertions(+), 2 deletions(-)
>>
>> diff --git a/boot/image-fit.c b/boot/image-fit.c
>> index 2f2d3e9304d..cccaa48f683 100644
>> --- a/boot/image-fit.c
>> +++ b/boot/image-fit.c
>> @@ -23,7 +23,6 @@
>>   #include <log.h>
>>   #include <mapmem.h>
>>   #include <asm/io.h>
>> -#include <malloc.h>
>>   #include <memalign.h>
>>   #include <asm/global_data.h>
>>   #ifdef CONFIG_DM_HASH
>> @@ -36,6 +35,7 @@ DECLARE_GLOBAL_DATA_PTR;
>>   #include <bootm.h>
>>   #include <image.h>
>>   #include <bootstage.h>
>> +#include <malloc.h>
>>   #include <upl.h>
>>   #include <u-boot/crc.h>
>>
>> @@ -2279,7 +2279,7 @@ int fit_image_load(struct bootm_headers *images, ulong addr,
>>
>>                  log_debug("decompressing image\n");
>>                  if (load == data) {
>> -                       loadbuf = malloc(max_decomp_len);
>> +                       loadbuf = memalign(8, max_decomp_len);
>>                          load = map_to_sysmem(loadbuf);
>>                  } else {
>>                          loadbuf = map_sysmem(load, max_decomp_len);
>> @@ -2291,6 +2291,11 @@ int fit_image_load(struct bootm_headers *images, ulong addr,
>>                          return -ENOEXEC;
>>                  }
>>                  len = load_end - load;
>> +       } else if (load_op != FIT_LOAD_IGNORED && image_type == IH_TYPE_FLATDT &&
>> +                  ((uintptr_t)buf & 7)) {
>> +               loadbuf = memalign(8, len);
>> +               load = map_to_sysmem(loadbuf);
>> +               memcpy(loadbuf, buf, len);
>>          } else if (load != data) {
>>                  log_debug("copying\n");
>>                  loadbuf = map_sysmem(load, len);
>> --
>> 2.51.0
>>
> 
> We really can't do a memory allocation in fit_image_load() !

We already do, notice the malloc() when "decompressing image" part.

> The problem should be fixed higher up, in its callers, etc. For one
> thing, the caller knows whether it is a DT or not, so putting this
> logic here is messy. See boot_get_fdt_fit()

This is triggered by a fitImage with DT embedded in that fitImage as 
non-external-data. That DT is at 4-byte aligned address, so how can the 
caller deal with it ? The caller gets a fitImage, which itself is at 
8-byte aligned address, but the DT in it is not at 8-byte aligned 
address, so that DT has to be relocated somehow and that happens here.

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] boot: Assure FDT is always at 8-byte aligned address
  2025-11-13 21:56   ` Marek Vasut
@ 2025-11-13 22:46     ` Simon Glass
  2025-11-15 17:19       ` Marek Vasut
  0 siblings, 1 reply; 11+ messages in thread
From: Simon Glass @ 2025-11-13 22:46 UTC (permalink / raw)
  To: Marek Vasut
  Cc: Marek Vasut, u-boot, Heinrich Schuchardt, Quentin Schulz,
	Tom Rini, Wolfgang Wallner

Hi Marek,

On Thu, 13 Nov 2025 at 14:57, Marek Vasut <marek.vasut@mailbox.org> wrote:
>
> On 11/13/25 8:33 PM, Simon Glass wrote:
>
> Hello Simon,
>
> >> The fitImage may contain FDT at 4-byte aligned address, because alignment
> >> of DT tags is 4 bytes. However, libfdt and also Linux expects DT to be at
> >> 8-byte aligned address. Make sure that the DTs embedded in fitImages are
> >> always used from 8-byte aligned addresses. In case the DT is decompressed,
> >> make sure the target buffer is 8-byte aligned. In case the DT is only
> >> loaded, make sure the target buffer is 8-byte aligned too.
> >>
> >> Signed-off-by: Marek Vasut <marek.vasut+renesas@mailbox.org>
> >> ---
> >> Cc: Heinrich Schuchardt <xypron.glpk@gmx.de>
> >> Cc: Quentin Schulz <quentin.schulz@cherry.de>
> >> Cc: Simon Glass <sjg@chromium.org>
> >> Cc: Tom Rini <trini@konsulko.com>
> >> Cc: Wolfgang Wallner <wolfgang.wallner@br-automation.com>
> >> Cc: u-boot@lists.denx.de
> >> ---
> >>   boot/image-fit.c | 9 +++++++--
> >>   1 file changed, 7 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/boot/image-fit.c b/boot/image-fit.c
> >> index 2f2d3e9304d..cccaa48f683 100644
> >> --- a/boot/image-fit.c
> >> +++ b/boot/image-fit.c
> >> @@ -23,7 +23,6 @@
> >>   #include <log.h>
> >>   #include <mapmem.h>
> >>   #include <asm/io.h>
> >> -#include <malloc.h>
> >>   #include <memalign.h>
> >>   #include <asm/global_data.h>
> >>   #ifdef CONFIG_DM_HASH
> >> @@ -36,6 +35,7 @@ DECLARE_GLOBAL_DATA_PTR;
> >>   #include <bootm.h>
> >>   #include <image.h>
> >>   #include <bootstage.h>
> >> +#include <malloc.h>
> >>   #include <upl.h>
> >>   #include <u-boot/crc.h>
> >>
> >> @@ -2279,7 +2279,7 @@ int fit_image_load(struct bootm_headers *images, ulong addr,
> >>
> >>                  log_debug("decompressing image\n");
> >>                  if (load == data) {
> >> -                       loadbuf = malloc(max_decomp_len);
> >> +                       loadbuf = memalign(8, max_decomp_len);
> >>                          load = map_to_sysmem(loadbuf);
> >>                  } else {
> >>                          loadbuf = map_sysmem(load, max_decomp_len);
> >> @@ -2291,6 +2291,11 @@ int fit_image_load(struct bootm_headers *images, ulong addr,
> >>                          return -ENOEXEC;
> >>                  }
> >>                  len = load_end - load;
> >> +       } else if (load_op != FIT_LOAD_IGNORED && image_type == IH_TYPE_FLATDT &&
> >> +                  ((uintptr_t)buf & 7)) {
> >> +               loadbuf = memalign(8, len);
> >> +               load = map_to_sysmem(loadbuf);
> >> +               memcpy(loadbuf, buf, len);
> >>          } else if (load != data) {
> >>                  log_debug("copying\n");
> >>                  loadbuf = map_sysmem(load, len);
> >> --
> >> 2.51.0
> >>
> >
> > We really can't do a memory allocation in fit_image_load() !
>
> We already do, notice the malloc() when "decompressing image" part.
>
> > The problem should be fixed higher up, in its callers, etc. For one
> > thing, the caller knows whether it is a DT or not, so putting this
> > logic here is messy. See boot_get_fdt_fit()
>
> This is triggered by a fitImage with DT embedded in that fitImage as
> non-external-data. That DT is at 4-byte aligned address, so how can the
> caller deal with it ? The caller gets a fitImage, which itself is at
> 8-byte aligned address, but the DT in it is not at 8-byte aligned
> address, so that DT has to be relocated somehow and that happens here.

OK, but don't put the code in here...see boot_get_fdt_fit(). But even
then, why does it matter where the FDT is? We are going to use
fdt_openinto() at some point and put it elsewhere, right, so we can do
pre-boot fixups?

Perhaps we should deprecate FITs with internal data, too?

Regards,
Simon

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] boot: Assure FDT is always at 8-byte aligned address
  2025-11-13 22:46     ` Simon Glass
@ 2025-11-15 17:19       ` Marek Vasut
  2025-11-17 17:04         ` Simon Glass
  0 siblings, 1 reply; 11+ messages in thread
From: Marek Vasut @ 2025-11-15 17:19 UTC (permalink / raw)
  To: Simon Glass
  Cc: Marek Vasut, u-boot, Heinrich Schuchardt, Quentin Schulz,
	Tom Rini, Wolfgang Wallner

On 11/13/25 11:46 PM, Simon Glass wrote:

Hello Simon,

>>> The problem should be fixed higher up, in its callers, etc. For one
>>> thing, the caller knows whether it is a DT or not, so putting this
>>> logic here is messy. See boot_get_fdt_fit()
>>
>> This is triggered by a fitImage with DT embedded in that fitImage as
>> non-external-data. That DT is at 4-byte aligned address, so how can the
>> caller deal with it ? The caller gets a fitImage, which itself is at
>> 8-byte aligned address, but the DT in it is not at 8-byte aligned
>> address, so that DT has to be relocated somehow and that happens here.
> 
> OK, but don't put the code in here...see boot_get_fdt_fit(). But even
> then, why does it matter where the FDT is?

The FDT has to be at 8-byte aligned offset.

> We are going to use
> fdt_openinto() at some point and put it elsewhere, right, so we can do
> pre-boot fixups?

Look at the fit load code, cca. 10 lines below, it checks the FDT for 
validity using fdt_check_header() . At that point, the FDT must be at 
8-byte aligned address already:

2294         } else if (load_op != FIT_LOAD_IGNORED && image_type == 
IH_TYPE_FLATDT &&
2295                    ((uintptr_t)buf & 7)) {
2296                 loadbuf = memalign(8, len);
2297                 load = map_to_sysmem(loadbuf);
2298                 memcpy(loadbuf, buf, len);

...

2309         /* verify that image data is a proper FDT blob */
2310         if (load_op != FIT_LOAD_IGNORED && image_type == 
IH_TYPE_FLATDT &&
2311             fdt_check_header(loadbuf)) { <----------------- this
2312                 puts("Subimage data is not a FDT\n");
2313                 return -ENOEXEC;
2314         }

> Perhaps we should deprecate FITs with internal data, too?
We cannot break compatibility and stop supporting old fitImage, so this 
is irrelevant here.

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] boot: Assure FDT is always at 8-byte aligned address
  2025-11-15 17:19       ` Marek Vasut
@ 2025-11-17 17:04         ` Simon Glass
  2025-11-17 17:10           ` Marek Vasut
  0 siblings, 1 reply; 11+ messages in thread
From: Simon Glass @ 2025-11-17 17:04 UTC (permalink / raw)
  To: Marek Vasut
  Cc: Marek Vasut, u-boot, Heinrich Schuchardt, Quentin Schulz,
	Tom Rini, Wolfgang Wallner

Hi Marek,

On Sat, 15 Nov 2025 at 17:20, Marek Vasut <marek.vasut@mailbox.org> wrote:
>
> On 11/13/25 11:46 PM, Simon Glass wrote:
>
> Hello Simon,
>
> >>> The problem should be fixed higher up, in its callers, etc. For one
> >>> thing, the caller knows whether it is a DT or not, so putting this
> >>> logic here is messy. See boot_get_fdt_fit()
> >>
> >> This is triggered by a fitImage with DT embedded in that fitImage as
> >> non-external-data. That DT is at 4-byte aligned address, so how can the
> >> caller deal with it ? The caller gets a fitImage, which itself is at
> >> 8-byte aligned address, but the DT in it is not at 8-byte aligned
> >> address, so that DT has to be relocated somehow and that happens here.
> >
> > OK, but don't put the code in here...see boot_get_fdt_fit(). But even
> > then, why does it matter where the FDT is?
>
> The FDT has to be at 8-byte aligned offset.
>
> > We are going to use
> > fdt_openinto() at some point and put it elsewhere, right, so we can do
> > pre-boot fixups?
>
> Look at the fit load code, cca. 10 lines below, it checks the FDT for
> validity using fdt_check_header() . At that point, the FDT must be at
> 8-byte aligned address already:
>
> 2294         } else if (load_op != FIT_LOAD_IGNORED && image_type ==
> IH_TYPE_FLATDT &&
> 2295                    ((uintptr_t)buf & 7)) {
> 2296                 loadbuf = memalign(8, len);
> 2297                 load = map_to_sysmem(loadbuf);
> 2298                 memcpy(loadbuf, buf, len);
>
> ...
>
> 2309         /* verify that image data is a proper FDT blob */
> 2310         if (load_op != FIT_LOAD_IGNORED && image_type ==
> IH_TYPE_FLATDT &&
> 2311             fdt_check_header(loadbuf)) { <----------------- this
> 2312                 puts("Subimage data is not a FDT\n");
> 2313                 return -ENOEXEC;
> 2314         }
>
> > Perhaps we should deprecate FITs with internal data, too?
> We cannot break compatibility and stop supporting old fitImage, so this
> is irrelevant here.

OK, so please create a function which can detect an FDT header without
it being aligned, like the other code you wrote. Then it will be safe
to call that here, even if unaligned.

Regards,
Simon

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] boot: Assure FDT is always at 8-byte aligned address
  2025-11-17 17:04         ` Simon Glass
@ 2025-11-17 17:10           ` Marek Vasut
  2025-11-18  3:47             ` Simon Glass
  0 siblings, 1 reply; 11+ messages in thread
From: Marek Vasut @ 2025-11-17 17:10 UTC (permalink / raw)
  To: Simon Glass
  Cc: Marek Vasut, u-boot, Heinrich Schuchardt, Quentin Schulz,
	Tom Rini, Wolfgang Wallner

On 11/17/25 6:04 PM, Simon Glass wrote:

Hello Simon,

>>>>> The problem should be fixed higher up, in its callers, etc. For one
>>>>> thing, the caller knows whether it is a DT or not, so putting this
>>>>> logic here is messy. See boot_get_fdt_fit()
>>>>
>>>> This is triggered by a fitImage with DT embedded in that fitImage as
>>>> non-external-data. That DT is at 4-byte aligned address, so how can the
>>>> caller deal with it ? The caller gets a fitImage, which itself is at
>>>> 8-byte aligned address, but the DT in it is not at 8-byte aligned
>>>> address, so that DT has to be relocated somehow and that happens here.
>>>
>>> OK, but don't put the code in here...see boot_get_fdt_fit(). But even
>>> then, why does it matter where the FDT is?
>>
>> The FDT has to be at 8-byte aligned offset.
>>
>>> We are going to use
>>> fdt_openinto() at some point and put it elsewhere, right, so we can do
>>> pre-boot fixups?
>>
>> Look at the fit load code, cca. 10 lines below, it checks the FDT for
>> validity using fdt_check_header() . At that point, the FDT must be at
>> 8-byte aligned address already:
>>
>> 2294         } else if (load_op != FIT_LOAD_IGNORED && image_type ==
>> IH_TYPE_FLATDT &&
>> 2295                    ((uintptr_t)buf & 7)) {
>> 2296                 loadbuf = memalign(8, len);
>> 2297                 load = map_to_sysmem(loadbuf);
>> 2298                 memcpy(loadbuf, buf, len);
>>
>> ...
>>
>> 2309         /* verify that image data is a proper FDT blob */
>> 2310         if (load_op != FIT_LOAD_IGNORED && image_type ==
>> IH_TYPE_FLATDT &&
>> 2311             fdt_check_header(loadbuf)) { <----------------- this
>> 2312                 puts("Subimage data is not a FDT\n");
>> 2313                 return -ENOEXEC;
>> 2314         }
>>
>>> Perhaps we should deprecate FITs with internal data, too?
>> We cannot break compatibility and stop supporting old fitImage, so this
>> is irrelevant here.
> 
> OK, so please create a function which can detect an FDT header without
> it being aligned, like the other code you wrote. Then it will be safe
> to call that here, even if unaligned.
But we actually do want to detect unaligned broken FDT header and either 
fix it up or stop processing, we don't want to perpetuate handling of 
broken FDTs and pretend that is OK, it shouldn't be I think. Hence this 
fixup.

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] boot: Assure FDT is always at 8-byte aligned address
  2025-11-17 17:10           ` Marek Vasut
@ 2025-11-18  3:47             ` Simon Glass
  2025-11-19 21:29               ` Marek Vasut
  0 siblings, 1 reply; 11+ messages in thread
From: Simon Glass @ 2025-11-18  3:47 UTC (permalink / raw)
  To: Marek Vasut
  Cc: Marek Vasut, u-boot, Heinrich Schuchardt, Quentin Schulz,
	Tom Rini, Wolfgang Wallner

Hi Marek,

On Mon, 17 Nov 2025 at 19:25, Marek Vasut <marek.vasut@mailbox.org> wrote:
>
> On 11/17/25 6:04 PM, Simon Glass wrote:
>
> Hello Simon,
>
> >>>>> The problem should be fixed higher up, in its callers, etc. For one
> >>>>> thing, the caller knows whether it is a DT or not, so putting this
> >>>>> logic here is messy. See boot_get_fdt_fit()
> >>>>
> >>>> This is triggered by a fitImage with DT embedded in that fitImage as
> >>>> non-external-data. That DT is at 4-byte aligned address, so how can the
> >>>> caller deal with it ? The caller gets a fitImage, which itself is at
> >>>> 8-byte aligned address, but the DT in it is not at 8-byte aligned
> >>>> address, so that DT has to be relocated somehow and that happens here.
> >>>
> >>> OK, but don't put the code in here...see boot_get_fdt_fit(). But even
> >>> then, why does it matter where the FDT is?
> >>
> >> The FDT has to be at 8-byte aligned offset.
> >>
> >>> We are going to use
> >>> fdt_openinto() at some point and put it elsewhere, right, so we can do
> >>> pre-boot fixups?
> >>
> >> Look at the fit load code, cca. 10 lines below, it checks the FDT for
> >> validity using fdt_check_header() . At that point, the FDT must be at
> >> 8-byte aligned address already:
> >>
> >> 2294         } else if (load_op != FIT_LOAD_IGNORED && image_type ==
> >> IH_TYPE_FLATDT &&
> >> 2295                    ((uintptr_t)buf & 7)) {
> >> 2296                 loadbuf = memalign(8, len);
> >> 2297                 load = map_to_sysmem(loadbuf);
> >> 2298                 memcpy(loadbuf, buf, len);
> >>
> >> ...
> >>
> >> 2309         /* verify that image data is a proper FDT blob */
> >> 2310         if (load_op != FIT_LOAD_IGNORED && image_type ==
> >> IH_TYPE_FLATDT &&
> >> 2311             fdt_check_header(loadbuf)) { <----------------- this
> >> 2312                 puts("Subimage data is not a FDT\n");
> >> 2313                 return -ENOEXEC;
> >> 2314         }
> >>
> >>> Perhaps we should deprecate FITs with internal data, too?
> >> We cannot break compatibility and stop supporting old fitImage, so this
> >> is irrelevant here.
> >
> > OK, so please create a function which can detect an FDT header without
> > it being aligned, like the other code you wrote. Then it will be safe
> > to call that here, even if unaligned.
> But we actually do want to detect unaligned broken FDT header and either
> fix it up or stop processing, we don't want to perpetuate handling of
> broken FDTs and pretend that is OK, it shouldn't be I think. Hence this
> fixup.

The decision as to whether something is an FDT is made a lot earlier
than the actual processing of it. For the former there is no need to
allocate and copy. For the latter we need to.

Anyway, you are doing the patches, so do what you think is best. But
please create a function for this, rather than lots of little
hand-crafted checks around the place.

Regards,
Simon

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] boot: Assure FDT is always at 8-byte aligned address
  2025-11-18  3:47             ` Simon Glass
@ 2025-11-19 21:29               ` Marek Vasut
  2025-11-20  2:07                 ` Simon Glass
  0 siblings, 1 reply; 11+ messages in thread
From: Marek Vasut @ 2025-11-19 21:29 UTC (permalink / raw)
  To: Simon Glass
  Cc: Marek Vasut, u-boot, Heinrich Schuchardt, Quentin Schulz,
	Tom Rini, Wolfgang Wallner

On 11/18/25 4:47 AM, Simon Glass wrote:

Hello Simon,

>>>> 2294         } else if (load_op != FIT_LOAD_IGNORED && image_type ==
>>>> IH_TYPE_FLATDT &&
>>>> 2295                    ((uintptr_t)buf & 7)) {
>>>> 2296                 loadbuf = memalign(8, len);
>>>> 2297                 load = map_to_sysmem(loadbuf);
>>>> 2298                 memcpy(loadbuf, buf, len);
>>>>
>>>> ...
>>>>
>>>> 2309         /* verify that image data is a proper FDT blob */
>>>> 2310         if (load_op != FIT_LOAD_IGNORED && image_type ==
>>>> IH_TYPE_FLATDT &&
>>>> 2311             fdt_check_header(loadbuf)) { <----------------- this
>>>> 2312                 puts("Subimage data is not a FDT\n");
>>>> 2313                 return -ENOEXEC;
>>>> 2314         }
>>>>
>>>>> Perhaps we should deprecate FITs with internal data, too?
>>>> We cannot break compatibility and stop supporting old fitImage, so this
>>>> is irrelevant here.
>>>
>>> OK, so please create a function which can detect an FDT header without
>>> it being aligned, like the other code you wrote. Then it will be safe
>>> to call that here, even if unaligned.
>> But we actually do want to detect unaligned broken FDT header and either
>> fix it up or stop processing, we don't want to perpetuate handling of
>> broken FDTs and pretend that is OK, it shouldn't be I think. Hence this
>> fixup.
> 
> The decision as to whether something is an FDT is made a lot earlier
> than the actual processing of it. For the former there is no need to
> allocate and copy. For the latter we need to.

The test whether the FDT is valid cannot be done earlier, this place is 
literally the first place when the FDT is decompressed/loaded to the 
target location.

> Anyway, you are doing the patches, so do what you think is best. But
> please create a function for this, rather than lots of little
> hand-crafted checks around the place.
I am confused, function for what exactly ?

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] boot: Assure FDT is always at 8-byte aligned address
  2025-11-19 21:29               ` Marek Vasut
@ 2025-11-20  2:07                 ` Simon Glass
  0 siblings, 0 replies; 11+ messages in thread
From: Simon Glass @ 2025-11-20  2:07 UTC (permalink / raw)
  To: Marek Vasut
  Cc: Marek Vasut, u-boot, Heinrich Schuchardt, Quentin Schulz,
	Tom Rini, Wolfgang Wallner

Hi Marek,

On Wed, 19 Nov 2025 at 17:20, Marek Vasut <marek.vasut@mailbox.org> wrote:
>
> On 11/18/25 4:47 AM, Simon Glass wrote:
>
> Hello Simon,
>
> >>>> 2294         } else if (load_op != FIT_LOAD_IGNORED && image_type ==
> >>>> IH_TYPE_FLATDT &&
> >>>> 2295                    ((uintptr_t)buf & 7)) {
> >>>> 2296                 loadbuf = memalign(8, len);
> >>>> 2297                 load = map_to_sysmem(loadbuf);
> >>>> 2298                 memcpy(loadbuf, buf, len);
> >>>>
> >>>> ...
> >>>>
> >>>> 2309         /* verify that image data is a proper FDT blob */
> >>>> 2310         if (load_op != FIT_LOAD_IGNORED && image_type ==
> >>>> IH_TYPE_FLATDT &&
> >>>> 2311             fdt_check_header(loadbuf)) { <----------------- this
> >>>> 2312                 puts("Subimage data is not a FDT\n");
> >>>> 2313                 return -ENOEXEC;
> >>>> 2314         }
> >>>>
> >>>>> Perhaps we should deprecate FITs with internal data, too?
> >>>> We cannot break compatibility and stop supporting old fitImage, so this
> >>>> is irrelevant here.
> >>>
> >>> OK, so please create a function which can detect an FDT header without
> >>> it being aligned, like the other code you wrote. Then it will be safe
> >>> to call that here, even if unaligned.
> >> But we actually do want to detect unaligned broken FDT header and either
> >> fix it up or stop processing, we don't want to perpetuate handling of
> >> broken FDTs and pretend that is OK, it shouldn't be I think. Hence this
> >> fixup.
> >
> > The decision as to whether something is an FDT is made a lot earlier
> > than the actual processing of it. For the former there is no need to
> > allocate and copy. For the latter we need to.
>
> The test whether the FDT is valid cannot be done earlier, this place is
> literally the first place when the FDT is decompressed/loaded to the
> target location.

Yes, this is the place where you are testing whether it is valid.

Later, the FDT is actually used.

It just isn't right to allocate memory in fit_image_load() - it is for
loading an image.

>
> > Anyway, you are doing the patches, so do what you think is best. But
> > please create a function for this, rather than lots of little
> > hand-crafted checks around the place.
> I am confused, function for what exactly ?

I believe the code you should patch is in boot_get_fdt_fit(), after
fit_image_load() returns. That is where the processing happens.

That is where the function I suggested (and tried to write) could be
called, to get an FDT that can be used. It also happens to be one of
the places where boot_relocate_fdt() is called, which could mean you
don't need to allocate memory.

Regards,
Simon

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] boot: Assure FDT is always at 8-byte aligned address
  2025-11-13 11:54 [PATCH] boot: Assure FDT is always at 8-byte aligned address Marek Vasut
  2025-11-13 19:33 ` Simon Glass
@ 2025-11-28 17:41 ` Tom Rini
  1 sibling, 0 replies; 11+ messages in thread
From: Tom Rini @ 2025-11-28 17:41 UTC (permalink / raw)
  To: u-boot, Marek Vasut
  Cc: Heinrich Schuchardt, Quentin Schulz, Simon Glass,
	Wolfgang Wallner

On Thu, 13 Nov 2025 12:54:51 +0100, Marek Vasut wrote:

> The fitImage may contain FDT at 4-byte aligned address, because alignment
> of DT tags is 4 bytes. However, libfdt and also Linux expects DT to be at
> 8-byte aligned address. Make sure that the DTs embedded in fitImages are
> always used from 8-byte aligned addresses. In case the DT is decompressed,
> make sure the target buffer is 8-byte aligned. In case the DT is only
> loaded, make sure the target buffer is 8-byte aligned too.
> 
> [...]

Applied to u-boot/next, thanks!

[1/1] boot: Assure FDT is always at 8-byte aligned address
      commit: 8fbcc0e0e839a8e25f636c76e59311033d3817b5
-- 
Tom



^ permalink raw reply	[flat|nested] 11+ messages in thread

end of thread, other threads:[~2025-11-28 17:41 UTC | newest]

Thread overview: 11+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-11-13 11:54 [PATCH] boot: Assure FDT is always at 8-byte aligned address Marek Vasut
2025-11-13 19:33 ` Simon Glass
2025-11-13 21:56   ` Marek Vasut
2025-11-13 22:46     ` Simon Glass
2025-11-15 17:19       ` Marek Vasut
2025-11-17 17:04         ` Simon Glass
2025-11-17 17:10           ` Marek Vasut
2025-11-18  3:47             ` Simon Glass
2025-11-19 21:29               ` Marek Vasut
2025-11-20  2:07                 ` Simon Glass
2025-11-28 17:41 ` Tom Rini

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox