Subject: Re: [PATCH v2 1/3] KASLR: Parse all memmap entries in cmdline
To: Baoquan He <bhe@redhat.com>, <linux-kernel@vger.kernel.org>,
        <keescook@chromium.org>, <mingo@kernel.org>
References: <1493001650-5793-1-git-send-email-bhe@redhat.com>
 <1493001650-5793-2-git-send-email-bhe@redhat.com>
CC: <dave.jiang@intel.com>, <dan.j.williams@intel.com>, <hpa@zytor.com>,
        <tglx@linutronix.de>, <dyoung@redhat.com>,
        Ingo Molnar <mingo@redhat.com>, <x86@kernel.org>,
        Yinghai Lu <yinghai@kernel.org>, Borislav Petkov <bp@suse.de>
From: Dou Liyang <douly.fnst@cn.fujitsu.com>
Message-ID: <9d662e87-bd4e-6cfd-f27b-e971e25ec0ca@cn.fujitsu.com>
Date: Mon, 24 Apr 2017 16:00:14 +0800
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101
 Thunderbird/45.4.0
MIME-Version: 1.0
In-Reply-To: <1493001650-5793-2-git-send-email-bhe@redhat.com>
Content-Type: text/plain; charset="gbk"; format=flowed
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 4044
Lines: 150

Hi Baoquan,

At 04/24/2017 10:40 AM, Baoquan He 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.
>
[...]
>
> -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;

Is it better that we make that variable *static* to remove the global
variable(mem_avoid_memmap_index)?

-       int i = mem_avoid_memmap_index;
+       static int i;

         if (i >= MAX_MEMMAP_REGIONS)
                 return;
@@ -172,7 +172,6 @@ static void mem_avoid_memmap(char *str)
                 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)


>
> -	/* 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 guess we just parsed and handled 4 MEMMAP_REGIONS and might ignore
the following in the whole cmdline.

Is it reasonable?  Is there any priority? The smaller the size, the more 
priority?

Thanks,
	Liyang

> -	i = 0;
> -	str = arg;
>  	while (str && (i < MAX_MEMMAP_REGIONS)) {
>  		int rc;
>  		unsigned long long start, size;
> @@ -196,12 +172,55 @@ 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;
>  }
>
> +
> +/* Macros used by the included decompressor code below. */
> +#define STATIC
> +#include <linux/decompress/mm.h>
> +
> +#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;
> +
> +	tmp_cmdline = malloc(COMMAND_LINE_SIZE);
> +	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, &param, &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 +342,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
>