[coreboot] [RFC][PATCH] v3: lar header after member support

Carl-Daniel Hailfinger c-d.hailfinger.devel.2006 at gmx.net
Fri Mar 28 12:38:25 CET 2008


On 28.03.2008 10:33, Stefan Reinauer wrote:
> Please dont do this. There's no reason to not just add a jump
> instruction in front of an archive on architectures like mips and PPC
>   

Ugh. How do we handle extraction of archive contents, then? The jump
instruction is not part of any archive member and will simply be
discarded. That would violate our otherwise clean LAR design.


> Also, NACK as long as the code adds #warnings instead of fixing issues.
>   

See below.


> Carl-Daniel Hailfinger wrote:
>   
>> We need this for MIPS.
>>
>> On 05.12.2007 03:26, Carl-Daniel Hailfinger wrote:
>>   
>>     
>>> Index: LinuxBIOSv3-larheaderaftermember/lib/lar.c
>>> ===================================================================
>>> --- LinuxBIOSv3-larheaderaftermember/lib/lar.c	(Revision 539)
>>> +++ LinuxBIOSv3-larheaderaftermember/lib/lar.c	(Arbeitskopie)
>>> @@ -137,9 +139,30 @@
>>>  		 * In the case of consecutive archive members with header-
>>>  		 * before-member structure, the next iteration of the loop will
>>>  		 * start exactly at the beginning of the next header.
>>> -		 */
>>> +		 * In the case of header-after-member (e.g. for bottom boot
>>> +		 * architectures) the calculation below will effectively cause
>>> +		 * walk < header. To elaborate a bit more, in this case
>>> +		 * (header->offset + header->len - 1) will evaluate to a value
>>> +		 * between -1 (header directly after file), -16 (file, 15 bytes
>>> +		 * pad, header), and even smaller values if there is 16 bytes
>>> +		 * or more of padding between member and header. The outer
>>> +		 * expression will usually evaluate to 0xfffffff0, cause the
>>> +		 * expected overflow with unsigned arithmetic and result in a
>>> +		 * decrease of walk. That condition can be checked. */
>>> +#warning FIXME: This loop will explode if this code is ever compiled in 64bit mode
>>> +#warning because of the hardcoded 0xfffffff0.
>>>       

We assume sizeof(void *) == sizeof(long) in a number of places. That
assumption is untrue on Win64. Is there any other ABI where that
assumption is incorrect as well?

>>>  		walk += (ntohl(header->offset) + ntohl(header->len) - 1)
>>>  			 & 0xfffffff0;
>>>       

I'm considering to replace the 0xfffffff0 with (unsigned long)-16 to
make this more explicit.

Then again, unless we have archive members >=2^31 bytes, the above is
not a bug, just poorly documented code. I'll remove the warning from the
patch.


>>> +		/* If we have header-after-member, walk < header is true.
>>> +		 * Go forward instead by starting at header, adding header size
>>> +		 * and strlen(fullname). The result of this calculation is the
>>> +		 * position of the terminating \0 of fullname. Round that
>>> +		 * address down to the next 16 byte boundary. */
>>> +		if (walk < (char *)header) {
>>> +			walk = (char *)header;
>>> +			walk += (sizeof(struct lar_header) + strlen(fullname))
>>> +				& 0xfffffff0;
>>> +		}
>>>  	}
>>>  	printk(BIOS_SPEW, "LAR: File not found!\n");
>>>  	return 1;
>>>
>>>       


Regards,
Carl-Daniel




More information about the coreboot mailing list