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

> Regards,
> Simon

Best regards,
David