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
On 12/28/2015 09:31 AM, Kevin O'Connor wrote:
On Thu, Dec 24, 2015 at 03:45:03PM -0800, Alex Gagniuc wrote:
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?
I won't be able to test on actual hardware until next year, so feel free to do so.
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.
I'm suggesting to not rely on (0 - 4) to produce 0xfffffffc. The (unsigned) cast does not solve the C issue of writing a negative value into an unsigned type. Personally, I would refactor to something like (casts purchased separately):
#define CBFS_MASTER_PTR_LOC 0xfffffffc
if (CONFIG_CBFS_LOCATION) *hdr = (CONFIG_CBFS_LOCATION - 4); else *hdr = CBFS_MASTER_PTR_LOC;
It's a little more code, but it avoids the trap of blaming things on undefined behavior, a trap which I fell into when debugging the crash.
Alex
On Mon, Dec 28, 2015 at 04:09:22PM -0800, Alex G. wrote:
Are you suggesting that an (unsigned) cast should be added? I doubt it would matter.
I'm suggesting to not rely on (0 - 4) to produce 0xfffffffc. The (unsigned) cast does not solve the C issue of writing a negative value into an unsigned type.
FYI, assigning "0 - 4" to an unsigned integer is not undefined. From the C11 spec, section 6.3.1.3:
1 When a value with integer type is converted to another integer type other than _Bool, if the value can be represented by the new type, it is unchanged.
2 Otherwise, if the new type is unsigned, the value is converted by repeatedly adding or subtracting one more than the maximum value that can be represented in the new type until the value is in the range of the new type.
-Kevin
-
Alex G. -
Kevin O'Connor