linux-kernel - Re: [PATCH 1/4] kaslr: parse the extended movable

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <5e20f27e-5658-5d0c-4cb2-41b39124a0d6@cn.fujitsu.com>
Date:   Fri, 20 Oct 2017 11:04:42 +0800
From:   Dou Liyang <douly.fnst@...fujitsu.com>
To:     Chao Fan <fanc.fnst@...fujitsu.com>,
        <linux-kernel@...r.kernel.org>, <x86@...nel.org>, <hpa@...or.com>,
        <tglx@...utronix.de>, <mingo@...hat.com>, <bhe@...hat.com>,
        <keescook@...omium.org>
CC:     <indou.takao@...fujitsu.com>, <caoj.fnst@...fujitsu.com>
Subject: Re: [PATCH 1/4] kaslr: parse the extended
 movable_node=nn[KMG]@ss[KMG]

Hi Chao,

At 10/19/2017 06:02 PM, Chao Fan wrote:
> Extend the movable_node to movable_node=nn[KMG]@ss[KMG].
> Since in current code, kaslr may choose the memory region in hot-pluggable
> nodes. So we can specific the region in immovable node. And store the
> regions in immovable_mem.
>

I guess you may mean that:

In current Linux with KASLR. Kernel may choose a memory region in
movable node for extracting kernel code, which will make the node
can't be hot-removed.

Solve it by only specific the region in immovable node. So create
immovable_mem to store the region of movable node, and only choose
the memory in immovable_mem array.

> Multiple regions can be specified, comma delimited.
> Considering the usage of memory, only support for 4 regions.
> 4 regions contains 2 nodes at least, and is enough for kernel to
> extract.
>
> Signed-off-by: Chao Fan <fanc.fnst@...fujitsu.com>
> ---
>  arch/x86/boot/compressed/kaslr.c | 63 +++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 62 insertions(+), 1 deletion(-)
>
> diff --git a/arch/x86/boot/compressed/kaslr.c b/arch/x86/boot/compressed/kaslr.c
> index 17818ba6906f..3c1f5204693b 100644
> --- a/arch/x86/boot/compressed/kaslr.c
> +++ b/arch/x86/boot/compressed/kaslr.c
> @@ -107,6 +107,12 @@ enum mem_avoid_index {
>
>  static struct mem_vector mem_avoid[MEM_AVOID_MAX];
>
> +/* Only supporting at most 4 immovable memory regions with kaslr */
> +#define MAX_IMMOVABLE_MEM	4
> +
> +static struct mem_vector immovable_mem[MAX_IMMOVABLE_MEM];
> +static int num_immovable_region;
> +
>  static bool mem_overlaps(struct mem_vector *one, struct mem_vector *two)
>  {
>  	/* Item one is entirely before item two. */
> @@ -167,6 +173,28 @@ parse_memmap(char *p, unsigned long long *start, unsigned long long *size)
>  	return -EINVAL;
>  }
>
> +static int parse_immovable_mem(char *p,
> +			       unsigned long long *start,
> +			       unsigned long long *size)
> +{
> +	char *oldp;
> +
> +	if (!p)
> +		return -EINVAL;
> +
> +	oldp = p;
> +	*size = memparse(p, &p);
> +	if (p == oldp)
> +		return -EINVAL;
> +
> +	if (*p == '@') {
> +		*start = memparse(p + 1, &p);
> +		return 0;
> +	}
> +

Here you don't consider that if @ss[KMG] is omitted.

> +	return -EINVAL;
> +}
> +
>  static void mem_avoid_memmap(char *str)
>  {
>  	static int i;
> @@ -206,6 +234,36 @@ static void mem_avoid_memmap(char *str)
>  		memmap_too_large = true;
>  }
>
> +#ifdef CONFIG_MEMORY_HOTPLUG
> +static void mem_mark_immovable(char *str)
> +{
> +	int i = 0;
> +

you have use num_immovable_region, 'i' is useless. just remove it.

> +	while (str && (i < MAX_IMMOVABLE_MEM)) {
> +		int rc;
> +		unsigned long long start, size;
> +		char *k = strchr(str, ',');
> +

Why do you put this definition here? IMO, moving it out is better.

> +		if (k)
> +			*k++ = 0;
> +
> +		rc = parse_immovable_mem(str, &start, &size);
> +		if (rc < 0)
> +			break;
> +		str = k;
> +
> +		immovable_mem[i].start = start;
> +		immovable_mem[i].size = size;
> +		i++;

Replace it with num_immovable_region

> +	}
> +	num_immovable_region = i;

Just remove it.

> +}
> +#else
> +static inline void mem_mark_immovable(char *str)
> +{
> +}
> +#endif
> +
>  static int handle_mem_memmap(void)
>  {
>  	char *args = (char *)get_cmd_line_ptr();
> @@ -214,7 +272,8 @@ static int handle_mem_memmap(void)
>  	char *param, *val;
>  	u64 mem_size;
>
> -	if (!strstr(args, "memmap=") && !strstr(args, "mem="))
> +	if (!strstr(args, "memmap=") && !strstr(args, "mem=") &&
> +	    !strstr(args, "movable_node="))
>  		return 0;
>
>  	tmp_cmdline = malloc(len + 1);
> @@ -239,6 +298,8 @@ static int handle_mem_memmap(void)
>
>  		if (!strcmp(param, "memmap")) {
>  			mem_avoid_memmap(val);
> +		} else if (!strcmp(param, "movable_node")) {
> +			mem_mark_immovable(val);

AFAIK, handle_mem_memmap() is invoked in mem_avoid_init(), which is used 
to avoid mem. But, here the value of immovable node is the memory
you want to mark and use, it is better that we split it out.

BTW, Using movable_node to store the memory of immovable node is
strange and make me confuse. How about adding a new command option.

Thanks,
	dou.
>  		} else if (!strcmp(param, "mem")) {
>  			char *p = val;
>
>