DMARC-Filter: OpenDMARC Filter v1.3.2 mx1.redhat.com 9E11E65CEF
Date: Mon, 24 Apr 2017 17:09:42 +0800
From: Baoquan He <bhe@redhat.com>
To: Dou Liyang <douly.fnst@cn.fujitsu.com>
Cc: linux-kernel@vger.kernel.org, keescook@chromium.org, mingo@kernel.org,
        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>
Subject: Re: [PATCH v2 1/3] KASLR: Parse all memmap entries in cmdline
Message-ID: <20170424090942.GD2310@x1>
References: <1493001650-5793-1-git-send-email-bhe@redhat.com>
 <1493001650-5793-2-git-send-email-bhe@redhat.com>
 <9d662e87-bd4e-6cfd-f27b-e971e25ec0ca@cn.fujitsu.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <9d662e87-bd4e-6cfd-f27b-e971e25ec0ca@cn.fujitsu.com>
User-Agent: Mutt/1.7.0 (2016-08-17)
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 4574
Lines: 160

On 04/24/17 at 04:00pm, Dou Liyang wrote:
> 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)?

Yeah, I am fine with it. Can change lik this:

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

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

It's on purpose. Please see the discussion during Dave Jiang's patch
posting.

commit f28442497b5c ("x86/boot: Fix KASLR and memmap= collision")
> 
> 
> > -	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
> > 
> 
>