[BUG] fdt_pack_reg in common/fdt_support.c can cause crash from unaligned access

[BUG] fdt_pack_reg in common/fdt_support.c can cause crash from unaligned access

[David Virag]

Hi,

I'm trying to port U-Boot to a new board (Samsung JACKPOTLTE, ARMv8,
Exynos7885) but when CONFIG_ARCH_FIXUP_FDT_MEMORY is enabled, the bootm
command leads to an unaligned memory access, which results in a
synchronous abort.

After a long debugging session, I concluded that fdt_pack_reg in
common/fdt_support.c writes to unaligned addresses in its for loop.
In the case of address_cells being 2, and size_cells being 1, the
buffer pointer gets incremented by 12 in each loop, making the second
iteration (i=1) write a 64bit value to a non 64bit aligned address.

Turning the alignment check enable bit (A) off in SCTLR makes the
function work as intended. I couldn't find code that touches this bit,
but I may have missed something. I don't think writing in two parts
should be the fix, but something should be done about this. As far as I
understand, any arm64 board that has this bit turned on, either from
previous code or just the initial status of the bit after power on,
could crash here.

This is on top of the latest commit as of now
(0beb649053b86b2cfd5cf55a0fc68bc2fe91a430)

What should be done here?

Best regards,
David

Re: [BUG] fdt_pack_reg in common/fdt_support.c can cause crash from unaligned access

[Simon Glass]

Hi David,

On Sun, 9 Jul 2023 at 19:11, David Virag <virag.david003@gmail.com> wrote:
>
> Hi,
>
> I'm trying to port U-Boot to a new board (Samsung JACKPOTLTE, ARMv8,
> Exynos7885) but when CONFIG_ARCH_FIXUP_FDT_MEMORY is enabled, the bootm
> command leads to an unaligned memory access, which results in a
> synchronous abort.
>
> After a long debugging session, I concluded that fdt_pack_reg in
> common/fdt_support.c writes to unaligned addresses in its for loop.
> In the case of address_cells being 2, and size_cells being 1, the
> buffer pointer gets incremented by 12 in each loop, making the second
> iteration (i=1) write a 64bit value to a non 64bit aligned address.
>
> Turning the alignment check enable bit (A) off in SCTLR makes the
> function work as intended. I couldn't find code that touches this bit,
> but I may have missed something. I don't think writing in two parts
> should be the fix, but something should be done about this. As far as I
> understand, any arm64 board that has this bit turned on, either from
> previous code or just the initial status of the bit after power on,
> could crash here.
>
> This is on top of the latest commit as of now
> (0beb649053b86b2cfd5cf55a0fc68bc2fe91a430)
>
> What should be done here?

+Tom Rini

>
> Best regards,
> David

Re: [BUG] fdt_pack_reg in common/fdt_support.c can cause crash from unaligned access

[Tom Rini]

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

On Mon, Jul 10, 2023 at 01:45:46PM -0600, Simon Glass wrote:
> Hi David,
> 
> On Sun, 9 Jul 2023 at 19:11, David Virag <virag.david003@gmail.com> wrote:
> >
> > Hi,
> >
> > I'm trying to port U-Boot to a new board (Samsung JACKPOTLTE, ARMv8,
> > Exynos7885) but when CONFIG_ARCH_FIXUP_FDT_MEMORY is enabled, the bootm
> > command leads to an unaligned memory access, which results in a
> > synchronous abort.
> >
> > After a long debugging session, I concluded that fdt_pack_reg in
> > common/fdt_support.c writes to unaligned addresses in its for loop.
> > In the case of address_cells being 2, and size_cells being 1, the
> > buffer pointer gets incremented by 12 in each loop, making the second
> > iteration (i=1) write a 64bit value to a non 64bit aligned address.
> >
> > Turning the alignment check enable bit (A) off in SCTLR makes the
> > function work as intended. I couldn't find code that touches this bit,
> > but I may have missed something. I don't think writing in two parts
> > should be the fix, but something should be done about this. As far as I
> > understand, any arm64 board that has this bit turned on, either from
> > previous code or just the initial status of the bit after power on,
> > could crash here.
> >
> > This is on top of the latest commit as of now
> > (0beb649053b86b2cfd5cf55a0fc68bc2fe91a430)
> >
> > What should be done here?
> 
> +Tom Rini

