On Thu, Dec 24, 2015 at 03:45:03PM -0800, Alex Gagniuc wrote:
When SeaBIOS is run on an FMAP-based CBFS (AKA, without a CBFS master header, it crashes). The problem is in coreboot_cbfs_init. A disassembly with -Mintel shows the following:
00000000 <coreboot_cbfs_init>: ... 7: 8b 35 fc ff ff ff mov esi,DWORD PTR ds:0xfffffffc d: 81 3e 4f 52 42 43 cmp DWORD PTR [esi],0x4342524f
7: 8b 35 fc ff ff ff mov esi,DWORD PTR ds:0xfffffffc
Move the contents of memory address 0xfffffffc to esi. Since there's no legacy CBFS pointer, the region is all 1's. esi now contains 0xffffffff
d: 81 3e 4f 52 42 43 cmp DWORD PTR [esi],0x4342524f
32-bit dereference esi (0xffffffff) -- causes general protection fault. The dereference is not because of unaligned access, but because the access goes over the 32-bit address space in flat mode. The way around this iss to sanitize the value at [0xffffffff] before dereferencing it. A sane check is:
if(address_of_hdr > (0xfffffff0 - sizeof(*hdr))) fail(); This is sane check because because the x86 reset vector is resides at 0xfffffff0 (there are cases where this isn't true, but that's besides the point here).
Thanks. Can you put together a patch, or do you want me to?
There's also an unrelated problem with the C code that generates this assembly:
struct cbfs_header *hdr = *(void **)(CONFIG_CBFS_LOCATION - 4);
With CONFIG_CBFS_LOCATION = 0, this places a negative value (-4) in an unsigned type (pointer). The behavior of this is undefined according to the C standard.
Are you suggesting that an (unsigned) cast should be added? I doubt it would matter.
-Kevin