[PATCH] libfdt: Add FDT alignment check to fdt_check_header()

[PATCH] libfdt: Add FDT alignment check to fdt_check_header()

[Rob Herring]

Only checking the FDT alignment in fdt_ro_probe_() means that
fdt_check_header() can pass, but then subsequent API calls fail on
alignment checks. Let's add an alignment check to fdt_check_header() so
alignment errors are found up front.

Cc: Tom Rini <trini-OWPKS81ov/FWk0Htik3J/w@public.gmane.org>
Cc: Frank Rowand <frowand.list-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
Signed-off-by: Rob Herring <robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
---
For background, the new alignment check triggered a crash in the 
linux kernel. Yes, we should fix the error handling, but 
fdt_check_header() shouldn't tell us the FDT is valid only to fail 
later on.

Maybe we should move the check instead, but fdt_ro_probe_() and
fdt_check_header() already have a lot of the same checks.

 libfdt/fdt.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/libfdt/fdt.c b/libfdt/fdt.c
index 3e893073da05..9fe7cf4b747d 100644
--- a/libfdt/fdt.c
+++ b/libfdt/fdt.c
@@ -90,6 +90,10 @@ int fdt_check_header(const void *fdt)
 {
 	size_t hdrsize;
 
+	/* The device tree must be at an 8-byte aligned address */
+	if ((uintptr_t)fdt & 7)
+		return -FDT_ERR_ALIGNMENT;
+
 	if (fdt_magic(fdt) != FDT_MAGIC)
 		return -FDT_ERR_BADMAGIC;
 	if (!can_assume(LATEST)) {
-- 
2.27.0

Re: [PATCH] libfdt: Add FDT alignment check to fdt_check_header()

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 1644 bytes --]

On Tue, Apr 06, 2021 at 02:07:12PM -0500, Rob Herring wrote:
> Only checking the FDT alignment in fdt_ro_probe_() means that
> fdt_check_header() can pass, but then subsequent API calls fail on
> alignment checks. Let's add an alignment check to fdt_check_header() so
> alignment errors are found up front.
> 
> Cc: Tom Rini <trini-OWPKS81ov/FWk0Htik3J/w@public.gmane.org>
> Cc: Frank Rowand <frowand.list-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> Signed-off-by: Rob Herring <robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>

Good catch.

Applied, thanks.

> ---
> For background, the new alignment check triggered a crash in the 
> linux kernel. Yes, we should fix the error handling, but 
> fdt_check_header() shouldn't tell us the FDT is valid only to fail 
> later on.
> 
> Maybe we should move the check instead, but fdt_ro_probe_() and
> fdt_check_header() already have a lot of the same checks.
> 
>  libfdt/fdt.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/libfdt/fdt.c b/libfdt/fdt.c
> index 3e893073da05..9fe7cf4b747d 100644
> --- a/libfdt/fdt.c
> +++ b/libfdt/fdt.c
> @@ -90,6 +90,10 @@ int fdt_check_header(const void *fdt)
>  {
>  	size_t hdrsize;
>  
> +	/* The device tree must be at an 8-byte aligned address */
> +	if ((uintptr_t)fdt & 7)
> +		return -FDT_ERR_ALIGNMENT;
> +
>  	if (fdt_magic(fdt) != FDT_MAGIC)
>  		return -FDT_ERR_BADMAGIC;
>  	if (!can_assume(LATEST)) {

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH] libfdt: Add FDT alignment check to fdt_check_header()

[Simon Glass]

Hi Rob,

On Wed, 7 Apr 2021 at 07:07, Rob Herring <robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> wrote:
>
> Only checking the FDT alignment in fdt_ro_probe_() means that
> fdt_check_header() can pass, but then subsequent API calls fail on
> alignment checks. Let's add an alignment check to fdt_check_header() so
> alignment errors are found up front.
>
> Cc: Tom Rini <trini-OWPKS81ov/FWk0Htik3J/w@public.gmane.org>
> Cc: Frank Rowand <frowand.list-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> Signed-off-by: Rob Herring <robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
> ---
> For background, the new alignment check triggered a crash in the
> linux kernel. Yes, we should fix the error handling, but
> fdt_check_header() shouldn't tell us the FDT is valid only to fail
> later on.
>
> Maybe we should move the check instead, but fdt_ro_probe_() and
> fdt_check_header() already have a lot of the same checks.
>
>  libfdt/fdt.c | 4 ++++
>  1 file changed, 4 insertions(+)

At present U-Boot uses a 4-byte alignment, so far as I know, so this
will break things.

Is this because of the need to align the memory-reservation block?

>
> diff --git a/libfdt/fdt.c b/libfdt/fdt.c
> index 3e893073da05..9fe7cf4b747d 100644
> --- a/libfdt/fdt.c
> +++ b/libfdt/fdt.c
> @@ -90,6 +90,10 @@ int fdt_check_header(const void *fdt)
>  {
>         size_t hdrsize;
>
> +       /* The device tree must be at an 8-byte aligned address */
> +       if ((uintptr_t)fdt & 7)
> +               return -FDT_ERR_ALIGNMENT;
> +
>         if (fdt_magic(fdt) != FDT_MAGIC)
>                 return -FDT_ERR_BADMAGIC;
>         if (!can_assume(LATEST)) {
> --
> 2.27.0
>

Regards,
Simon

Re: [PATCH] libfdt: Add FDT alignment check to fdt_check_header()

[Rob Herring]

On Wed, Apr 7, 2021 at 10:35 AM Simon Glass <sjg-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org> wrote:
>
> Hi Rob,
>
> On Wed, 7 Apr 2021 at 07:07, Rob Herring <robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> wrote:
> >
> > Only checking the FDT alignment in fdt_ro_probe_() means that
> > fdt_check_header() can pass, but then subsequent API calls fail on
> > alignment checks. Let's add an alignment check to fdt_check_header() so
> > alignment errors are found up front.
> >
> > Cc: Tom Rini <trini-OWPKS81ov/FWk0Htik3J/w@public.gmane.org>
> > Cc: Frank Rowand <frowand.list-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> > Signed-off-by: Rob Herring <robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
> > ---
> > For background, the new alignment check triggered a crash in the
> > linux kernel. Yes, we should fix the error handling, but
> > fdt_check_header() shouldn't tell us the FDT is valid only to fail
> > later on.
> >
> > Maybe we should move the check instead, but fdt_ro_probe_() and
> > fdt_check_header() already have a lot of the same checks.
> >
> >  libfdt/fdt.c | 4 ++++
> >  1 file changed, 4 insertions(+)
>
> At present U-Boot uses a 4-byte alignment, so far as I know, so this
> will break things.

It was the u-boot folks that wanted this in the first place... Look at
the recent commits from Tom and the discussion on the list about them.

> Is this because of the need to align the memory-reservation block?

But yes, the spec does require some sections to be 8-byte aligned
which implies the whole thing has to be.

Rob

Re: [PATCH] libfdt: Add FDT alignment check to fdt_check_header()

[Tom Rini]

On Thu, Apr 08, 2021 at 03:35:35AM +1200, Simon Glass wrote:
> Hi Rob,
> 
> On Wed, 7 Apr 2021 at 07:07, Rob Herring <robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> wrote:
> >
> > Only checking the FDT alignment in fdt_ro_probe_() means that
> > fdt_check_header() can pass, but then subsequent API calls fail on
> > alignment checks. Let's add an alignment check to fdt_check_header() so
> > alignment errors are found up front.
> >
> > Cc: Tom Rini <trini-OWPKS81ov/FWk0Htik3J/w@public.gmane.org>
> > Cc: Frank Rowand <frowand.list-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> > Signed-off-by: Rob Herring <robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
> > ---
> > For background, the new alignment check triggered a crash in the
> > linux kernel. Yes, we should fix the error handling, but
> > fdt_check_header() shouldn't tell us the FDT is valid only to fail
> > later on.
> >
> > Maybe we should move the check instead, but fdt_ro_probe_() and
> > fdt_check_header() already have a lot of the same checks.
> >
> >  libfdt/fdt.c | 4 ++++
> >  1 file changed, 4 insertions(+)
> 
> At present U-Boot uses a 4-byte alignment, so far as I know, so this
> will break things.

It's 8 byte, not 4 byte and I have nothing good to say about places that
get by with 4-and-not-8 alignment.

-- 
Tom

Re: [PATCH] libfdt: Add FDT alignment check to fdt_check_header()

[Simon Glass]

Hi Rob,

On Thu, 8 Apr 2021 at 05:26, Rob Herring <robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> wrote:
>
> On Wed, Apr 7, 2021 at 10:35 AM Simon Glass <sjg-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org> wrote:
> >
> > Hi Rob,
> >
> > On Wed, 7 Apr 2021 at 07:07, Rob Herring <robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> wrote:
> > >
> > > Only checking the FDT alignment in fdt_ro_probe_() means that
> > > fdt_check_header() can pass, but then subsequent API calls fail on
> > > alignment checks. Let's add an alignment check to fdt_check_header() so
> > > alignment errors are found up front.
> > >
> > > Cc: Tom Rini <trini-OWPKS81ov/FWk0Htik3J/w@public.gmane.org>
> > > Cc: Frank Rowand <frowand.list-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> > > Signed-off-by: Rob Herring <robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
> > > ---
> > > For background, the new alignment check triggered a crash in the
> > > linux kernel. Yes, we should fix the error handling, but
> > > fdt_check_header() shouldn't tell us the FDT is valid only to fail
> > > later on.
> > >
> > > Maybe we should move the check instead, but fdt_ro_probe_() and
> > > fdt_check_header() already have a lot of the same checks.
> > >
> > >  libfdt/fdt.c | 4 ++++
> > >  1 file changed, 4 insertions(+)
> >
> > At present U-Boot uses a 4-byte alignment, so far as I know, so this
> > will break things.
>
> It was the u-boot folks that wanted this in the first place... Look at
> the recent commits from Tom and the discussion on the list about them.

OK I guess I just missed that. I recall the push-back against
supporting unaligned access but not the 8-byte stuff.

>
> > Is this because of the need to align the memory-reservation block?
>
> But yes, the spec does require some sections to be 8-byte aligned
> which implies the whole thing has to be.

I was looking at that but from what I could tell it is not stated
anywhere. In fact it is, but I missed it.

I sent:

https://github.com/devicetree-org/devicetree-specification/pull/43

Regards,
Simon

Re: [PATCH] libfdt: Add FDT alignment check to fdt_check_header()

[Simon Glass]

Hi Tom & Rob,

On Thu, 8 Apr 2021 at 06:09, Tom Rini <trini-OWPKS81ov/FWk0Htik3J/w@public.gmane.org> wrote:
>
> On Thu, Apr 08, 2021 at 03:35:35AM +1200, Simon Glass wrote:
> > Hi Rob,
> >
> > On Wed, 7 Apr 2021 at 07:07, Rob Herring <robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> wrote:
> > >
> > > Only checking the FDT alignment in fdt_ro_probe_() means that
> > > fdt_check_header() can pass, but then subsequent API calls fail on
> > > alignment checks. Let's add an alignment check to fdt_check_header() so
> > > alignment errors are found up front.
> > >
> > > Cc: Tom Rini <trini-OWPKS81ov/FWk0Htik3J/w@public.gmane.org>
> > > Cc: Frank Rowand <frowand.list-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> > > Signed-off-by: Rob Herring <robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
> > > ---
> > > For background, the new alignment check triggered a crash in the
> > > linux kernel. Yes, we should fix the error handling, but
> > > fdt_check_header() shouldn't tell us the FDT is valid only to fail
> > > later on.
> > >
> > > Maybe we should move the check instead, but fdt_ro_probe_() and
> > > fdt_check_header() already have a lot of the same checks.
> > >
> > >  libfdt/fdt.c | 4 ++++
> > >  1 file changed, 4 insertions(+)
> >
> > At present U-Boot uses a 4-byte alignment, so far as I know, so this
> > will break things.
>
> It's 8 byte, not 4 byte and I have nothing good to say about places that
> get by with 4-and-not-8 alignment.

I am thinking of this in arch/arm/cpu/u-boot.lds :

. = ALIGN(4);

.image_copy_end : {

At least for now, we use 4-byte alignment on 32-bit ARM, for example.

Regards,
Simon

Re: [PATCH] libfdt: Add FDT alignment check to fdt_check_header()

[Tom Rini]

On Thu, Apr 08, 2021 at 06:36:53AM +1200, Simon Glass wrote:
> Hi Tom & Rob,
> 
> On Thu, 8 Apr 2021 at 06:09, Tom Rini <trini-OWPKS81ov/FWk0Htik3J/w@public.gmane.org> wrote:
> >
> > On Thu, Apr 08, 2021 at 03:35:35AM +1200, Simon Glass wrote:
> > > Hi Rob,
> > >
> > > On Wed, 7 Apr 2021 at 07:07, Rob Herring <robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> wrote:
> > > >
> > > > Only checking the FDT alignment in fdt_ro_probe_() means that
> > > > fdt_check_header() can pass, but then subsequent API calls fail on
> > > > alignment checks. Let's add an alignment check to fdt_check_header() so
> > > > alignment errors are found up front.
> > > >
> > > > Cc: Tom Rini <trini-OWPKS81ov/FWk0Htik3J/w@public.gmane.org>
> > > > Cc: Frank Rowand <frowand.list-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> > > > Signed-off-by: Rob Herring <robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
> > > > ---
> > > > For background, the new alignment check triggered a crash in the
> > > > linux kernel. Yes, we should fix the error handling, but
> > > > fdt_check_header() shouldn't tell us the FDT is valid only to fail
> > > > later on.
> > > >
> > > > Maybe we should move the check instead, but fdt_ro_probe_() and
> > > > fdt_check_header() already have a lot of the same checks.
> > > >
> > > >  libfdt/fdt.c | 4 ++++
> > > >  1 file changed, 4 insertions(+)
> > >
> > > At present U-Boot uses a 4-byte alignment, so far as I know, so this
> > > will break things.
> >
> > It's 8 byte, not 4 byte and I have nothing good to say about places that
> > get by with 4-and-not-8 alignment.
> 
> I am thinking of this in arch/arm/cpu/u-boot.lds :
> 
> . = ALIGN(4);
> 
> .image_copy_end : {
> 
> At least for now, we use 4-byte alignment on 32-bit ARM, for example.

Lets move that over to the U-Boot list and see what needs whacking where
then, keeping in mind that it's where we use the DTB once it's in memory
that needs 8 byte alignment, not where it might be placed in a blob for
storage.

-- 
Tom