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@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.*/@@ -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 filewalk += (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" */
- 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@hermann-uwe.de
Thanks, will fix and commit.
Carl-Daniel