[LinuxBIOS] [PATCH] v3: improve LAR comments, fix possible access to uninitialized memory
Uwe Hermann
uwe at hermann-uwe.de
Tue Oct 16 19:30:57 CEST 2007
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)
> 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?
> - /* 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");
> 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
> 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>
Uwe.
--
http://www.hermann-uwe.de | http://www.holsham-traders.de
http://www.crazy-hacks.org | http://www.unmaintained-free-software.org
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 189 bytes
Desc: Digital signature
URL: <http://www.coreboot.org/pipermail/coreboot/attachments/20071016/bccc814e/attachment.sig>
More information about the coreboot
mailing list