Bug #281

cbfs stage loading||encoding is not endian safe.

Added by Marty Plummer 3 months ago. Updated 3 months ago.

Status:NewStart date:10/07/2020
Priority:NormalDue date:
Assignee:-% Done:

0%

Category:-
Target version:-

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.

History

#1 Updated by Marty Plummer 3 months 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.

#2 Updated by Aaron Durbin 3 months 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?

#3 Updated by Marty Plummer 3 months 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

#4 Updated by Aaron Durbin 3 months 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

#5 Updated by Marty Plummer 3 months 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

#6 Updated by Aaron Durbin 3 months ago

Thanks for the info. In the end it doesn't matter...

https://review.coreboot.org/plugins/gitiles/coreboot/+/refs/heads/master/util/cbfstool/cbfs-mkstage.c#80

`

-------/* 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

#7 Updated by Marty Plummer 3 months 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).

#8 Updated by Marty Plummer 3 months 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.

#9 Updated by Aaron Durbin 3 months 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.

#11 Updated by Marty Plummer 3 months 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?

Also available in: Atom PDF