[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>

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,
+				   struct bzimage_context *bzimg ) {
-				   char *cmdline ) {
+	const char *vga;
+	const char *mem;
 	char *sep;
-	char *vga;
+	char *end;
-	char *mem;
 
 	/* Look for "vga=" */
-	if ( ( vga = strstr ( cmdline, "vga=" ) ) ) {
+	if ( ( vga = image_argument ( image, "vga=" ) ) ) {
-		vga += 4;
 		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 );
+			bzimg->vid_mode = strtoul ( vga, &end, 0 );
-			if ( *vga ) {
+			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=" ) ) ) {
+	if ( ( mem = image_argument ( image, "mem=" ) ) ) {
-		mem += 4;
+		bzimg->mem_limit = strtoul ( mem, &end, 0 );
-		bzimg->mem_limit = strtoul ( mem, &mem, 0 );
+		switch ( *end ) {
-		switch ( *mem ) {
 		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,
+				  struct bzimage_context *bzimg ) {
-				  const char *cmdline ) {
+	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.

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;
+	const char *arg;
-	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=" ) ) ) {
+	if ( ( arg = image_argument ( image, "mode=" ) ) ) {
-		arg += 5;
 		mode = strtoul ( arg, &end, 8 /* Octal for file mode */ );
 		if ( *end && ( *end != ' ' ) ) {
 			DBGC ( image, "CPIO %p strange \"mode=\" "

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;
+}

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 );