Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1624674 > unrolled thread

[PATCH 2/4] KASLR: Parse all memmap entries in cmdline

Started byBaoquan He <bhe@redhat.com>
First post2017-04-17 15:40 +0200
Last post2017-04-19 02:10 +0200
Articles 5 — 2 participants

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [PATCH 2/4] KASLR: Parse all memmap entries in cmdline Baoquan He <bhe@redhat.com> - 2017-04-17 15:40 +0200
    Re: [PATCH 2/4] KASLR: Parse all memmap entries in cmdline Kees Cook <keescook@chromium.org> - 2017-04-18 22:30 +0200
      Re: [PATCH 2/4] KASLR: Parse all memmap entries in cmdline Baoquan He <bhe@redhat.com> - 2017-04-19 01:00 +0200
        Re: [PATCH 2/4] KASLR: Parse all memmap entries in cmdline Kees Cook <keescook@chromium.org> - 2017-04-19 01:40 +0200
          Re: [PATCH 2/4] KASLR: Parse all memmap entries in cmdline Baoquan He <bhe@redhat.com> - 2017-04-19 02:10 +0200

#1624674 — [PATCH 2/4] KASLR: Parse all memmap entries in cmdline

FromBaoquan He <bhe@redhat.com>
Date2017-04-17 15:40 +0200
Subject[PATCH 2/4] KASLR: Parse all memmap entries in cmdline
Message-ID<txf2O-1br-33@gated-at.bofh.it>
In commit f28442497b5c ("x86/boot: Fix KASLR and memmap= collision"),
memmap= option is parsed so that kaslr can avoid those reserved
regions. It uses cmdline_find_option to get the value if memmap=
is specified, however the problem is cmdline_find_option can only
find the last entry if multiple memmap entries are provided. This
is not correct.

In this patch, the whole cmdline will be scanned to search each
memmap, all of them will be parsed and handled.

Signed-off-by: Baoquan He <bhe@redhat.com>
Cc: "H. Peter Anvin" <hpa@zytor.com>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Ingo Molnar <mingo@redhat.com>
Cc: x86@kernel.org
Cc: Kees Cook <keescook@chromium.org>
Cc: Baoquan He <bhe@redhat.com>
Cc: Yinghai Lu <yinghai@kernel.org>
Cc: Borislav Petkov <bp@suse.de>
---
 arch/x86/boot/compressed/cmdline.c |   2 +-
 arch/x86/boot/compressed/kaslr.c   | 112 ++++++++++++++++++-------------------
 arch/x86/boot/string.c             |   8 +++
 3 files changed, 65 insertions(+), 57 deletions(-)

diff --git a/arch/x86/boot/compressed/cmdline.c b/arch/x86/boot/compressed/cmdline.c
index 73ccf63..9dc1ce6 100644
--- a/arch/x86/boot/compressed/cmdline.c
+++ b/arch/x86/boot/compressed/cmdline.c
@@ -13,7 +13,7 @@ static inline char rdfs8(addr_t addr)
 	return *((char *)(fs + addr));
 }
 #include "../cmdline.c"