... I was hoping you had an idea Simon. Is this part of the code we
share with libfdt itself, or one of the helpers we made?

-- 
Tom

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

Re: [BUG] fdt_pack_reg in common/fdt_support.c can cause crash from unaligned access

[Simon Glass]

Hi,

On Mon, 10 Jul 2023 at 14:13, Tom Rini <trini@konsulko.com> wrote:
>
> On Mon, Jul 10, 2023 at 01:45:46PM -0600, Simon Glass wrote:
> > Hi David,
> >
> > On Sun, 9 Jul 2023 at 19:11, David Virag <virag.david003@gmail.com> wrote:
> > >
> > > Hi,
> > >
> > > I'm trying to port U-Boot to a new board (Samsung JACKPOTLTE, ARMv8,
> > > Exynos7885) but when CONFIG_ARCH_FIXUP_FDT_MEMORY is enabled, the bootm
> > > command leads to an unaligned memory access, which results in a
> > > synchronous abort.
> > >
> > > After a long debugging session, I concluded that fdt_pack_reg in
> > > common/fdt_support.c writes to unaligned addresses in its for loop.
> > > In the case of address_cells being 2, and size_cells being 1, the
> > > buffer pointer gets incremented by 12 in each loop, making the second
> > > iteration (i=1) write a 64bit value to a non 64bit aligned address.
> > >
> > > Turning the alignment check enable bit (A) off in SCTLR makes the
> > > function work as intended. I couldn't find code that touches this bit,
> > > but I may have missed something. I don't think writing in two parts
> > > should be the fix, but something should be done about this. As far as I
> > > understand, any arm64 board that has this bit turned on, either from
> > > previous code or just the initial status of the bit after power on,
> > > could crash here.
> > >
> > > This is on top of the latest commit as of now
> > > (0beb649053b86b2cfd5cf55a0fc68bc2fe91a430)
> > >
> > > What should be done here?
> >
> > +Tom Rini
>
> ... I was hoping you had an idea Simon. Is this part of the code we
> share with libfdt itself, or one of the helpers we made?

Hmmm, is the DT itself 64-bit aligned? It needs to be.

Looking at fdt_find_separate() it needs _end

Looking at arch/arm/cpu/armv8/u-boot.lds I don't see an ALIGN before _end.

So perhaps that is the problem?

Regards,
Simon

Re: [BUG] fdt_pack_reg in common/fdt_support.c can cause crash from unaligned access

[David Virag]

On Mon, 2023-07-10 at 15:38 -0600, Simon Glass wrote:
> Hi,
> 
> On Mon, 10 Jul 2023 at 14:13, Tom Rini <trini@konsulko.com> wrote:
> > 
> > On Mon, Jul 10, 2023 at 01:45:46PM -0600, Simon Glass wrote:
> > > Hi David,
> > > 
> > > On Sun, 9 Jul 2023 at 19:11, David Virag
> > > <virag.david003@gmail.com> wrote:
> > > > 
> > > > Hi,
> > > > 
> > > > I'm trying to port U-Boot to a new board (Samsung JACKPOTLTE,
> > > > ARMv8,
> > > > Exynos7885) but when CONFIG_ARCH_FIXUP_FDT_MEMORY is enabled,
> > > > the bootm
> > > > command leads to an unaligned memory access, which results in a
> > > > synchronous abort.
> > > > 
> > > > After a long debugging session, I concluded that fdt_pack_reg
> > > > in
> > > > common/fdt_support.c writes to unaligned addresses in its for
> > > > loop.
> > > > In the case of address_cells being 2, and size_cells being 1,
> > > > the
> > > > buffer pointer gets incremented by 12 in each loop, making the
> > > > second
> > > > iteration (i=1) write a 64bit value to a non 64bit aligned
> > > > address.
> > > > 
> > > > Turning the alignment check enable bit (A) off in SCTLR makes
> > > > the
> > > > function work as intended. I couldn't find code that touches
> > > > this bit,
> > > > but I may have missed something. I don't think writing in two
> > > > parts
> > > > should be the fix, but something should be done about this. As
> > > > far as I
> > > > understand, any arm64 board that has this bit turned on, either
> > > > from
> > > > previous code or just the initial status of the bit after power
> > > > on,
> > > > could crash here.
> > > > 
> > > > This is on top of the latest commit as of now
> > > > (0beb649053b86b2cfd5cf55a0fc68bc2fe91a430)
> > > > 
> > > > What should be done here?
> > > 
> > > +Tom Rini
> > 
> > ... I was hoping you had an idea Simon. Is this part of the code we
> > share with libfdt itself, or one of the helpers we made?

