From: Simon Glass <sjg@google.com>
To: David Virag <virag.david003@gmail.com>
Cc: Tom Rini <trini@konsulko.com>, u-boot@lists.denx.de
Subject: Re: [BUG] fdt_pack_reg in common/fdt_support.c can cause crash from unaligned access
Date: Tue, 11 Jul 2023 13:13:23 -0600 [thread overview]
Message-ID: <CAPnjgZ3zWV-JZ=B9rXtS7+XsCxZ1ML8m2Xmz5byD79F9jb090Q@mail.gmail.com> (raw)
In-Reply-To: <8fa4eead45bf25f5fa4611c71e68d21a94b094a7.camel@gmail.com>
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
next prev parent reply other threads:[~2023-07-11 19:13 UTC|newest]
Thread overview: 7+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-07-09 21:42 [BUG] fdt_pack_reg in common/fdt_support.c can cause crash from unaligned access David Virag
2023-07-10 19:45 ` Simon Glass
2023-07-10 20:13 ` Tom Rini
2023-07-10 21:38 ` Simon Glass
2023-07-11 10:34 ` David Virag
2023-07-11 19:13 ` Simon Glass [this message]
2024-03-27 6:18 ` Sam Protsenko
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to='CAPnjgZ3zWV-JZ=B9rXtS7+XsCxZ1ML8m2Xmz5byD79F9jb090Q@mail.gmail.com' \
--to=sjg@google.com \
--cc=trini@konsulko.com \
--cc=u-boot@lists.denx.de \
--cc=virag.david003@gmail.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.