[LinuxBIOS] [PATCH] v3: improve LAR comments, fix possible access to uninitialized memory
Carl-Daniel Hailfinger
c-d.hailfinger.devel.2006 at gmx.net
Tue Oct 16 20:42:42 CEST 2007
On 16.10.2007 19:30, Uwe Hermann wrote:
> On Sat, Oct 13, 2007 at 07:52:55PM +0200, Carl-Daniel Hailfinger wrote:
>
>> Fix a corner case access to uninitialized memory (NULL pointer
>> dereference or worse) in case the archive length is exactly
>> sizeof(struct lar_header). Such an archive is invalid because the
>> filename directly after the LAR header is always dereferenced and has to
>> be at least 1 byte in the "empty filename" case (only terminating \0).
>> Improve LAR code documentation and reorder variables in one assignment
>> to make the code more obvious and readable. This will help people
>> understand what the code does when they look at it half a year from now.
>>
>> Signed-off-by: Carl-Daniel Hailfinger <c-d.hailfinger.devel.2006 at gmx.net>
>> ---
>>
>> Index: LinuxBIOSv3/include/lar.h
>> ===================================================================
>> --- LinuxBIOSv3/include/lar.h (Revision 505)
>> +++ LinuxBIOSv3/include/lar.h (Arbeitskopie)
>> @@ -52,10 +52,9 @@
>>
>> #include <types.h>
>>
>> -/* see note in lib/lar.c as to why this is ARCHIVE and not LARCHIVE */
>> #define MAGIC "LARCHIVE"
>> #define MAX_PATHLEN 1024
>> -/* NOTE -- This and the user-mode lar.h are NOT IN SYNC. Be careful. */
>> +/* NOTE -- This and the user-mode lar.h may NOT be IN SYNC. Be careful. */
>>
>
> IN SYNC -> in sync (the NOT should stay all-caps)
>
OK
>> struct lar_header {
>> char magic[8];
>> u32 len;
>> Index: LinuxBIOSv3/lib/lar.c
>> ===================================================================
>> --- LinuxBIOSv3/lib/lar.c (Revision 505)
>> +++ LinuxBIOSv3/lib/lar.c (Arbeitskopie)
>> @@ -49,7 +49,7 @@
>> * Given a file name in the LAR , search for it, and fill out a return struct with the
>> * result.
>> * @param archive A descriptor for current archive. This is actually a mem_file type,
>> - * which is a machine-dependent representation of hte actual archive. In particular,
>> + * which is a machine-dependent representation of the actual archive. In particular,
>>
>
> Yep.
>
>> * things like u32 are void * in the mem_file struct.
>> * @param filename filename to find
>> * @param result pointer to mem_file struct which is filled in if the file is found
>> @@ -65,30 +65,43 @@
>> printk(BIOS_SPEW, "LAR: Start %p len 0x%x\n", archive->start,
>> archive->len);
>>
>> - if (archive->len < sizeof(struct lar_header))
>> - printk(BIOS_ERR, "Error: truncated archive (%d bytes); minimum possible size is %d bytes\n",
>> - archive->len, sizeof(struct lar_header));
>> + /* Why check for sizeof(struct lar_header) + 1? The code below expects
>> + * a filename to follow directly after the LAR header and will
>> + * dereference the address directly after the header. However, if
>> + * archive->len == sizeof(struct lar_header), printing the filename
>> + * will dereference memory outside the archive. Without looking at the
>> + * filename, the only thing we can check is that there is at least room
>> + * for an empty filename (only the terminating \0).
>> + */
>> + if (archive->len < sizeof(struct lar_header) + 1)
>> + printk(BIOS_ERR, "Error: truncated archive (%d bytes); minimum"
>> + " possible size is %d bytes\n",
>> + archive->len, sizeof(struct lar_header) + 1);
>>
>
> Is this build-tested and tested with on a real lar file etc?
>
It is build-tested. It should not fail with any real LAR file unless
that LAR file is deliberately truncated and therefore invalid.
>> - /* Getting this for loop right is harder than it looks. All quantities are
>> - * unsigned. The LAR stretches from (e.g.) 0xfff0000 for 0x100000
>> - * bytes, i.e. to address ZERO.
>> - * As a result, 'walk', can wrap to zero and keep going (this has been
>> - * seen in practice). Recall that these are unsigned; walk can
>> - * wrap to zero; so, question, when is walk less than any of these:
>> - * archive->start
>> - * Answer: once walk wraps to zero, it is < archive->start
>> - * archive->start + archive->len
>> - * archive->start + archive->len - 1
>> - * Answer: In the case that archive->start + archive->len == 0, ALWAYS!
>> - * A lot of expressions have been tried and all have been wrong.
>> - * So what would work? Simple:
>> - * test for walk < archive->start + archive->len - 1 to cover the case
>> - * that the archive does NOT occupy ALL of the top of memory and
>> - * wrap to zero;
>> - * and test for walk >= archive->start,
>> - * to cover the case that you wrapped to zero.
>> - * Unsigned pointer arithmetic that wraps to zero can be messy.
>> - */
>> + /* Getting this for loop right is harder than it looks. All quantities
>> + * are unsigned. The LAR stretches from (e.g.) 0xfff0000 for 0x100000
>> + * bytes, i.e. to address ZERO.
>> + * As a result, 'walk', can wrap to zero and keep going (this has been
>> + * seen in practice). Recall that these are unsigned; walk can
>> + * wrap to zero; so, question, when is walk less than any of these:
>> + * archive->start
>> + * Answer: once walk wraps to zero, it is < archive->start
>> + * archive->start + archive->len
>> + * archive->start + archive->len - 1
>> + * Answer: In the case that archive->start + archive->len == 0, ALWAYS!
>> + * A lot of expressions have been tried and all have been wrong.
>> + * So what would work? Simple:
>> + * test for walk < archive->start + archive->len - sizeof(lar_header)
>> + * to cover the case that the archive does NOT occupy ALL of the
>> + * top of memory and wrap to zero; RESIST the temptation to change
>> + * that comparison to <= because if a header did terminate the
>> + * archive, the filename (stored directly after the header) would
>> + * be outside the archive and you'd get a nice NULL pointer for
>> + * the filename
>> + * and test for walk >= archive->start,
>> + * to cover the case that you wrapped to zero.
>> + * Unsigned pointer arithmetic that wraps to zero can be messy.
>> + */
>> for (walk = archive->start;
>> (walk < (char *)(archive->start + archive->len - sizeof(struct lar_header))) &&
>> (walk >= (char *)archive->start); walk += 16) {
>> @@ -98,7 +111,7 @@
>> header = (struct lar_header *)walk;
>> fullname = walk + sizeof(struct lar_header);
>>
>> - printk(BIOS_SPEW, "LAR: search for %s\n", fullname);
>> + printk(BIOS_SPEW, "LAR: seen member %s\n", fullname);
>> // FIXME: check checksum
>>
>> if (strcmp(fullname, filename) == 0) {
>> @@ -115,11 +128,20 @@
>> result->compression, result->entry, result->loadaddress);
>> return 0;
>> }
>> - // skip file
>> - walk += (ntohl(header->len) + ntohl(header->offset) -
>> - 1) & 0xfffffff0;
>> + /* skip file:
>> + * The next iteration of the for loop will add 16 to walk, so
>> + * we now add offset (from header start to data start) and len
>> + * (member length), subtract 1 (to get the address of the last
>> + * byte of the member) and round this down to the next 16 byte
>> + * boundary.
>> + * 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.
>> + */
>> + walk += (ntohl(header->offset) + ntohl(header->len) - 1)
>> + & 0xfffffff0;
>> }
>> - printk(BIOS_SPEW, "NO FILE FOUND\n");
>> + printk(BIOS_SPEW, "LAR: NO FILE FOUND\n");
>>
>
> printk(BIOS_SPEW, "LAR: NO FILE FOUND!\n");
>
OK
>> return 1;
>> }
>>
>> @@ -157,7 +179,7 @@
>> * the loadaddress pointer in the mem_file struct.
>> * @param archive A descriptor for current archive.
>> * @param filename filename to find
>> - * returns 0 on success, -1 otherwise
>> + * returns entry on success, (void*)-1 otherwise
>> */
>>
>> void *load_file(struct mem_file *archive, const char *filename)
>> Index: LinuxBIOSv3/util/lar/lar.h
>> ===================================================================
>> --- LinuxBIOSv3/util/lar/lar.h (Revision 505)
>> +++ LinuxBIOSv3/util/lar/lar.h (Arbeitskopie)
>> @@ -61,13 +61,15 @@
>> typedef uint32_t u32;
>> typedef uint8_t u8;
>>
>> -/* NOTE -- This and the linuxbios lar.h are NOT IN SYNC. Be careful. */
>> +/* NOTE -- This and the user-mode lar.h may NOT be IN SYNC. Be careful. */
>>
>
> IN SYNC -> in sync
>
OK
>> struct lar_header {
>> char magic[8];
>> u32 len;
>> u32 reallen;
>> u32 checksum;
>> u32 compchecksum;
>> + /* Filenames are limited to 2^31-1-sizeof(lar_header)-1 bytes.
>> + * "Nobody will ever need more than 640k" */
>> u32 offset;
>> /* Compression:
>> * 0 = no compression
>> Index: LinuxBIOSv3/arch/x86/stage1.c
>> ===================================================================
>> --- LinuxBIOSv3/arch/x86/stage1.c (Revision 505)
>> +++ LinuxBIOSv3/arch/x86/stage1.c (Arbeitskopie)
>> @@ -69,7 +69,7 @@
>> }
>>
>> /*
>> - * This function is called from assembler code whith its argument on the
>> + * This function is called from assembler code with its argument on the
>> * stack. Force the compiler to generate always correct code for this case.
>> */
>> void __attribute__((stdcall)) stage1_main(u32 bist)
>> @@ -140,7 +140,7 @@
>> } else {
>> printk(BIOS_DEBUG, "Choosing fallback boot.\n");
>> ret = execute_in_place(&archive, "fallback/initram");
>> - /* Try a normal boot if fallback doesn't exists in the lar.
>> + /* Try a normal boot if fallback doesn't exist in the lar.
>> * TODO: There are other ways to do this.
>> * It could be ifdef or the boot flag could be forced.
>> */
>>
>
>
> Looks good otherwise. With the above fixes:
>
> Acked-by: Uwe Hermann <uwe at hermann-uwe.de>
>
Thanks, will fix and commit.
Carl-Daniel
More information about the coreboot
mailing list