[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 at 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 at 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 at 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