https://ticket.coreboot.org/https://ticket.coreboot.org/themes/PurpleMine2-2.16.2/favicon/favicon.ico?12020-10-07T00:59:18ZIssue Trackercoreboot - Bug #281: cbfs stage loading||encoding is not endian safe.https://ticket.coreboot.org/issues/281?journal_id=7282020-10-07T00:59:18ZMarty Plummerhanetzer@startmail.com
<ul></ul><p>This is also an issue if you build a big-endian coreboot<br>
on a big-endian system (in my case a talos ii running be<br>
gentoo), so something is definitely up.</p>
coreboot - Bug #281: cbfs stage loading||encoding is not endian safe.https://ticket.coreboot.org/issues/281?journal_id=7292020-10-07T12:49:52ZAaron Durbinadurbin@google.com
<ul></ul><p>fsize is derived from the region_device size for the data in cbfs_for_each_file():</p>
<pre><code>file.len = read_be32(&file.len);
file.offset = read_be32(&file.offset);
</code></pre>
<p>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. </p>
<p>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?</p>
coreboot - Bug #281: cbfs stage loading||encoding is not endian safe.https://ticket.coreboot.org/issues/281?journal_id=7302020-10-07T14:33:40ZMarty Plummerhanetzer@startmail.com
<ul></ul><p>Aaron Durbin wrote:</p>
<blockquote>
<p>fsize is derived from the region_device size for the data in cbfs_for_each_file():</p>
<pre><code>file.len = read_be32(&file.len);
file.offset = read_be32(&file.offset);
</code></pre>
<p>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. </p>
<p>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?</p>
</blockquote>
<pre><code>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 |................|
</code></pre>
<p><sup>seems</sup> to be the pertenant parts you want. At src/lib/cbfs.c:299<br>
the values are as follows:<br>
fsize: 0x0000_313c<br>
stage.len: 0x3c31_0000</p>
coreboot - Bug #281: cbfs stage loading||encoding is not endian safe.https://ticket.coreboot.org/issues/281?journal_id=7312020-10-07T14:56:32ZAaron Durbinadurbin@google.com
<ul></ul><p>Original sighting:<br>
"fsize of 0x5c32_0000 and a stage.len of 0x0000_325c"</p>
<p>Update <a class="issue tracker-1 status-5 priority-2 priority-default closed behind-schedule" title="Bug: The build system isn't helpful when util/xcompile/xcompile fails because of missing dependencies. (Closed)" href="https://ticket.coreboot.org/issues/3">#3</a>:<br>
"fsize: 0x0000_313c stage.len: 0x3c31_0000"</p>
<p>Those are in conflict. From the hexdump I can see cbfs_file is correct for romstage. And given update <a class="issue tracker-1 status-5 priority-2 priority-default closed behind-schedule" title="Bug: The build system isn't helpful when util/xcompile/xcompile fails because of missing dependencies. (Closed)" href="https://ticket.coreboot.org/issues/3">#3</a> where stage.len is different.</p>
<p>struct cbfs_stage starts at 0x202b8 for fallback/romstage file.</p>
<pre><code>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..|
</code></pre>
<p>Last field (at 0x202cc) is the memlen which is clearly in little endian format.</p>
<p>Can you please provide the following for your cbfstool?</p>
<pre><code>$ readelf -h $(which cbfstool)
</code></pre>
<p>cbfs stage file is assumed to be in target endian format: <a href="https://review.coreboot.org/plugins/gitiles/coreboot/+/refs/heads/master/src/lib/cbfs.c#301" class="external">https://review.coreboot.org/plugins/gitiles/coreboot/+/refs/heads/master/src/lib/cbfs.c#301</a></p>
coreboot - Bug #281: cbfs stage loading||encoding is not endian safe.https://ticket.coreboot.org/issues/281?journal_id=7322020-10-07T15:05:35ZMarty Plummerhanetzer@startmail.com
<ul></ul><p>Aaron Durbin wrote:</p>
<blockquote>
<p>Original sighting:<br>
"fsize of 0x5c32_0000 and a stage.len of 0x0000_325c"</p>
<p>Update <a class="issue tracker-1 status-5 priority-2 priority-default closed behind-schedule" title="Bug: The build system isn't helpful when util/xcompile/xcompile fails because of missing dependencies. (Closed)" href="https://ticket.coreboot.org/issues/3">#3</a>:<br>
"fsize: 0x0000_313c stage.len: 0x3c31_0000"</p>
<p>Those are in conflict. From the hexdump I can see cbfs_file is correct for romstage. And given update <a class="issue tracker-1 status-5 priority-2 priority-default closed behind-schedule" title="Bug: The build system isn't helpful when util/xcompile/xcompile fails because of missing dependencies. (Closed)" href="https://ticket.coreboot.org/issues/3">#3</a> where stage.len is different.</p>
</blockquote>
<p>I had to recompile; I've been building little-endian for the moment because otherwise I'd be stuck here.</p>
<blockquote>
<p>struct cbfs_stage starts at 0x202b8 for fallback/romstage file.</p>
<pre><code>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..|
</code></pre>
<p>Last field (at 0x202cc) is the memlen which is clearly in little endian format.</p>
<p>Can you please provide the following for your cbfstool?</p>
<pre><code>$ readelf -h $(which cbfstool)
</code></pre></blockquote>
<pre><code>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
</code></pre>
<blockquote>
<p>cbfs stage file is assumed to be in target endian format: <a href="https://review.coreboot.org/plugins/gitiles/coreboot/+/refs/heads/master/src/lib/cbfs.c#301" class="external">https://review.coreboot.org/plugins/gitiles/coreboot/+/refs/heads/master/src/lib/cbfs.c#301</a></p>
</blockquote>
coreboot - Bug #281: cbfs stage loading||encoding is not endian safe.https://ticket.coreboot.org/issues/281?journal_id=7332020-10-07T15:15:54ZAaron Durbinadurbin@google.com
<ul></ul><p>Thanks for the info. In the end it doesn't matter...</p>
<p><a href="https://review.coreboot.org/plugins/gitiles/coreboot/+/refs/heads/master/util/cbfstool/cbfs-mkstage.c#80" class="external">https://review.coreboot.org/plugins/gitiles/coreboot/+/refs/heads/master/util/cbfstool/cbfs-mkstage.c#80</a></p>
<p>`</p>
<blockquote>
<p>-------/* N.B. The original plan was that SELF data was B.E.<br>
------- * but: this is all L.E.<br>
------- * Maybe we should just change the spec.<br>
------- */<br>
-------xdr_le.put32(outheader, algo);<br>
-------xdr_le.put64(outheader, entry);<br>
-------xdr_le.put64(outheader, loadaddr);<br>
-------xdr_le.put32(outheader, filesize);<br>
-------xdr_le.put32(outheader, memsize);<br>
`</p>
</blockquote>
<p>Given that cbfs_stage being written in little endian unconditionally we should probably fix the code then that I linked to in <a class="issue tracker-2 status-5 priority-2 priority-default closed" title="Feature: Documentation for coreboot's public interface (Closed)" href="https://ticket.coreboot.org/issues/4">#4</a>. </p>
<p>Can you please try this patch?</p>
<pre><code>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
</code></pre> coreboot - Bug #281: cbfs stage loading||encoding is not endian safe.https://ticket.coreboot.org/issues/281?journal_id=7342020-10-08T00:29:02ZMarty Plummerhanetzer@startmail.com
<ul></ul><p>Patch seems to work. I get as far with a big endian coreboot as I do with<br>
a little endian coreboot now (romstage->ramstage handoff fails for some reason).</p>
coreboot - Bug #281: cbfs stage loading||encoding is not endian safe.https://ticket.coreboot.org/issues/281?journal_id=7352020-10-08T01:34:23ZMarty Plummerhanetzer@startmail.com
<ul></ul><p>This may be enough for now, but perhaps we should see about reworking<br>
cbfs* to properly encode these things in big/little endian depending<br>
on the target, and have the code just use the 'native' format instead<br>
of doing these explicit read_{be,le}* calls.</p>
coreboot - Bug #281: cbfs stage loading||encoding is not endian safe.https://ticket.coreboot.org/issues/281?journal_id=7362020-10-08T13:40:14ZAaron Durbinadurbin@google.com
<ul></ul><p>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.</p>
coreboot - Bug #281: cbfs stage loading||encoding is not endian safe.https://ticket.coreboot.org/issues/281?journal_id=7372020-10-08T19:00:44ZAaron Durbinadurbin@google.com
<ul></ul><p><a href="https://review.coreboot.org/c/coreboot/+/46227" class="external">https://review.coreboot.org/c/coreboot/+/46227</a></p>
coreboot - Bug #281: cbfs stage loading||encoding is not endian safe.https://ticket.coreboot.org/issues/281?journal_id=7382020-10-08T22:11:27ZMarty Plummerhanetzer@startmail.com
<ul></ul><p>Aaron Durbin wrote:</p>
<blockquote>
<p>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.</p>
</blockquote>
<p>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?</p>