Since the offending code is in common/fdt_support.c and not somewhere
in lib/libfdt, I'd say it's one of the helpers.

> 
> Hmmm, is the DT itself 64-bit aligned? It needs to be.

It should be. I forgot to mention, but I'm loading the DT itself from
the FIT image to address 0x82000000. I'm trying to boot a Linux kernel
with it.

According to uart output, working FDT gets set to 0x82000000, then for
some reason gets loaded to 0x8f908000. Both are aligned, so this
shouldn't be a problem.

Here is the uart output, maybe there's something interesting in it
(probably should've provided it earlier):

## Loading fdt from FIT Image at 89000000 ...
   Using 'alarm' configuration
   Trying 'fdt' fdt subimage
     Description:  DTB
     Type:         Flat Device Tree
     Compression:  uncompressed
     Data Start:   0x8a5be9a0
     Data Size:    21936 Bytes = 21.4 KiB
     Architecture: AArch64
     Load Address: 0x82000000
     Hash algo:    sha1
     Hash value:   b289ae4aab26ae514e06ca76b4ad8f3f9c6d6cab
   Verifying Hash Integrity ... sha1+ OK
   Loading fdt from 0x8a5be9a0 to 0x82000000
   Booting using the fdt blob at 0x82000000
Working FDT set to 82000000
   Loading Kernel Image
   Loading Ramdisk to 8f911000, end 8ffff885 ... OK
   Loading Device Tree to 000000008f908000, end 000000008f9105af ... OK
Working FDT set to 8f908000
"Synchronous Abort" handler, esr 0x96000061, far 0xffb4d28c
elr: 000000000001aec8 lr : 000000000001ae70 (reloc)
elr: 00000000fff88ec8 lr : 00000000fff88e70
x0 : 0000000000000001 x1 : 0000000000000001
x2 : 0000009000000000 x3 : 0000000090000000
x4 : 000000000000000c x5 : 0000000000000008
x6 : 0000000000000008 x7 : 000000008f908000
x8 : 000000000000004c x9 : 00000000ffb4d0fc
x10: 0000000000000003 x11: 0000000000004e78
x12: 00000000ffb4d1f8 x13: 000000008f908000
x14: 00000000ffffffff x15: 00000000ffb4d448
x16: 0000000000000000 x17: 0000000000000004
x18: 00000000ffb4ecb0 x19: 00000000ffb4d28c
x20: 0000000000000010 x21: 0000000000000002
x22: 00000000ffb4d390 x23: 00000000ffb4d410
x24: 000000008f908000 x25: 00000000fffcbbc9
x26: 00000000fff70834 x27: 0000000000000000
x28: 0000000000000000 x29: 00000000ffb4d1f0

Code: b8666ac2 71000abf 54000181 dac00c62 (f9000262)
Resetting CPU ...

resetting ...


From u-boot.map, addresses 000000000001aec8 in elr and 000000000001ae70
in lr should be the fdt_pack_reg function (000000000001ae30).

The thing is that the function above does access data unaligned,
since... well, the data is not always aligned there. With 64bit address
cells and 32bit size cells, even if the first reads are aligned, it
will shift in and out of being 32 bits off from aligned.

We read 64 bits, increment the ptr by 64 bit, read 32 bits, increment
by 32 bits, and read 64 bits again, which is 96 bits from the start.

> 
> Looking at fdt_find_separate() it needs _end
> 
> Looking at arch/arm/cpu/armv8/u-boot.lds I don't see an ALIGN before
> _end.
> 
> So perhaps that is the problem?
> 

I don't know about this, but it's not related to the problem I'm
running into.

> Regards,
> Simon

Best regards,
David

Re: [BUG] fdt_pack_reg in common/fdt_support.c can cause crash from unaligned access

[Simon Glass]

Hi David,

On Tue, 11 Jul 2023 at 04:34, David Virag <virag.david003@gmail.com> wrote:
>
> On Mon, 2023-07-10 at 15:38 -0600, Simon Glass wrote:
> > Hi,
> >
> > On Mon, 10 Jul 2023 at 14:13, Tom Rini <trini@konsulko.com> wrote:
> > >
> > > On Mon, Jul 10, 2023 at 01:45:46PM -0600, Simon Glass wrote:
> > > > Hi David,
> > > >
> > > > On Sun, 9 Jul 2023 at 19:11, David Virag
> > > > <virag.david003@gmail.com> wrote:
> > > > >
> > > > > Hi,
> > > > >
> > > > > I'm trying to port U-Boot to a new board (Samsung JACKPOTLTE,
> > > > > ARMv8,
> > > > > Exynos7885) but when CONFIG_ARCH_FIXUP_FDT_MEMORY is enabled,
> > > > > the bootm
> > > > > command leads to an unaligned memory access, which results in a
> > > > > synchronous abort.
> > > > >
> > > > > After a long debugging session, I concluded that fdt_pack_reg
> > > > > in
> > > > > common/fdt_support.c writes to unaligned addresses in its for
> > > > > loop.
> > > > > In the case of address_cells being 2, and size_cells being 1,
> > > > > the
> > > > > buffer pointer gets incremented by 12 in each loop, making the
> > > > > second
> > > > > iteration (i=1) write a 64bit value to a non 64bit aligned
> > > > > address.
> > > > >
> > > > > Turning the alignment check enable bit (A) off in SCTLR makes
> > > > > the
> > > > > function work as intended. I couldn't find code that touches
> > > > > this bit,
> > > > > but I may have missed something. I don't think writing in two
> > > > > parts
> > > > > should be the fix, but something should be done about this. As
> > > > > far as I
> > > > > understand, any arm64 board that has this bit turned on, either
> > > > > from
> > > > > previous code or just the initial status of the bit after power
> > > > > on,
> > > > > could crash here.
> > > > >
> > > > > This is on top of the latest commit as of now
> > > > > (0beb649053b86b2cfd5cf55a0fc68bc2fe91a430)
> > > > >
> > > > > What should be done here?
> > > >
> > > > +Tom Rini
> > >
> > > ... I was hoping you had an idea Simon. Is this part of the code we
> > > share with libfdt itself, or one of the helpers we made?
>
> Since the offending code is in common/fdt_support.c and not somewhere
> in lib/libfdt, I'd say it's one of the helpers.
>
> >
> > Hmmm, is the DT itself 64-bit aligned? It needs to be.
>
> It should be. I forgot to mention, but I'm loading the DT itself from
> the FIT image to address 0x82000000. I'm trying to boot a Linux kernel
> with it.
>
> According to uart output, working FDT gets set to 0x82000000, then for
> some reason gets loaded to 0x8f908000. Both are aligned, so this
> shouldn't be a problem.
>
> Here is the uart output, maybe there's something interesting in it
> (probably should've provided it earlier):
>
> ## Loading fdt from FIT Image at 89000000 ...
>    Using 'alarm' configuration
>    Trying 'fdt' fdt subimage
>      Description:  DTB
>      Type:         Flat Device Tree
>      Compression:  uncompressed
>      Data Start:   0x8a5be9a0
>      Data Size:    21936 Bytes = 21.4 KiB
>      Architecture: AArch64
>      Load Address: 0x82000000
>      Hash algo:    sha1
>      Hash value:   b289ae4aab26ae514e06ca76b4ad8f3f9c6d6cab
>    Verifying Hash Integrity ... sha1+ OK
>    Loading fdt from 0x8a5be9a0 to 0x82000000
>    Booting using the fdt blob at 0x82000000
> Working FDT set to 82000000
>    Loading Kernel Image
>    Loading Ramdisk to 8f911000, end 8ffff885 ... OK
>    Loading Device Tree to 000000008f908000, end 000000008f9105af ... OK
> Working FDT set to 8f908000
> "Synchronous Abort" handler, esr 0x96000061, far 0xffb4d28c
> elr: 000000000001aec8 lr : 000000000001ae70 (reloc)
> elr: 00000000fff88ec8 lr : 00000000fff88e70
> x0 : 0000000000000001 x1 : 0000000000000001
> x2 : 0000009000000000 x3 : 0000000090000000
> x4 : 000000000000000c x5 : 0000000000000008
> x6 : 0000000000000008 x7 : 000000008f908000
> x8 : 000000000000004c x9 : 00000000ffb4d0fc
> x10: 0000000000000003 x11: 0000000000004e78
> x12: 00000000ffb4d1f8 x13: 000000008f908000
> x14: 00000000ffffffff x15: 00000000ffb4d448
> x16: 0000000000000000 x17: 0000000000000004
> x18: 00000000ffb4ecb0 x19: 00000000ffb4d28c
> x20: 0000000000000010 x21: 0000000000000002
> x22: 00000000ffb4d390 x23: 00000000ffb4d410
> x24: 000000008f908000 x25: 00000000fffcbbc9
> x26: 00000000fff70834 x27: 0000000000000000
> x28: 0000000000000000 x29: 00000000ffb4d1f0
>
> Code: b8666ac2 71000abf 54000181 dac00c62 (f9000262)
> Resetting CPU ...
>
> resetting ...
>
>
> From u-boot.map, addresses 000000000001aec8 in elr and 000000000001ae70
> in lr should be the fdt_pack_reg function (000000000001ae30).
>
> The thing is that the function above does access data unaligned,
> since... well, the data is not always aligned there. With 64bit address
> cells and 32bit size cells, even if the first reads are aligned, it
> will shift in and out of being 32 bits off from aligned.
>
> We read 64 bits, increment the ptr by 64 bit, read 32 bits, increment
> by 32 bits, and read 64 bits again, which is 96 bits from the start.
>
> >
> > Looking at fdt_find_separate() it needs _end
> >
> > Looking at arch/arm/cpu/armv8/u-boot.lds I don't see an ALIGN before
> > _end.
> >
> > So perhaps that is the problem?
> >
>
> I don't know about this, but it's not related to the problem I'm
> running into.

Thinking about this a bit more, there is no requirement that FDT
properties start on an 64-bit byte boundary. The requirement for
32-bit.

So I suspect the answer might be that we have a problem here, on ARM.

One solution might be to add a helper like put_unaligned_be64() which
uses a CONFIG to indicate whether 32-bit-aligned 64-bit read/write is
supported, and either just does the write, or calls
put_unaligned_be64().

Another option might be to adjust fdt_pack_reg() to write the cells
one at a time.

Regards,
Simon

Re: [BUG] fdt_pack_reg in common/fdt_support.c can cause crash from unaligned access

[Sam Protsenko]

[snip]

>
> So I suspect the answer might be that we have a problem here, on ARM.
>
> One solution might be to add a helper like put_unaligned_be64() which
> uses a CONFIG to indicate whether 32-bit-aligned 64-bit read/write is
> supported, and either just does the write, or calls
> put_unaligned_be64().
>
> Another option might be to adjust fdt_pack_reg() to write the cells
> one at a time.
>

Hi Simon, David,

Just stumbled upon the same issue on E850-96 board when trying to boot
the kernel using `bootm' command. It's an ARM64 board (Exynos850 SoC).
Did you by chance find any solution to this? Looks to me it actually
needs some sort of fix. I wonder why more people don't see this error
on other ARM64 board. Anyways, would be happy to help out fixing it,
so please let me know if you have any advice on this.

Thanks!

> Regards,
> Simon