[multiboot] Remove userptr_t from Multiboot and ELF image parsing

Simplify Multiboot and ELF image parsing by assuming that the
Multiboot and ELF headers are directly accessible via pointer
dereferences, and add some missing header validations.

Signed-off-by: Michael Brown <mcb30@ipxe.org>

diff --git a/src/arch/x86/image/elfboot.c b/src/arch/x86/image/elfboot.c
index 8d167ef4d919386aad805cd38a047105d001b1c2..63a3460d33f7d41d44fa47cbbaadd9d538811fea 100644 (file)
--- a/src/arch/x86/image/elfboot.c
+++ b/src/arch/x86/image/elfboot.c
@@ -87,7 +87,7 @@ static int elfboot_exec ( struct image *image ) {
  * @v dest             Destination address
  * @ret rc             Return status code
  */
-static int elfboot_check_segment ( struct image *image, Elf_Phdr *phdr,
+static int elfboot_check_segment ( struct image *image, const Elf_Phdr *phdr,
                                   physaddr_t dest ) {
 
        /* Check that ELF segment uses flat physical addressing */
@@ -107,7 +107,7 @@ static int elfboot_check_segment ( struct image *image, Elf_Phdr *phdr,
  * @ret rc             Return status code
  */
 static int elfboot_probe ( struct image *image ) {
-       Elf32_Ehdr ehdr;
+       const Elf32_Ehdr *ehdr;
        static const uint8_t e_ident[] = {
                [EI_MAG0]       = ELFMAG0,
                [EI_MAG1]       = ELFMAG1,
@@ -122,14 +122,19 @@ static int elfboot_probe ( struct image *image ) {
        int rc;
 
        /* Read ELF header */
-       copy_from_user ( &ehdr, image->data, 0, sizeof ( ehdr ) );
-       if ( memcmp ( ehdr.e_ident, e_ident, sizeof ( e_ident ) ) != 0 ) {
+       if ( image->len < sizeof ( *ehdr ) ) {
+               DBGC ( image, "ELF %s too short for ELF header\n",
+                      image->name );
+               return -ENOEXEC;
+       }
+       ehdr = image->data;
+       if ( memcmp ( ehdr->e_ident, e_ident, sizeof ( e_ident ) ) != 0 ) {
                DBGC ( image, "ELF %s invalid identifier\n", image->name );
                return -ENOEXEC;
        }
 
        /* Check that this image uses flat physical addressing */
-       if ( ( rc = elf_segments ( image, &ehdr, elfboot_check_segment,
+       if ( ( rc = elf_segments ( image, ehdr, elfboot_check_segment,
                                   &entry, &max ) ) != 0 ) {
                DBGC ( image, "ELF %s is not loadable: %s\n",
                       image->name, strerror ( rc ) );

diff --git a/src/arch/x86/image/multiboot.c b/src/arch/x86/image/multiboot.c
index b23cf9ddc3ec85d77d9acde5723968e7254a7517..7c8963475229a3729d3ec162561bddb87ce52c93 100644 (file)
--- a/src/arch/x86/image/multiboot.c
+++ b/src/arch/x86/image/multiboot.c
@@ -35,7 +35,6 @@ FILE_LICENCE ( GPL2_OR_LATER_OR_UBDL );
 #include <assert.h>
 #include <realmode.h>
 #include <multiboot.h>
-#include <ipxe/uaccess.h>
 #include <ipxe/image.h>
 #include <ipxe/segment.h>
 #include <ipxe/io.h>
@@ -87,14 +86,6 @@ FEATURE ( FEATURE_IMAGE, "MBOOT", DHCP_EB_FEATURE_MULTIBOOT, 1 );
  */
 #define MB_UNSUPPORTED_FLAGS ( MB_COMPULSORY_FLAGS & ~MB_SUPPORTED_FLAGS )
 
-/** A multiboot header descriptor */
-struct multiboot_header_info {
-       /** The actual multiboot header */
-       struct multiboot_header mb;
-       /** Offset of header within the multiboot image */
-       size_t offset;
-};
-
 /** Multiboot module command lines */
 static char __bss16_array ( mb_cmdlines, [MB_MAX_CMDLINE] );
 #define mb_cmdlines __use_data16 ( mb_cmdlines )
@@ -267,82 +258,89 @@ static struct multiboot_module __bss16_array ( mbmodules, [MAX_MODULES] );
  * Find multiboot header
  *
  * @v image            Multiboot file
- * @v hdr              Multiboot header descriptor to fill in
- * @ret rc             Return status code
+ * @ret offset         Offset to Multiboot header, or negative error
  */
-static int multiboot_find_header ( struct image *image,
-                                  struct multiboot_header_info *hdr ) {
-       uint32_t buf[64];
+static int multiboot_find_header ( struct image *image ) {
+       const struct multiboot_header *mb;
        size_t offset;
-       unsigned int buf_idx;
        uint32_t checksum;
 
-       /* Scan through first 8kB of image file 256 bytes at a time.
-        * (Use the buffering to avoid the overhead of a
-        * copy_from_user() for every dword.)
-        */
-       for ( offset = 0 ; offset < 8192 ; offset += sizeof ( buf[0] ) ) {
+       /* Scan through first 8kB of image file */
+       for ( offset = 0 ; offset < 8192 ; offset += 4 ) {
                /* Check for end of image */
-               if ( offset > image->len )
+               if ( ( offset + sizeof ( *mb ) ) > image->len )
                        break;
-               /* Refill buffer if applicable */
-               buf_idx = ( ( offset % sizeof ( buf ) ) / sizeof ( buf[0] ) );
-               if ( buf_idx == 0 ) {
-                       copy_from_user ( buf, image->data, offset,
-                                        sizeof ( buf ) );
-               }
+               mb = ( image->data + offset );
                /* Check signature */
-               if ( buf[buf_idx] != MULTIBOOT_HEADER_MAGIC )
+               if ( mb->magic != MULTIBOOT_HEADER_MAGIC )
                        continue;
                /* Copy header and verify checksum */
-               copy_from_user ( &hdr->mb, image->data, offset,
-                                sizeof ( hdr->mb ) );
-               checksum = ( hdr->mb.magic + hdr->mb.flags +
-                            hdr->mb.checksum );
+               checksum = ( mb->magic + mb->flags + mb->checksum );
                if ( checksum != 0 )
                        continue;
-               /* Record offset of multiboot header and return */
-               hdr->offset = offset;
-               return 0;
+               /* Return header */
+               return offset;
        }
 
        /* No multiboot header found */
+       DBGC ( image, "MULTIBOOT %s has no multiboot header\n",
+              image->name );
        return -ENOEXEC;
 }
 
 /**
  * Load raw multiboot image into memory
  *
- * @v image            Multiboot file
- * @v hdr              Multiboot header descriptor
+ * @v image            Multiboot image
+ * @v offset           Offset to Multiboot header
  * @ret entry          Entry point
  * @ret max            Maximum used address
  * @ret rc             Return status code
  */
-static int multiboot_load_raw ( struct image *image,
-                               struct multiboot_header_info *hdr,
+static int multiboot_load_raw ( struct image *image, size_t offset,
                                physaddr_t *entry, physaddr_t *max ) {
-       size_t offset;
+       const struct multiboot_header *mb = ( image->data + offset );
        size_t filesz;
        size_t memsz;
-       userptr_t buffer;
+       void *buffer;
        int rc;
 
        /* Sanity check */
-       if ( ! ( hdr->mb.flags & MB_FLAG_RAW ) ) {
+       if ( ! ( mb->flags & MB_FLAG_RAW ) ) {
                DBGC ( image, "MULTIBOOT %s is not flagged as a raw image\n",
                       image->name );
                return -EINVAL;
        }
 
-       /* Verify and prepare segment */
-       offset = ( hdr->offset - hdr->mb.header_addr + hdr->mb.load_addr );
-       filesz = ( hdr->mb.load_end_addr ?
-                  ( hdr->mb.load_end_addr - hdr->mb.load_addr ) :
+       /* Calculate starting offset within file */
+       if ( ( mb->load_addr > mb->header_addr ) ||
+            ( ( mb->header_addr - mb->load_addr ) > offset ) ) {
+               DBGC ( image, "MULTIBOOT %s has misplaced header\n",
+                      image->name );
+               return -EINVAL;
+       }
+       offset -= ( mb->header_addr - mb->load_addr );
+       assert ( offset < image->len );
+
+       /* Calculate length of initialized data */
+       filesz = ( mb->load_end_addr ?
+                  ( mb->load_end_addr - mb->load_addr ) :
                   ( image->len - offset ) );
-       memsz = ( hdr->mb.bss_end_addr ?
-                 ( hdr->mb.bss_end_addr - hdr->mb.load_addr ) : filesz );
-       buffer = phys_to_virt ( hdr->mb.load_addr );
+       if ( filesz > image->len ) {
+               DBGC ( image, "MULTIBOOT %s has overlength data\n",
+                      image->name );
+               return -EINVAL;
+       }
+
+       /* Calculate length of uninitialised data */
+       memsz = ( mb->bss_end_addr ?
+                 ( mb->bss_end_addr - mb->load_addr ) : filesz );
+       DBGC ( image, "MULTIBOOT %s loading [%zx,%zx) to [%x,%zx,%zx)\n",
+              image->name, offset, ( offset + filesz ), mb->load_addr,
+              ( mb->load_addr + filesz ), ( mb->load_addr + memsz ) );
+
+       /* Verify and prepare segment */
+       buffer = phys_to_virt ( mb->load_addr );
        if ( ( rc = prep_segment ( buffer, filesz, memsz ) ) != 0 ) {
                DBGC ( image, "MULTIBOOT %s could not prepare segment: %s\n",
                       image->name, strerror ( rc ) );
@@ -353,8 +351,8 @@ static int multiboot_load_raw ( struct image *image,
        memcpy ( buffer, ( image->data + offset ), filesz );
 
        /* Record execution entry point and maximum used address */
-       *entry = hdr->mb.entry_addr;
-       *max = ( hdr->mb.load_addr + memsz );
+       *entry = mb->entry_addr;
+       *max = ( mb->load_addr + memsz );
 
        return 0;
 }
@@ -388,22 +386,24 @@ static int multiboot_load_elf ( struct image *image, physaddr_t *entry,
  * @ret rc             Return status code
  */
 static int multiboot_exec ( struct image *image ) {
-       struct multiboot_header_info hdr;
+       const struct multiboot_header *mb;
        physaddr_t entry;
        physaddr_t max;
+       int offset;
        int rc;
 
        /* Locate multiboot header, if present */
-       if ( ( rc = multiboot_find_header ( image, &hdr ) ) != 0 ) {
-               DBGC ( image, "MULTIBOOT %s has no multiboot header\n",
-                      image->name );
+       offset = multiboot_find_header ( image );
+       if ( offset < 0 ) {
+               rc = offset;
                return rc;
        }
+       mb = ( image->data + offset );
 
        /* Abort if we detect flags that we cannot support */
-       if ( hdr.mb.flags & MB_UNSUPPORTED_FLAGS ) {
-               DBGC ( image, "MULTIBOOT %s flags %08x not supported\n",
-                      image->name, ( hdr.mb.flags & MB_UNSUPPORTED_FLAGS ) );
+       if ( mb->flags & MB_UNSUPPORTED_FLAGS ) {
+               DBGC ( image, "MULTIBOOT %s flags %#08x not supported\n",
+                      image->name, ( mb->flags & MB_UNSUPPORTED_FLAGS ) );
                return -ENOTSUP;
        }
 
@@ -413,8 +413,10 @@ static int multiboot_exec ( struct image *image ) {
         * behaviour.
         */
        if ( ( ( rc = multiboot_load_elf ( image, &entry, &max ) ) != 0 ) &&
-            ( ( rc = multiboot_load_raw ( image, &hdr, &entry, &max ) ) != 0 ))
+            ( ( rc = multiboot_load_raw ( image, offset, &entry,
+                                          &max ) ) != 0 ) ) {
                return rc;
+       }
 
        /* Populate multiboot information structure */
        memset ( &mbinfo, 0, sizeof ( mbinfo ) );
@@ -469,17 +471,19 @@ static int multiboot_exec ( struct image *image ) {
  * @ret rc             Return status code
  */
 static int multiboot_probe ( struct image *image ) {
-       struct multiboot_header_info hdr;
+       const struct multiboot_header *mb;
+       int offset;
        int rc;
 
        /* Locate multiboot header, if present */
-       if ( ( rc = multiboot_find_header ( image, &hdr ) ) != 0 ) {
-               DBGC ( image, "MULTIBOOT %s has no multiboot header\n",
-                      image->name );
+       offset = multiboot_find_header ( image );
+       if ( offset < 0 ) {
+               rc = offset;
                return rc;
        }
-       DBGC ( image, "MULTIBOOT %s found header with flags %08x\n",
-              image->name, hdr.mb.flags );
+       mb = ( image->data + offset );
+       DBGC ( image, "MULTIBOOT %s found header at +%#x with flags %#08x\n",
+              image->name, offset, mb->flags );
 
        return 0;
 }

diff --git a/src/image/elf.c b/src/image/elf.c
index 1514342541169ea908573a6bf5d1713766cefd17..83712c3b0d0eb3fe83b527cd4b4ddd409cef2e89 100644 (file)
--- a/src/image/elf.c
+++ b/src/image/elf.c
@@ -35,7 +35,6 @@ FILE_LICENCE ( GPL2_OR_LATER_OR_UBDL );
 
 #include <errno.h>
 #include <elf.h>
-#include <ipxe/uaccess.h>
 #include <ipxe/segment.h>
 #include <ipxe/image.h>
 #include <ipxe/elf.h>
@@ -48,9 +47,9 @@ FILE_LICENCE ( GPL2_OR_LATER_OR_UBDL );
  * @v dest             Destination address
  * @ret rc             Return status code
  */
-static int elf_load_segment ( struct image *image, Elf_Phdr *phdr,
+static int elf_load_segment ( struct image *image, const Elf_Phdr *phdr,
                              physaddr_t dest ) {
-       userptr_t buffer = phys_to_virt ( dest );
+       void *buffer = phys_to_virt ( dest );
        int rc;
 
        DBGC ( image, "ELF %s loading segment [%x,%x) to [%lx,%lx,%lx)\n",
@@ -82,9 +81,11 @@ static int elf_load_segment ( struct image *image, Elf_Phdr *phdr,
  * @ret max            Maximum used address
  * @ret rc             Return status code
  */
-static int elf_segment ( struct image *image, Elf_Ehdr *ehdr, Elf_Phdr *phdr,
+static int elf_segment ( struct image *image, const Elf_Ehdr *ehdr,
+                        const Elf_Phdr *phdr,
                         int ( * process ) ( struct image *image,
-                                            Elf_Phdr *phdr, physaddr_t dest ),
+                                            const Elf_Phdr *phdr,
+                                            physaddr_t dest ),
                         physaddr_t *entry, physaddr_t *max ) {
        physaddr_t dest;
        physaddr_t end;
@@ -151,11 +152,12 @@ static int elf_segment ( struct image *image, Elf_Ehdr *ehdr, Elf_Phdr *phdr,
  * @ret max            Maximum used address
  * @ret rc             Return status code
  */
-int elf_segments ( struct image *image, Elf_Ehdr *ehdr,
-                  int ( * process ) ( struct image *image, Elf_Phdr *phdr,
+int elf_segments ( struct image *image, const Elf_Ehdr *ehdr,
+                  int ( * process ) ( struct image *image,
+                                      const Elf_Phdr *phdr,
                                       physaddr_t dest ),
                   physaddr_t *entry, physaddr_t *max ) {
-       Elf_Phdr phdr;
+       const Elf_Phdr *phdr;
        Elf_Off phoff;
        unsigned int phnum;
        int rc;
@@ -169,13 +171,14 @@ int elf_segments ( struct image *image, Elf_Ehdr *ehdr,
        /* Read and process ELF program headers */
        for ( phoff = ehdr->e_phoff , phnum = ehdr->e_phnum ; phnum ;
              phoff += ehdr->e_phentsize, phnum-- ) {
-               if ( phoff > image->len ) {
+               if ( ( image->len < phoff ) ||
+                    ( ( image->len - phoff ) < sizeof ( *phdr ) ) ) {
                        DBGC ( image, "ELF %s program header %d outside "
                               "image\n", image->name, phnum );
                        return -ENOEXEC;
                }
-               copy_from_user ( &phdr, image->data, phoff, sizeof ( phdr ) );
-               if ( ( rc = elf_segment ( image, ehdr, &phdr, process,
+               phdr = ( image->data + phoff );
+               if ( ( rc = elf_segment ( image, ehdr, phdr, process,
                                          entry, max ) ) != 0 )
                        return rc;
        }
@@ -206,19 +209,23 @@ int elf_load ( struct image *image, physaddr_t *entry, physaddr_t *max ) {
                [EI_MAG3]       = ELFMAG3,
                [EI_CLASS]      = ELFCLASS,
        };
-       Elf_Ehdr ehdr;
+       const Elf_Ehdr *ehdr;
        int rc;
 
        /* Read ELF header */
-       copy_from_user ( &ehdr, image->data, 0, sizeof ( ehdr ) );
-       if ( memcmp ( &ehdr.e_ident[EI_MAG0], e_ident,
-                     sizeof ( e_ident ) ) != 0 ) {
+       if ( image->len < sizeof ( *ehdr ) ) {
+               DBGC ( image, "ELF %s too short for ELF header\n",
+                      image->name );
+               return -ENOEXEC;
+       }
+       ehdr = image->data;
+       if ( memcmp ( ehdr->e_ident, e_ident, sizeof ( e_ident ) ) != 0 ) {
                DBGC ( image, "ELF %s has invalid signature\n", image->name );
                return -ENOEXEC;
        }
 
        /* Load ELF segments into memory */
-       if ( ( rc = elf_segments ( image, &ehdr, elf_load_segment,
+       if ( ( rc = elf_segments ( image, ehdr, elf_load_segment,
                                   entry, max ) ) != 0 )
                return rc;
 

diff --git a/src/include/ipxe/elf.h b/src/include/ipxe/elf.h
index 033c3f7a80d7495aa5f784a8e7936453d78bc320..8e51f710bd7c87b0d018b303d8ff3c5cefbb4394 100644 (file)
--- a/src/include/ipxe/elf.h
+++ b/src/include/ipxe/elf.h
@@ -19,9 +19,10 @@ typedef Elf32_Phdr   Elf_Phdr;
 typedef Elf32_Off      Elf_Off;
 #define ELFCLASS       ELFCLASS32
 
-extern int elf_segments ( struct image *image, Elf_Ehdr *ehdr,
+extern int elf_segments ( struct image *image, const Elf_Ehdr *ehdr,
                          int ( * process ) ( struct image *image,
-                                             Elf_Phdr *phdr, physaddr_t dest ),
+                                             const Elf_Phdr *phdr,
+                                             physaddr_t dest ),
                          physaddr_t *entry, physaddr_t *max );
 extern int elf_load ( struct image *image, physaddr_t *entry, physaddr_t *max );