Bug #281
opencbfs stage loading||encoding is not endian safe.
0%
Description
https://github.com/hanetzer/coreboot/tree/power9-endian-demo
^ code in question.
Build a cross-pp64 toolchain, switch mainboards to qemu-powernv9.
Use https://github.com/hanetzer/qemu/tree/powernv-5.2 for qemu
(its the same as https://github.com/legoater/qemu/tree/powernv-5.2
with a commit to enable mapping non-pnor spi flash memory).
Execute something like:
qemu-system-ppc64 -cpu power9 -M powernv,hb-mode=true -m 1G -s -S \
-chardev socket,id=qemu-monitor,host=localhost,port=7777,server,nowait,telnet \
-mon qemu-monitor,mode=readline -nographic -L ./build/ \
-bios ./build/cbfs/fallback/bootblock.bin \
-drive file=./build/coreboot.rom,format=raw,if=mtd
Attach gdb and break at src/lib/cbfs.c:299, use gdb to print
fsize and stage.len; assuming you build a big-endian coreboot-ppc64
(the default) you'll end up with an fsize of 0x5c32_0000 and a stage.len
of 0x0000_325c or so.
Updated by Marty Plummer over 3 years ago
This is also an issue if you build a big-endian coreboot
on a big-endian system (in my case a talos ii running be
gentoo), so something is definitely up.
Updated by Aaron Durbin over 3 years ago
fsize is derived from the region_device size for the data in cbfs_for_each_file():
file.len = read_be32(&file.len);
file.offset = read_be32(&file.offset);
Those fields are expected to be big endian. There's a comment in cbfs_prog_stage_load() where the stage fields are in cbfstool host byte order, but the fields above are from struct cbfs_file.
Given that it would seem cbfs_file.len is written in the wrong endianness from cbfstool. Do you have a hexdump of the cbfs image for the offset of the affected file?
Updated by Marty Plummer over 3 years ago
Aaron Durbin wrote:
fsize is derived from the region_device size for the data in cbfs_for_each_file():
file.len = read_be32(&file.len); file.offset = read_be32(&file.offset);Those fields are expected to be big endian. There's a comment in cbfs_prog_stage_load() where the stage fields are in cbfstool host byte order, but the fields above are from struct cbfs_file.
Given that it would seem cbfs_file.len is written in the wrong endianness from cbfstool. Do you have a hexdump of the cbfs image for the offset of the affected file?
00020000 5f 5f 46 4d 41 50 5f 5f 01 01 00 00 00 00 00 00 |__FMAP__........|
00020010 00 00 00 00 00 04 46 4c 41 53 48 00 00 00 00 00 |......FLASH.....|
00020020 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 |................|
00020030 00 00 00 00 00 00 04 00 00 00 00 00 00 00 00 04 |................|
00020040 42 49 4f 53 00 00 00 00 00 00 00 00 00 00 00 00 |BIOS............|
00020050 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 |................|
00020060 00 00 00 00 00 00 00 00 02 00 42 4f 4f 54 42 4c |..........BOOTBL|
00020070 4f 43 4b 00 00 00 00 00 00 00 00 00 00 00 00 00 |OCK.............|
00020080 00 00 00 00 00 00 00 00 00 00 00 00 00 00 02 00 |................|
00020090 00 02 00 00 46 4d 41 50 00 00 00 00 00 00 00 00 |....FMAP........|
000200a0 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 |................|
000200b0 00 00 00 00 00 00 00 02 02 00 00 fe fd 03 43 4f |..............CO|
000200c0 52 45 42 4f 4f 54 00 00 00 00 00 00 00 00 00 00 |REBOOT..........|
000200d0 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 |................|
000200e0 ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff |................|
*
00020200 4c 41 52 43 48 49 56 45 00 00 00 20 00 00 00 02 |LARCHIVE... ....|
00020210 00 00 00 00 00 00 00 38 63 62 66 73 20 6d 61 73 |.......8cbfs mas|
00020220 74 65 72 20 68 65 61 64 65 72 00 00 00 00 00 00 |ter header......|
00020230 00 00 00 00 00 00 00 00 4f 52 42 43 31 31 31 32 |........ORBC1112|
00020240 04 00 00 00 00 00 00 04 00 00 00 40 00 02 02 00 |...........@....|
00020250 ff ff ff ff 00 00 00 00 ff ff ff ff ff ff ff ff |................|
00020260 ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff |................|
*
00020280 4c 41 52 43 48 49 56 45 00 00 31 58 00 00 00 10 |LARCHIVE..1X....|
00020290 00 00 00 00 00 00 00 38 66 61 6c 6c 62 61 63 6b |.......8fallback|
000202a0 2f 72 6f 6d 73 74 61 67 65 00 00 00 00 00 00 00 |/romstage.......|
000202b0 00 00 00 00 00 00 00 00 02 00 00 00 00 00 02 00 |................|
000202c0 00 00 00 00 00 00 01 00 00 00 00 00 3c 31 00 00 |............<1..|
000202d0 c8 88 01 00 04 22 4d 18 60 50 fb 2d 31 00 00 1f |....."M.`P.-1...|
000202e0 00 01 00 ff ff ff ff ff ff ff ff ff ff ff ff ff |................|
000202f0 ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff |................|
*
00023440 4c 41 52 43 48 49 56 45 00 00 4d ed 00 00 00 10 |LARCHIVE..M.....|
00023450 00 00 00 00 00 00 00 38 66 61 6c 6c 62 61 63 6b |.......8fallback|
00023460 2f 72 61 6d 73 74 61 67 65 00 00 00 00 00 00 00 |/ramstage.......|
00023470 00 00 00 00 00 00 00 00 01 00 00 00 00 00 10 00 |................|
seems to be the pertenant parts you want. At src/lib/cbfs.c:299
the values are as follows:
fsize: 0x0000_313c
stage.len: 0x3c31_0000
Updated by Aaron Durbin over 3 years ago
Original sighting:
"fsize of 0x5c32_0000 and a stage.len of 0x0000_325c"
Update #3:
"fsize: 0x0000_313c stage.len: 0x3c31_0000"
Those are in conflict. From the hexdump I can see cbfs_file is correct for romstage. And given update #3 where stage.len is different.
struct cbfs_stage starts at 0x202b8 for fallback/romstage file.
000202b0 00 00 00 00 00 00 00 00 02 00 00 00 00 00 02 00 |................|
000202c0 00 00 00 00 00 00 01 00 00 00 00 00 3c 31 00 00 |............<1..|
Last field (at 0x202cc) is the memlen which is clearly in little endian format.
Can you please provide the following for your cbfstool?
$ readelf -h $(which cbfstool)
cbfs stage file is assumed to be in target endian format: https://review.coreboot.org/plugins/gitiles/coreboot/+/refs/heads/master/src/lib/cbfs.c#301
Updated by Marty Plummer over 3 years ago
Aaron Durbin wrote:
Original sighting:
"fsize of 0x5c32_0000 and a stage.len of 0x0000_325c"Update #3:
"fsize: 0x0000_313c stage.len: 0x3c31_0000"Those are in conflict. From the hexdump I can see cbfs_file is correct for romstage. And given update #3 where stage.len is different.
I had to recompile; I've been building little-endian for the moment because otherwise I'd be stuck here.
struct cbfs_stage starts at 0x202b8 for fallback/romstage file.
000202b0 00 00 00 00 00 00 00 00 02 00 00 00 00 00 02 00 |................| 000202c0 00 00 00 00 00 00 01 00 00 00 00 00 3c 31 00 00 |............<1..|Last field (at 0x202cc) is the memlen which is clearly in little endian format.
Can you please provide the following for your cbfstool?
$ readelf -h $(which cbfstool)
ELF Header:
Magic: 7f 45 4c 46 02 01 01 00 00 00 00 00 00 00 00 00
Class: ELF64
Data: 2's complement, little endian
Version: 1 (current)
OS/ABI: UNIX - System V
ABI Version: 0
Type: DYN (Shared object file)
Machine: Advanced Micro Devices X86-64
Version: 0x1
Entry point address: 0x4100
Start of program headers: 64 (bytes into file)
Start of section headers: 1838744 (bytes into file)
Flags: 0x0
Size of this header: 64 (bytes)
Size of program headers: 56 (bytes)
Number of program headers: 11
Size of section headers: 64 (bytes)
Number of section headers: 37
Section header string table index: 36
cbfs stage file is assumed to be in target endian format: https://review.coreboot.org/plugins/gitiles/coreboot/+/refs/heads/master/src/lib/cbfs.c#301
Updated by Aaron Durbin over 3 years ago
Thanks for the info. In the end it doesn't matter...
`
-------/* N.B. The original plan was that SELF data was B.E.
------- * but: this is all L.E.
------- * Maybe we should just change the spec.
------- */
-------xdr_le.put32(outheader, algo);
-------xdr_le.put64(outheader, entry);
-------xdr_le.put64(outheader, loadaddr);
-------xdr_le.put32(outheader, filesize);
-------xdr_le.put32(outheader, memsize);
`
Given that cbfs_stage being written in little endian unconditionally we should probably fix the code then that I linked to in #4.
Can you please try this patch?
diff --git a/src/lib/cbfs.c b/src/lib/cbfs.c
index cb66f81d99..29d79cd9d0 100644
--- a/src/lib/cbfs.c
+++ b/src/lib/cbfs.c
@@ -4,8 +4,8 @@
#include <boot_device.h>
#include <cbfs.h>
#include <commonlib/bsd/compression.h>
+#include <commonlib/endian.h>
#include <console/console.h>
-#include <endian.h>
#include <fmap.h>
#include <lib.h>
#include <security/tpm/tspi/crtm.h>
@@ -296,6 +296,12 @@ int cbfs_prog_stage_load(struct prog *pstage)
foffset = 0;
foffset += sizeof(stage);
+ stage.compression = read_le32(&stage.compression);
+ stage.entry = read_le64(&stage.entry);
+ stage.load = read_le64(&stage.load);
+ stage.len = read_le32(&stage.len);
+ stage.memlen = read_le32(&stage.memlen);
+
assert(fsize == stage.len);
/* Note: cbfs_stage fields are currently in the endianness of the
Updated by Marty Plummer over 3 years ago
Patch seems to work. I get as far with a big endian coreboot as I do with
a little endian coreboot now (romstage->ramstage handoff fails for some reason).
Updated by Marty Plummer over 3 years ago
This may be enough for now, but perhaps we should see about reworking
cbfs* to properly encode these things in big/little endian depending
on the target, and have the code just use the 'native' format instead
of doing these explicit read_{be,le}* calls.
Updated by Aaron Durbin over 3 years ago
I agree that this should be corrected, but there's not much we can do while still maintaining backwards compatibility. The endian calls, regardless of which one employed, would be required. The thing that always bugs me is the inconsistency. The cbfs_file is well defined and uses big endian. Then we have to deal with the cbfs_stage which is not well defined and happens to be little endian. I can formulate a proper patch.
Updated by Aaron Durbin over 3 years ago
Updated by Marty Plummer over 3 years ago
Aaron Durbin wrote:
I agree that this should be corrected, but there's not much we can do while still maintaining backwards compatibility. The endian calls, regardless of which one employed, would be required. The thing that always bugs me is the inconsistency. The cbfs_file is well defined and uses big endian. Then we have to deal with the cbfs_stage which is not well defined and happens to be little endian. I can formulate a proper patch.
I mean, we can probably maintain backwards compat for LE targets (which is most targets) and add a idk, --endian [big|little] flag to cbfstool going forward?