Re: [PATCH v4] x86: fix kaslr and memmap collision

[Dave Jiang <dave.jiang@intel.com>]

On 01/03/2017 07:37 PM, Baoquan He wrote:
> Hi Dave,
> 
> I have several concerns, please see the inline comments.
> 
> On 01/03/17 at 01:48pm, Dave Jiang wrote:
>> CONFIG_RANDOMIZE_BASE relocates the kernel to a random base address.
>> However it does not take into account the memmap= parameter passed in from
>> the kernel cmdline. This results in the kernel sometimes being put in
>> the middle of the user memmap. Teaching kaslr to not insert the kernel in
> 		    ~~~~~~~~~~~
> It could be better to be replaced with "user-defined physical RAM map"
> or memmap directly because memmap is meaning user-defined physical RAM
> map. Please check the boot info printing about them. I was confused at
> first glance.

Will update

>> memmap defined regions. We will support up to 4 memmap regions. Any
>> additional regions will cause kaslr to disable. The mem_avoid set has
>> been augmented to add up to 4 regions of memmaps provided by the user
>                               ~~~
>     4 un-usable memmap region need be cared, usable memory is not
> concerned

Will update

.
>> to exclude those regions from the set of valid address range to insert
>> the uncompressed kernel image. The nn@ss ranges will be skipped by the
>> mem_avoid set since it indicates memory useable.
>>
>> Signed-off-by: Dave Jiang <dave.jiang@intel.com>
>> ---
>>  arch/x86/boot/boot.h             |    3 +
>>  arch/x86/boot/compressed/kaslr.c |  131 ++++++++++++++++++++++++++++++++++++++
>>  arch/x86/boot/string.c           |   38 +++++++++++
>>  3 files changed, 172 insertions(+)
>>
>> diff --git a/arch/x86/boot/boot.h b/arch/x86/boot/boot.h
>> index e5612f3..59c2075 100644
>> --- a/arch/x86/boot/boot.h
>> +++ b/arch/x86/boot/boot.h
>> @@ -332,7 +332,10 @@ int strncmp(const char *cs, const char *ct, size_t count);
>>  size_t strnlen(const char *s, size_t maxlen);
>>  unsigned int atou(const char *s);
>>  unsigned long long simple_strtoull(const char *cp, char **endp, unsigned int base);
>> +unsigned long simple_strtoul(const char *cp, char **endp, unsigned int base);
>> +long simple_strtol(const char *cp, char **endp, unsigned int base);
>>  size_t strlen(const char *s);
>> +char *strchr(const char *s, int c);
>>  
>>  /* tty.c */
>>  void puts(const char *);
>> diff --git a/arch/x86/boot/compressed/kaslr.c b/arch/x86/boot/compressed/kaslr.c
>> index a66854d..f5a1c8d 100644
>> --- a/arch/x86/boot/compressed/kaslr.c
>> +++ b/arch/x86/boot/compressed/kaslr.c
>> @@ -11,6 +11,7 @@
>>   */
>>  #include "misc.h"
>>  #include "error.h"
>> +#include "../boot.h"
>>  
>>  #include <generated/compile.h>
>>  #include <linux/module.h>
>> @@ -61,9 +62,16 @@ enum mem_avoid_index {
>>  	MEM_AVOID_INITRD,
>>  	MEM_AVOID_CMDLINE,
>>  	MEM_AVOID_BOOTPARAMS,
>> +	MEM_AVOID_MEMMAP1,
>> +	MEM_AVOID_MEMMAP2,
>> +	MEM_AVOID_MEMMAP3,
>> +	MEM_AVOID_MEMMAP4,
> 
> This looks not good. Could it be done like fixed_addresses?
> Something like:
> 
> 	MEM_AVOID_MEMMAP_BEGIN,
> 	MEM_AVOID_MEMMAP_BEGIN + MEM_AVOID_MAX -1,
> 
> Please point it out to me if there's some existing code in kernel like
> your way, I can also accept it.

I think you mean:
MEM_AVOID_MEMMAP_BEGIN + MAX_MEMMAP_REGIONS - 1

I will change


> 
>>  	MEM_AVOID_MAX,
>>  };
>>  
>> +/* only supporting at most 4 memmap regions with kaslr */
> And here, "Only supporting at most 4 un-usable memmap regions with kaslr"?
> Surely this is based on if you will ignore the usable memory and do not
> store it as 0. And also the log need be changed too accordingly.
>> +#define MAX_MEMMAP_REGIONS	4
>> +
>>  static struct mem_vector mem_avoid[MEM_AVOID_MAX];
>>  
>>  static bool mem_overlaps(struct mem_vector *one, struct mem_vector *two)
>> @@ -77,6 +85,121 @@ 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 *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;
>> +}
>> +
>> +static int
>> +parse_memmap(char *p, unsigned long long *start, unsigned long long *size)
>> +{
>> +	char *oldp;
>> +
>> +	if (!p)
>> +		return -EINVAL;
>> +
>> +	/* we don't care about this option here */
>> +	if (!strncmp(p, "exactmap", 8))
>> +		return -EINVAL;
>> +
>> +	oldp = p;
>> +	*size = _memparse(p, &p);
>> +	if (p == oldp)
>> +		return -EINVAL;
>> +
>> +	switch (*p) {
>> +	case '@':
>> +		/* skip this region, usable */
>> +		*start = 0;
>> +		*size = 0;
>> +		return 0;
> How about direclty return if nn@ss? Seems no need to waste one mem avoid
> region slot. In fact even amount of usable memory regions are provided to
> 100, it won't impact that you want to specify a reserve memmap region if
> you skip it direclty. Personal opinion.

We are not wasting the slot. If you look mem_avoid_memmap() where I call
the function, it will skip with a continue if size == 0 without
incrementing the 'i' counter. That will skip all the nn@ss regions
without counting against the max avoid mapping.

>> +	case '#':
>> +	case '$':
>> +	case '!':
>> +		*start = _memparse(p + 1, &p);
>> +		return 0;
>> +	}
>> +
>> +	return -EINVAL;
>> +}
>> +
>> +static int mem_avoid_memmap(void)
>> +{
>> +	char arg[128];
>> +	int rc = 0;
>> +
>> +	/* see if we have any memmap areas */
>> +	if (cmdline_find_option("memmap", arg, sizeof(arg)) > 0) {
>> +		int i = 0;
>> +		char *str = arg;
>> +
>> +		while (str && (i < MAX_MEMMAP_REGIONS)) {
>> +			unsigned long long start, size;
>> +			char *k = strchr(str, ',');
>> +
>> +			if (k)
>> +				*k++ = 0;
>> +
>> +			rc = parse_memmap(str, &start, &size);
>> +			if (rc < 0)
>> +				break;
>> +			str = k;
>> +			/* a usable region that should not be skipped */
>> +			if (size == 0)
>> +				continue;
>> +
>> +			mem_avoid[MEM_AVOID_MEMMAP1 + i].start = start;
>> +			mem_avoid[MEM_AVOID_MEMMAP1 + i].size = size;
>> +			i++;
>> +		}
>> +
>> +		/* more than 4 memmaps, fail kaslr */
>> +		if ((i >= MAX_MEMMAP_REGIONS) && str)
>> +			rc = -EINVAL;
>> +	}
>> +
>> +	return rc;
>> +}
>> +
>>  /*
>>   * 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
>> @@ -429,6 +552,7 @@ void choose_random_location(unsigned long input,
>>  			    unsigned long *virt_addr)
>>  {
>>  	unsigned long random_addr, min_addr;
>> +	int rc;
>>  
>>  	/* By default, keep output position unchanged. */
>>  	*virt_addr = *output;
>> @@ -438,6 +562,13 @@ void choose_random_location(unsigned long input,
>>  		return;
>>  	}
>>  
>> +	/* Mark the memmap regions we need to avoid */
>> +	rc = mem_avoid_memmap();
>> +	if (rc < 0) {
> Or write it like below, the rc is not needed. Personal coding style,
> just talk about it.
> 	if (mem_avoid_memmap()) {
> 		warn("KASLR disabled: memmap exceeds limit of 4, giving up.");
> 		return;
> 	}

Doesn't matter to me. I'll update as you suggest.

>> +
>>  	boot_params->hdr.loadflags |= KASLR_FLAG;
>>  
>>  	/* Prepare to add new identity pagetables on demand. */
>> diff --git a/arch/x86/boot/string.c b/arch/x86/boot/string.c
>> index cc3bd58..0464aaa 100644
>> --- a/arch/x86/boot/string.c
>> +++ b/arch/x86/boot/string.c
>> @@ -122,6 +122,31 @@ unsigned long long simple_strtoull(const char *cp, char **endp, unsigned int bas
>>  }
>>  
>>  /**
>> + * simple_strtoul - convert a string to an unsigned long
>> + * @cp: The start of the string
>> + * @endp: A pointer to the end of the parsed string will be placed here
>> + * @base: The number base to use
>> + */
>> +unsigned long simple_strtoul(const char *cp, char **endp, unsigned int base)
>> +{
>> +	return simple_strtoull(cp, endp, base);
>> +}
>> +
>> +/**
>> + * simple_strtol - convert a string to a signed long
>> + * @cp: The start of the string
>> + * @endp: A pointer to the end of the parsed string will be placed here
>> + * @base: The number base to use
>> + */
>> +long simple_strtol(const char *cp, char **endp, unsigned int base)
>> +{
>> +	if (*cp == '-')
>> +		return -simple_strtoul(cp + 1, endp, base);
>> +
>> +	return simple_strtoul(cp, endp, base);
>> +}
>> +
>> +/**
>>   * strlen - Find the length of a string
>>   * @s: The string to be sized
>>   */
>> @@ -155,3 +180,16 @@ char *strstr(const char *s1, const char *s2)
>>  	}
>>  	return NULL;
>>  }
>> +
>> +/**
>> + * strchr - Find the first occurrence of the character c in the string s.
>> + * @s: the string to be searched
>> + * @c: the character to search for
>> + */
>> +char *strchr(const char *s, int c)
>> +{
>> +	while (*s != (char)c)
>> +		if (*s++ == '\0')
>> +			return NULL;
>> +	return (char *)s;
>> +}
>>