| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <20170429101241.GB12873@x1>
Date: Sat, 29 Apr 2017 18:12:41 +0800
From: Baoquan He <bhe@...hat.com>
To: Kees Cook <keescook@...omium.org>
Cc: Ingo Molnar <mingo@...nel.org>,
LKML <linux-kernel@...r.kernel.org>,
Dave Young <dyoung@...hat.com>, douly.fnst@...fujitsu.com,
Dan Williams <dan.j.williams@...el.com>,
"H. Peter Anvin" <hpa@...or.com>,
Thomas Gleixner <tglx@...utronix.de>,
Ingo Molnar <mingo@...hat.com>,
"x86@...nel.org" <x86@...nel.org>, Yinghai Lu <yinghai@...nel.org>,
Borislav Petkov <bp@...e.de>
Subject: Re: [PATCH v3 1/3] KASLR: Parse all memmap entries in cmdline
On 04/28/17 at 12:37pm, Kees Cook wrote:
> On Wed, Apr 26, 2017 at 3:16 AM, Baoquan He <bhe@...hat.com> wrote:
> > In commit:
> >
> > f28442497b5c ("x86/boot: Fix KASLR and memmap= collision")
> >
> > ... the 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 that 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@...hat.com>
> > Cc: "H. Peter Anvin" <hpa@...or.com>
> > Cc: Thomas Gleixner <tglx@...utronix.de>
> > Cc: Ingo Molnar <mingo@...hat.com>
> > Cc: x86@...nel.org
> > Cc: Kees Cook <keescook@...omium.org>
> > Cc: Yinghai Lu <yinghai@...nel.org>
> > Cc: Borislav Petkov <bp@...e.de>
> > ---
> > arch/x86/boot/compressed/cmdline.c | 2 +-
> > arch/x86/boot/compressed/kaslr.c | 125 +++++++++++++++++++++----------------
> > arch/x86/boot/string.c | 8 +++
> > 3 files changed, 80 insertions(+), 55 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 6d9a546..53a06ec 100644
> > --- a/arch/x86/boot/compressed/kaslr.c
> > +++ b/arch/x86/boot/compressed/kaslr.c
> > @@ -9,14 +9,28 @@
> > * contain the entire properly aligned running kernel image.
> > *
> > */
> > +
> > +/*
> > + * linux/ctype.h is expected, while conflicts with boot/ctype.h
> > + * which is useless here. Do not include it.
> > + */
> > +#define BOOT_CTYPE_H
>
> Which include includes this?
Thanks for your reviewing, Kees!
misc.h includes it.
Function isdigit is implemented in boot/ctype.h. And with <linux/ctype.h>,
they will conflict. While next_args() need isspace() in linux/ctype.h to
filter out "space/lf/tab".
>
> > +
> > +/*
> > + * Exporting symbol in boot stage is meaningless, and will trigger
> > + * compiling error in some cases. Disable it here.
> > + */
> > +#define _LINUX_EXPORT_H
> > +#define EXPORT_SYMBOL(sym)
>
> And this?
If I want isspace() in linux/ctype.h, _ctype[] in lib/ctype.c is needed.
Meanwhile since next_args() is moved to lib/cmdline.c, including
lib/cmdline.c will bring EXPORT_SYMBOL. And including lib/cmdline.c can
provide a ready-made memparse() which makes us remove the redundent one
in kaslr.c.
>
> > +
> > #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>
>
> Why is misc.h's ctype insufficient?
misc.h's ctype doesn't provide a isspace which next_args() need. There's
a myisspace in boot/cmdline.c, I think it's insufficient, it only check
space.
static inline int myisspace(u8 c)
{
return c <= ' '; /* Close enough approximation */
}
>
> > #include <generated/utsrelease.h>
> >
> > /* Simplified build-specific string for starting entropy. */
> > @@ -61,6 +75,8 @@ struct mem_vector {
> > #define MAX_MEMMAP_REGIONS 4
> >
> > static bool memmap_too_large;
> > +extern unsigned long get_cmd_line_ptr(void);
>
> Can we avoid an extern in .c? Seems like this should be defined in
> misc.h or similar?
Yes, it shoud be put in msic.h. I thought putting it here can avoid
polluting the name space. Now I think you are right though now it's only
used by kaslr.c and compressed/cmdline.c where it's defined.
>
> > +
> >
> > enum mem_avoid_index {
> > MEM_AVOID_ZO_RANGE = 0,
> > @@ -85,49 +101,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,7 +123,7 @@ 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;
> >
> > @@ -155,27 +136,21 @@ parse_memmap(char *p, unsigned long long *start, unsigned long long *size)
> > 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];
> > + static int i;
> > int rc;
> > - int i;
> > - char *str;
> >
> > - /* 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;
> > @@ -202,6 +177,48 @@ static void mem_avoid_memmap(void)
> > memmap_too_large = true;
> > }
> >
> > +
> > +/* Macros used by the included decompressor code below. */
> > +#define STATIC
> > +#include <linux/decompress/mm.h>
>
> Can this be moved to the top of the file? (Also, I wonder if maybe it
> should be moved into misc.h?)
Yes, it can be moved to the top of kaslr.c
In misc.c those lib/decompress_xxx.c include linux/decompress/mm.h
already, and define STATIC as static. But I think it's fine to put it in
misc.h. Let me try.
>
> > +
> > +#define COMMAND_LINE_SIZE 256
> > +static int handle_mem_memmap(void)
> > +{
> > + char *args = (char *)get_cmd_line_ptr();
> > + size_t len = strlen((char *)args);
> > + char *tmp_cmdline;
> > + char *param, *val;
> > +
>
> Here to optimize for the common case, please do something like:
>
> if (!strstr("memmap=", args))
> return 0;
Hmm, handle_mem_memmap will be a common handler for memmap= and mem=
options. If here we use strstr to check it, do we need to check "mem="
too?
>
> > + tmp_cmdline = malloc(COMMAND_LINE_SIZE);
>
> Instead of COMMAND_LINE_SIZE hard-limit, please attempt a malloc of
> len + 1. Then you don't have to play games with len adjustments,
> truncation, etc.
Sure, will do.
>
> > + if (!tmp_cmdline )
> > + error("Failed to allocate space for tmp_cmdline");
> > +
> > + 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) {
> > + args = next_arg(args, ¶m, &val);
> > + /* Stop at -- */
> > + if (!val && strcmp(param, "--") == 0) {
> > + warn("Only '--' specified in cmdline");
> > + free(tmp_cmdline);
> > + return -1;
> > + }
> > +
> > + if (!strcmp(param, "memmap"))
> > + mem_avoid_memmap(val);
> > + }
> > +
> > + free(tmp_cmdline);
> > + 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 +340,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();
>
> The first "mem" in this name seems redundant.
Hmm, in this patch handle_mem_memmap only handles memmap= option, in
patch 2/3 it will handle mem= too. Do you think I should name it
handle_memmap() here, and rename it to handle_mem_memmap() in patch 2/3?
>
> >
> > #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
> >
>
> Looks like it's making progress, thanks!
>
> -Kees
>
> --
> Kees Cook
> Pixel Security
Powered by blists - more mailing lists