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