[BUG] fdt_pack_reg in common/fdt_support.c can cause crash from unaligned access
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
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
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?
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
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
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
[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
participants (4)
-
David Virag -
Sam Protsenko -
Simon Glass -
Tom Rini