-static unsigned long get_cmd_line_ptr(void)
+unsigned long get_cmd_line_ptr(void)
 {
 	unsigned long cmd_line_ptr = boot_params->hdr.cmd_line_ptr;
 
diff --git a/arch/x86/boot/compressed/kaslr.c b/arch/x86/boot/compressed/kaslr.c
index 8b7c9e7..36ab429 100644
--- a/arch/x86/boot/compressed/kaslr.c
+++ b/arch/x86/boot/compressed/kaslr.c
@@ -9,14 +9,16 @@
  * contain the entire properly aligned running kernel image.
  *
  */
+
+#define BOOT_CTYPE_H
 #include "misc.h"
 #include "error.h"
-#include "../boot.h"
 
 #include <generated/compile.h>
 #include <linux/module.h>
 #include <linux/uts.h>
 #include <linux/utsname.h>
+#include <linux/ctype.h>
 #include <generated/utsrelease.h>
 
 /* Simplified build-specific string for starting entropy. */
@@ -61,6 +63,9 @@ struct mem_vector {
 #define MAX_MEMMAP_REGIONS	4
 
 static bool memmap_too_large;
+int mem_avoid_memmap_index;
+extern unsigned long get_cmd_line_ptr(void);
+
 
 enum mem_avoid_index {
 	MEM_AVOID_ZO_RANGE = 0,
@@ -85,49 +90,14 @@ static bool mem_overlaps(struct mem_vector *one, struct mem_vector *two)
 	return true;
 }
 
-/**
- *	_memparse - Parse a string with mem suffixes into a number
- *	@ptr: Where parse begins
- *	@retptr: (output) Optional pointer to next char after parse completes
- *
- *	Parses a string into a number.  The number stored at @ptr is
- *	potentially suffixed with K, M, G, T, P, E.
- */
-static unsigned long long _memparse(const char *ptr, char **retptr)
+char *skip_spaces(const char *str)
 {
-	char *endptr;	/* Local pointer to end of parsed string */
-
-	unsigned long long ret = simple_strtoull(ptr, &endptr, 0);
-
-	switch (*endptr) {
-	case 'E':
-	case 'e':
-		ret <<= 10;
-	case 'P':
-	case 'p':
-		ret <<= 10;
-	case 'T':
-	case 't':
-		ret <<= 10;
-	case 'G':
-	case 'g':
-		ret <<= 10;
-	case 'M':
-	case 'm':
-		ret <<= 10;
-	case 'K':
-	case 'k':
-		ret <<= 10;
-		endptr++;
-	default:
-		break;
-	}
-
-	if (retptr)
-		*retptr = endptr;
-
-	return ret;
+	while (isspace(*str))
+		++str;
+	return (char *)str;
 }
+#include "../../../../lib/ctype.c"
+#include "../../../../lib/cmdline.c"
 
 static int
 parse_memmap(char *p, unsigned long long *start, unsigned long long *size)
@@ -142,40 +112,33 @@ parse_memmap(char *p, unsigned long long *start, unsigned long long *size)
 		return -EINVAL;
 
 	oldp = p;
-	*size = _memparse(p, &p);
+	*size = memparse(p, &p);
 	if (p == oldp)
 		return -EINVAL;
 
 	switch (*p) {
 	case '@':
 		/* Skip this region, usable */
-		*start = 0;
 		*size = 0;
-		return 0;
+		*start = 0;
 	case '#':
 	case '$':
 	case '!':
-		*start = _memparse(p + 1, &p);
+		*start = memparse(p + 1, &p);
 		return 0;
 	}
 
 	return -EINVAL;
 }
 
-static void mem_avoid_memmap(void)
+static void mem_avoid_memmap(char *str)
 {
-	char arg[128];
 	int rc;
-	int i;
-	char *str;
+	int i = mem_avoid_memmap_index;
 
-	/* See if we have any memmap areas */
-	rc = cmdline_find_option("memmap", arg, sizeof(arg));
-	if (rc <= 0)
+	if (i >= MAX_MEMMAP_REGIONS)
 		return;
 
-	i = 0;
-	str = arg;
 	while (str && (i < MAX_MEMMAP_REGIONS)) {
 		int rc;
 		unsigned long long start, size;
@@ -196,12 +159,49 @@ static void mem_avoid_memmap(void)
 		mem_avoid[MEM_AVOID_MEMMAP_BEGIN + i].size = size;
 		i++;
 	}
+	mem_avoid_memmap_index = i;
 
 	/* More than 4 memmaps, fail kaslr */
 	if ((i >= MAX_MEMMAP_REGIONS) && str)
 		memmap_too_large = true;
 }
 
+#define COMMAND_LINE_SIZE 256
+static int handle_mem_memmap(void)
+{
+	char *args = (char *)get_cmd_line_ptr();
+	char tmp_cmdline[COMMAND_LINE_SIZE];
+	size_t len = strlen((char *)args);
+	char *param, *val;
+
+	len = (len >= COMMAND_LINE_SIZE) ? COMMAND_LINE_SIZE - 1 : len;
+	memcpy(tmp_cmdline, args, len);
+	tmp_cmdline[len] = 0;
+	args = tmp_cmdline;
+
+	/* Chew leading spaces */
+	args = skip_spaces(args);
+
+	while (*args) {
+		int ret;
+
+		debug_putstr(args);
+		debug_putstr("\n");
+
+		args = next_arg(args, &param, &val);
+		/* Stop at -- */
+		if (!val && strcmp(param, "--") == 0) {
+			warn("Only '--' specified in cmdline");
+			return -1;
+		}
+
+		if (!strcmp(param, "memmap"))
+			mem_avoid_memmap(val);
+	}
+
+	return 0;
+}
+
 /*
  * In theory, KASLR can put the kernel anywhere in the range of [16M, 64T).
  * The mem_avoid array is used to store the ranges that need to be avoided
@@ -323,7 +323,7 @@ static void mem_avoid_init(unsigned long input, unsigned long input_size,
 	/* We don't need to set a mapping for setup_data. */
 
 	/* Mark the memmap regions we need to avoid */
-	mem_avoid_memmap();
+	handle_mem_memmap();
 
 #ifdef CONFIG_X86_VERBOSE_BOOTUP
 	/* Make sure video RAM can be used. */
diff --git a/arch/x86/boot/string.c b/arch/x86/boot/string.c
index 5457b02..630e366 100644
--- a/arch/x86/boot/string.c
+++ b/arch/x86/boot/string.c
@@ -122,6 +122,14 @@ unsigned long long simple_strtoull(const char *cp, char **endp, unsigned int bas
 	return result;
 }
 
+long simple_strtol(const char *cp, char **endp, unsigned int base)
+{
+	if (*cp == '-')
+		return -simple_strtoull(cp + 1, endp, base);
+
+	return simple_strtoull(cp, endp, base);
+}
+
 /**
  * strlen - Find the length of a string
  * @s: The string to be sized
-- 
2.5.5

[toc] | [next] | [standalone]

#1625641

FromKees Cook <keescook@chromium.org>
Date2017-04-18 22:30 +0200
Message-ID<txHV9-29d-29@gated-at.bofh.it>
In reply to#1624674
On Mon, Apr 17, 2017 at 6:34 AM, Baoquan He <bhe@redhat.com> wrote:
> In commit f28442497b5c ("x86/boot: Fix KASLR and memmap= collision"),
> memmap= option is parsed so that kaslr can avoid those reserved
> regions. It uses cmdline_find_option to get the value if memmap=
> is specified, however the problem is cmdline_find_option can only
> find the last entry if multiple memmap entries are provided. This
> is not correct.
>
> In this patch, the whole cmdline will be scanned to search each
> memmap, all of them will be parsed and handled.
>
> Signed-off-by: Baoquan He <bhe@redhat.com>
> Cc: "H. Peter Anvin" <hpa@zytor.com>
> Cc: Thomas Gleixner <tglx@linutronix.de>
> Cc: Ingo Molnar <mingo@redhat.com>
> Cc: x86@kernel.org
> Cc: Kees Cook <keescook@chromium.org>
> Cc: Baoquan He <bhe@redhat.com>
> Cc: Yinghai Lu <yinghai@kernel.org>
> Cc: Borislav Petkov <bp@suse.de>
> ---
>  arch/x86/boot/compressed/cmdline.c |   2 +-
>  arch/x86/boot/compressed/kaslr.c   | 112 ++++++++++++++++++-------------------
>  arch/x86/boot/string.c             |   8 +++
>  3 files changed, 65 insertions(+), 57 deletions(-)
>
> diff --git a/arch/x86/boot/compressed/cmdline.c b/arch/x86/boot/compressed/cmdline.c
> index 73ccf63..9dc1ce6 100644
> --- a/arch/x86/boot/compressed/cmdline.c
> +++ b/arch/x86/boot/compressed/cmdline.c
> @@ -13,7 +13,7 @@ static inline char rdfs8(addr_t addr)
>         return *((char *)(fs + addr));
>  }
>  #include "../cmdline.c"
> -static unsigned long get_cmd_line_ptr(void)
> +unsigned long get_cmd_line_ptr(void)
>  {
>         unsigned long cmd_line_ptr = boot_params->hdr.cmd_line_ptr;
>
> diff --git a/arch/x86/boot/compressed/kaslr.c b/arch/x86/boot/compressed/kaslr.c
> index 8b7c9e7..36ab429 100644
> --- a/arch/x86/boot/compressed/kaslr.c
> +++ b/arch/x86/boot/compressed/kaslr.c
> @@ -9,14 +9,16 @@
>   * contain the entire properly aligned running kernel image.
>   *
>   */
> +
> +#define BOOT_CTYPE_H
>  #include "misc.h"
>  #include "error.h"
> -#include "../boot.h"
>
>  #include <generated/compile.h>
>  #include <linux/module.h>
>  #include <linux/uts.h>
>  #include <linux/utsname.h>
> +#include <linux/ctype.h>
>  #include <generated/utsrelease.h>
>
>  /* Simplified build-specific string for starting entropy. */
> @@ -61,6 +63,9 @@ struct mem_vector {
>  #define MAX_MEMMAP_REGIONS     4
>
>  static bool memmap_too_large;
> +int mem_avoid_memmap_index;
> +extern unsigned long get_cmd_line_ptr(void);
> +
>
>  enum mem_avoid_index {
>         MEM_AVOID_ZO_RANGE = 0,
> @@ -85,49 +90,14 @@ static bool mem_overlaps(struct mem_vector *one, struct mem_vector *two)
>         return true;
>  }
>
> -/**
> - *     _memparse - Parse a string with mem suffixes into a number
> - *     @ptr: Where parse begins
> - *     @retptr: (output) Optional pointer to next char after parse completes
> - *
> - *     Parses a string into a number.  The number stored at @ptr is
> - *     potentially suffixed with K, M, G, T, P, E.
> - */
> -static unsigned long long _memparse(const char *ptr, char **retptr)
> +char *skip_spaces(const char *str)
>  {
> -       char *endptr;   /* Local pointer to end of parsed string */
> -
> -       unsigned long long ret = simple_strtoull(ptr, &endptr, 0);
> -
> -       switch (*endptr) {
> -       case 'E':
> -       case 'e':
> -               ret <<= 10;
> -       case 'P':
> -       case 'p':
> -               ret <<= 10;
> -       case 'T':
> -       case 't':
> -               ret <<= 10;
> -       case 'G':
> -       case 'g':
> -               ret <<= 10;
> -       case 'M':
> -       case 'm':
> -               ret <<= 10;
> -       case 'K':
> -       case 'k':
> -               ret <<= 10;
> -               endptr++;
> -       default:
> -               break;
> -       }
> -
> -       if (retptr)
> -               *retptr = endptr;
> -
> -       return ret;
> +       while (isspace(*str))
> +               ++str;
> +       return (char *)str;
>  }
> +#include "../../../../lib/ctype.c"
> +#include "../../../../lib/cmdline.c"
>
>  static int
>  parse_memmap(char *p, unsigned long long *start, unsigned long long *size)
> @@ -142,40 +112,33 @@ parse_memmap(char *p, unsigned long long *start, unsigned long long *size)
>                 return -EINVAL;
>
>         oldp = p;
> -       *size = _memparse(p, &p);
> +       *size = memparse(p, &p);
>         if (p == oldp)
>                 return -EINVAL;
>
>         switch (*p) {
>         case '@':
>                 /* Skip this region, usable */
> -               *start = 0;
>                 *size = 0;
> -               return 0;
> +               *start = 0;

Is this intentionally falling through? If so, why assign *start at all?

>         case '#':
>         case '$':
>         case '!':
> -               *start = _memparse(p + 1, &p);
> +               *start = memparse(p + 1, &p);
>                 return 0;
>         }
>
>         return -EINVAL;
>  }
>
> -static void mem_avoid_memmap(void)
> +static void mem_avoid_memmap(char *str)
>  {
> -       char arg[128];
>         int rc;
> -       int i;
> -       char *str;
> +       int i = mem_avoid_memmap_index;
>
> -       /* See if we have any memmap areas */
> -       rc = cmdline_find_option("memmap", arg, sizeof(arg));
> -       if (rc <= 0)
> +       if (i >= MAX_MEMMAP_REGIONS)
>                 return;
>
> -       i = 0;
> -       str = arg;
>         while (str && (i < MAX_MEMMAP_REGIONS)) {
>                 int rc;
>                 unsigned long long start, size;
> @@ -196,12 +159,49 @@ static void mem_avoid_memmap(void)
>                 mem_avoid[MEM_AVOID_MEMMAP_BEGIN + i].size = size;
>                 i++;
>         }
> +       mem_avoid_memmap_index = i;
>
>         /* More than 4 memmaps, fail kaslr */
>         if ((i >= MAX_MEMMAP_REGIONS) && str)
>                 memmap_too_large = true;
>  }
>
> +#define COMMAND_LINE_SIZE 256
> +static int handle_mem_memmap(void)
> +{
> +       char *args = (char *)get_cmd_line_ptr();
> +       char tmp_cmdline[COMMAND_LINE_SIZE];

Can't this use a dynamic allocation instead of the 256 limit?

> +       size_t len = strlen((char *)args);
> +       char *param, *val;
> +
> +       len = (len >= COMMAND_LINE_SIZE) ? COMMAND_LINE_SIZE - 1 : len;
> +       memcpy(tmp_cmdline, args, len);
> +       tmp_cmdline[len] = 0;
> +       args = tmp_cmdline;
> +
> +       /* Chew leading spaces */
> +       args = skip_spaces(args);
> +
> +       while (*args) {
> +               int ret;
> +
> +               debug_putstr(args);
> +               debug_putstr("\n");

Are these accidentally left over?

> +
> +               args = next_arg(args, &param, &val);
> +               /* Stop at -- */
> +               if (!val && strcmp(param, "--") == 0) {
> +                       warn("Only '--' specified in cmdline");
> +                       return -1;
> +               }
> +
> +               if (!strcmp(param, "memmap"))
> +                       mem_avoid_memmap(val);
> +       }
> +
> +       return 0;
> +}
> +
>  /*
>   * In theory, KASLR can put the kernel anywhere in the range of [16M, 64T).
>   * The mem_avoid array is used to store the ranges that need to be avoided
> @@ -323,7 +323,7 @@ static void mem_avoid_init(unsigned long input, unsigned long input_size,
>         /* We don't need to set a mapping for setup_data. */
>
>         /* Mark the memmap regions we need to avoid */
> -       mem_avoid_memmap();
> +       handle_mem_memmap();
>
>  #ifdef CONFIG_X86_VERBOSE_BOOTUP
>         /* Make sure video RAM can be used. */
> diff --git a/arch/x86/boot/string.c b/arch/x86/boot/string.c
> index 5457b02..630e366 100644
> --- a/arch/x86/boot/string.c
> +++ b/arch/x86/boot/string.c
> @@ -122,6 +122,14 @@ unsigned long long simple_strtoull(const char *cp, char **endp, unsigned int bas
>         return result;
>  }
>
> +long simple_strtol(const char *cp, char **endp, unsigned int base)
> +{
> +       if (*cp == '-')
> +               return -simple_strtoull(cp + 1, endp, base);
> +
> +       return simple_strtoull(cp, endp, base);
> +}
> +
>  /**
>   * strlen - Find the length of a string
>   * @s: The string to be sized
> --
> 2.5.5
>

Otherwise, yeah, this looks sensible.

-Kees

-- 
Kees Cook
Pixel Security

[toc] | [prev] | [next] | [standalone]

#1625687

FromBaoquan He <bhe@redhat.com>
Date2017-04-19 01:00 +0200
Message-ID<txKgi-3qF-7@gated-at.bofh.it>
In reply to#1625641
Hi Kees,

Thanks for your reviewing!

On 04/18/17 at 01:22pm, Kees Cook wrote:

> >  static int
> >  parse_memmap(char *p, unsigned long long *start, unsigned long long *size)
> > @@ -142,40 +112,33 @@ parse_memmap(char *p, unsigned long long *start, unsigned long long *size)
> >                 return -EINVAL;
> >
> >         oldp = p;
> > -       *size = _memparse(p, &p);
> > +       *size = memparse(p, &p);
> >         if (p == oldp)
> >                 return -EINVAL;
> >
> >         switch (*p) {
> >         case '@':
> >                 /* Skip this region, usable */
> > -               *start = 0;
> >                 *size = 0;
> > -               return 0;
> > +               *start = 0;
> 
> Is this intentionally falling through? If so, why assign *start at all?

OOPS, this is a mistake when I split patch. Here it should not be
changed in this patch though code change is OK with patch 3/4 together.

Will change that.

> 
> >         case '#':
> >         case '$':
> >         case '!':
> > -               *start = _memparse(p + 1, &p);
> > +               *start = memparse(p + 1, &p);
> >                 return 0;
> >         }
> >
> >         return -EINVAL;
> >  }
> >
> > -static void mem_avoid_memmap(void)
> > +static void mem_avoid_memmap(char *str)
> >  {
> > -       char arg[128];
> >         int rc;
> > -       int i;
> > -       char *str;
> > +       int i = mem_avoid_memmap_index;
> >
> > -       /* See if we have any memmap areas */
> > -       rc = cmdline_find_option("memmap", arg, sizeof(arg));
> > -       if (rc <= 0)
> > +       if (i >= MAX_MEMMAP_REGIONS)
> >                 return;
> >
> > -       i = 0;
> > -       str = arg;
> >         while (str && (i < MAX_MEMMAP_REGIONS)) {
> >                 int rc;
> >                 unsigned long long start, size;
> > @@ -196,12 +159,49 @@ static void mem_avoid_memmap(void)
> >                 mem_avoid[MEM_AVOID_MEMMAP_BEGIN + i].size = size;
> >                 i++;
> >         }
> > +       mem_avoid_memmap_index = i;
> >
> >         /* More than 4 memmaps, fail kaslr */
> >         if ((i >= MAX_MEMMAP_REGIONS) && str)
> >                 memmap_too_large = true;
> >  }
> >
> > +#define COMMAND_LINE_SIZE 256
> > +static int handle_mem_memmap(void)
> > +{
> > +       char *args = (char *)get_cmd_line_ptr();
> > +       char tmp_cmdline[COMMAND_LINE_SIZE];
> 
> Can't this use a dynamic allocation instead of the 256 limit?

This is in boot/compressed code, no mm allocator built yet? Am I right?

> 
> > +       size_t len = strlen((char *)args);
> > +       char *param, *val;
> > +
> > +       len = (len >= COMMAND_LINE_SIZE) ? COMMAND_LINE_SIZE - 1 : len;
> > +       memcpy(tmp_cmdline, args, len);
> > +       tmp_cmdline[len] = 0;
> > +       args = tmp_cmdline;
> > +
> > +       /* Chew leading spaces */
> > +       args = skip_spaces(args);
> > +
> > +       while (*args) {
> > +               int ret;
> > +
> > +               debug_putstr(args);
> > +               debug_putstr("\n");
> 
> Are these accidentally left over?

Yes, it's for debugging. Will remove.

Thanks
Baoquan

> 
> > +
> > +               args = next_arg(args, &param, &val);
> > +               /* Stop at -- */
> > +               if (!val && strcmp(param, "--") == 0) {
> > +                       warn("Only '--' specified in cmdline");
> > +                       return -1;
> > +               }
> > +
> > +               if (!strcmp(param, "memmap"))
> > +                       mem_avoid_memmap(val);
> > +       }
> > +
> > +       return 0;
> > +}
> > +
> >  /*
> >   * In theory, KASLR can put the kernel anywhere in the range of [16M, 64T).
> >   * The mem_avoid array is used to store the ranges that need to be avoided
> > @@ -323,7 +323,7 @@ static void mem_avoid_init(unsigned long input, unsigned long input_size,
> >         /* We don't need to set a mapping for setup_data. */
> >
> >         /* Mark the memmap regions we need to avoid */
> > -       mem_avoid_memmap();
> > +       handle_mem_memmap();
> >
> >  #ifdef CONFIG_X86_VERBOSE_BOOTUP
> >         /* Make sure video RAM can be used. */
> > diff --git a/arch/x86/boot/string.c b/arch/x86/boot/string.c
> > index 5457b02..630e366 100644
> > --- a/arch/x86/boot/string.c
> > +++ b/arch/x86/boot/string.c
> > @@ -122,6 +122,14 @@ unsigned long long simple_strtoull(const char *cp, char **endp, unsigned int bas
> >         return result;
> >  }
> >
> > +long simple_strtol(const char *cp, char **endp, unsigned int base)
> > +{
> > +       if (*cp == '-')
> > +               return -simple_strtoull(cp + 1, endp, base);
> > +
> > +       return simple_strtoull(cp, endp, base);
> > +}
> > +
> >  /**
> >   * strlen - Find the length of a string
> >   * @s: The string to be sized
> > --
> > 2.5.5
> >
> 
> Otherwise, yeah, this looks sensible.
> 
> -Kees
> 
> -- 
> Kees Cook
> Pixel Security

[toc] | [prev] | [next] | [standalone]

#1625729

FromKees Cook <keescook@chromium.org>
Date2017-04-19 01:40 +0200
Message-ID<txKT0-3YV-15@gated-at.bofh.it>
In reply to#1625687
On Tue, Apr 18, 2017 at 3:52 PM, Baoquan He <bhe@redhat.com> wrote:
> On 04/18/17 at 01:22pm, Kees Cook wrote:
>> > +#define COMMAND_LINE_SIZE 256
>> > +static int handle_mem_memmap(void)
>> > +{
>> > +       char *args = (char *)get_cmd_line_ptr();
>> > +       char tmp_cmdline[COMMAND_LINE_SIZE];
>>
>> Can't this use a dynamic allocation instead of the 256 limit?
>
> This is in boot/compressed code, no mm allocator built yet? Am I right?

misc.c uses malloc for phdrs, and the boot_heap is create to build an
area for those calls, see include/linux/decompress/mm.h. I *think* it
should be safe to use malloc here. It should be a pretty small
allocation normally.

-Kees

-- 
Kees Cook
Pixel Security

[toc] | [prev] | [next] | [standalone]

#1625739

FromBaoquan He <bhe@redhat.com>
Date2017-04-19 02:10 +0200
Message-ID<txLm2-4su-9@gated-at.bofh.it>
In reply to#1625729
On 04/18/17 at 04:32pm, Kees Cook wrote:
> On Tue, Apr 18, 2017 at 3:52 PM, Baoquan He <bhe@redhat.com> wrote:
> > On 04/18/17 at 01:22pm, Kees Cook wrote:
> >> > +#define COMMAND_LINE_SIZE 256
> >> > +static int handle_mem_memmap(void)
> >> > +{
> >> > +       char *args = (char *)get_cmd_line_ptr();
> >> > +       char tmp_cmdline[COMMAND_LINE_SIZE];
> >>
> >> Can't this use a dynamic allocation instead of the 256 limit?
> >
> > This is in boot/compressed code, no mm allocator built yet? Am I right?
> 
> misc.c uses malloc for phdrs, and the boot_heap is create to build an
> area for those calls, see include/linux/decompress/mm.h. I *think* it
> should be safe to use malloc here. It should be a pretty small
> allocation normally.

Yes, didn't notice this. Will use it to do dynamic malloc.
Thanks for telling!

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web