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.ji...@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;
>> +}
>>
