[image] Check delimiters when parsing command-line key-value arguments

The Linux kernel bzImage image format and the CPIO archive constructor
will parse the image command line for certain arguments of the form
"key=value".  This parsing is currently implemented using strstr() in
a way that can cause a false positive suffix match.  For example, a
command line containing "highmem=<n>" would erroneously be treated as
containing a value for "mem=<n>".

Fix by centralising the logic used for parsing such arguments, and
including a check that the argument immediately follows a whitespace
delimiter (or is at the start of the string).

Reported-by: Filippo Giunchedi <filippo@esaurito.net>
Signed-off-by: Michael Brown <mcb30@ipxe.org>

diff --git a/src/arch/x86/image/bzimage.c b/src/arch/x86/image/bzimage.c
index 769e0c9d8f8f9018307e783135e601f21e8ab362..b40bb2d070ebb9a3dc3318aa2cdef8e2a4d30af5 100644 (file)
--- a/src/arch/x86/image/bzimage.c
+++ b/src/arch/x86/image/bzimage.c
@@ -247,19 +247,17 @@ static void bzimage_update_header ( struct image *image,
  *
  * @v image            bzImage file
  * @v bzimg            bzImage context
- * @v cmdline          Kernel command line
  * @ret rc             Return status code
  */
 static int bzimage_parse_cmdline ( struct image *image,
-                                  struct bzimage_context *bzimg,
-                                  char *cmdline ) {
+                                  struct bzimage_context *bzimg ) {
+       const char *vga;
+       const char *mem;
        char *sep;
-       char *vga;
-       char *mem;
+       char *end;
 
        /* Look for "vga=" */
-       if ( ( vga = strstr ( cmdline, "vga=" ) ) ) {
-               vga += 4;
+       if ( ( vga = image_argument ( image, "vga=" ) ) ) {
                sep = strchr ( vga, ' ' );
                if ( sep )
                        *sep = '\0';
@@ -270,10 +268,10 @@ static int bzimage_parse_cmdline ( struct image *image,
                } else if ( strcmp ( vga, "ask" ) == 0 ) {
                        bzimg->vid_mode = BZI_VID_MODE_ASK;
                } else {
-                       bzimg->vid_mode = strtoul ( vga, &vga, 0 );
-                       if ( *vga ) {
+                       bzimg->vid_mode = strtoul ( vga, &end, 0 );
+                       if ( *end ) {
                                DBGC ( image, "bzImage %p strange \"vga=\" "
-                                      "terminator '%c'\n", image, *vga );
+                                      "terminator '%c'\n", image, *end );
                        }
                }
                if ( sep )
@@ -281,10 +279,9 @@ static int bzimage_parse_cmdline ( struct image *image,
        }
 
        /* Look for "mem=" */
-       if ( ( mem = strstr ( cmdline, "mem=" ) ) ) {
-               mem += 4;
-               bzimg->mem_limit = strtoul ( mem, &mem, 0 );
-               switch ( *mem ) {
+       if ( ( mem = image_argument ( image, "mem=" ) ) ) {
+               bzimg->mem_limit = strtoul ( mem, &end, 0 );
+               switch ( *end ) {
                case 'G':
                case 'g':
                        bzimg->mem_limit <<= 10;
@@ -302,7 +299,7 @@ static int bzimage_parse_cmdline ( struct image *image,
                        break;
                default:
                        DBGC ( image, "bzImage %p strange \"mem=\" "
-                              "terminator '%c'\n", image, *mem );
+                              "terminator '%c'\n", image, *end );
                        break;
                }
                bzimg->mem_limit -= 1;
@@ -316,11 +313,10 @@ static int bzimage_parse_cmdline ( struct image *image,
  *
  * @v image            bzImage image
  * @v bzimg            bzImage context
- * @v cmdline          Kernel command line
  */
 static void bzimage_set_cmdline ( struct image *image,
-                                 struct bzimage_context *bzimg,
-                                 const char *cmdline ) {
+                                 struct bzimage_context *bzimg ) {
+       const char *cmdline = ( image->cmdline ? image->cmdline : "" );
        size_t cmdline_len;
 
        /* Copy command line down to real-mode portion */
@@ -528,7 +524,6 @@ static void bzimage_load_initrds ( struct image *image,
  */
 static int bzimage_exec ( struct image *image ) {
        struct bzimage_context bzimg;
-       char *cmdline = ( image->cmdline ? image->cmdline : "" );
        int rc;
 
        /* Read and parse header from image */
@@ -551,7 +546,7 @@ static int bzimage_exec ( struct image *image ) {
        }
 
        /* Parse command line for bootloader parameters */
-       if ( ( rc = bzimage_parse_cmdline ( image, &bzimg, cmdline ) ) != 0)
+       if ( ( rc = bzimage_parse_cmdline ( image, &bzimg ) ) != 0)
                return rc;
 
        /* Check that initrds can be loaded */
@@ -568,7 +563,7 @@ static int bzimage_exec ( struct image *image ) {
                      bzimg.rm_filesz, bzimg.pm_sz );
 
        /* Store command line */
-       bzimage_set_cmdline ( image, &bzimg, cmdline );
+       bzimage_set_cmdline ( image, &bzimg );
 
        /* Prepare for exiting.  Must do this before loading initrds,
         * since loading the initrds will corrupt the external heap.

diff --git a/src/core/cpio.c b/src/core/cpio.c
index 27aee75817c4707ad1a9e77c1bb86742821565d8..4b607e260bbd99ed9f75acd6bb6092477af456c4 100644 (file)
--- a/src/core/cpio.c
+++ b/src/core/cpio.c
@@ -77,17 +77,12 @@ size_t cpio_name_len ( struct image *image ) {
  */
 static void cpio_parse_cmdline ( struct image *image,
                                 struct cpio_header *cpio ) {
-       const char *cmdline;
-       char *arg;
+       const char *arg;
        char *end;
        unsigned int mode;
 
-       /* Skip image filename */
-       cmdline = ( cpio_name ( image ) + cpio_name_len ( image ) );
-
        /* Look for "mode=" */
-       if ( ( arg = strstr ( cmdline, "mode=" ) ) ) {
-               arg += 5;
+       if ( ( arg = image_argument ( image, "mode=" ) ) ) {
                mode = strtoul ( arg, &end, 8 /* Octal for file mode */ );
                if ( *end && ( *end != ' ' ) ) {
                        DBGC ( image, "CPIO %p strange \"mode=\" "

diff --git a/src/core/image.c b/src/core/image.c
index 3e236ca603bd6b75618ffce20561a3a417259740..f6d3d8ddd8e89573e67ccb2520cc303c6ae46456 100644 (file)
--- a/src/core/image.c
+++ b/src/core/image.c
@@ -27,6 +27,7 @@ FILE_LICENCE ( GPL2_OR_LATER_OR_UBDL );
 #include <string.h>
 #include <stdlib.h>
 #include <stdio.h>
+#include <ctype.h>
 #include <errno.h>
 #include <assert.h>
 #include <libgen.h>
@@ -569,3 +570,33 @@ struct image * image_memory ( const char *name, userptr_t data, size_t len ) {
  err_alloc_image:
        return NULL;
 }
+
+/**
+ * Find argument within image command line
+ *
+ * @v image            Image
+ * @v key              Argument search key (including trailing delimiter)
+ * @ret value          Argument value, or NULL if not found
+ */
+const char * image_argument ( struct image *image, const char *key ) {
+       const char *cmdline = image->cmdline;
+       const char *search;
+       const char *match;
+       const char *next;
+
+       /* Find argument */
+       for ( search = cmdline ; search ; search = next ) {
+
+               /* Find next occurrence, if any */
+               match = strstr ( search, key );
+               if ( ! match )
+                       break;
+               next = ( match + strlen ( key ) );
+
+               /* Check preceding delimiter, if any */
+               if ( ( match == cmdline ) || isspace ( match[-1] ) )
+                       return next;
+       }
+
+       return NULL;
+}

diff --git a/src/include/ipxe/image.h b/src/include/ipxe/image.h
index 0a5a2603462293576bf7a41f9fd09a5561bab179..9e0c0f22a5b7b8286a8166805d118ee239fa6d71 100644 (file)
--- a/src/include/ipxe/image.h
+++ b/src/include/ipxe/image.h
@@ -195,6 +195,7 @@ extern struct image * image_find_selected ( void );
 extern int image_set_trust ( int require_trusted, int permanent );
 extern struct image * image_memory ( const char *name, userptr_t data,
                                     size_t len );
+extern const char * image_argument ( struct image *image, const char *key );
 extern int image_pixbuf ( struct image *image, struct pixel_buffer **pixbuf );
 extern int image_asn1 ( struct image *image, size_t offset,
                        struct asn1_cursor **cursor );