| 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
| ||
|
Message-ID: <0fe05bd2-1155-0016-9de7-688812f9dc00@cn.fujitsu.com>
Date: Fri, 20 Oct 2017 11:41:19 +0800
From: Dou Liyang <douly.fnst@...fujitsu.com>
To: Chao Fan <fanc.fnst@...fujitsu.com>
CC: <linux-kernel@...r.kernel.org>, <x86@...nel.org>, <hpa@...or.com>,
<tglx@...utronix.de>, <mingo@...hat.com>, <bhe@...hat.com>,
<keescook@...omium.org>, <indou.takao@...fujitsu.com>,
<caoj.fnst@...fujitsu.com>
Subject: Re: [PATCH 1/4] kaslr: parse the extended
movable_node=nn[KMG]@ss[KMG]
At 10/20/2017 11:22 AM, Chao Fan wrote:
> On Fri, Oct 20, 2017 at 11:04:42AM +0800, Dou Liyang wrote:
>> Hi Chao,
>>
> Hi Dou-san,
>
>> 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.
>>
>
> Thanks for the explaination.
>
>>> 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.
>
> Yes, will add. Many thanks.
>
>>
>>> + 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.
>
> Using num_immovable_region makes code too long. Using i will be
> clear and make sure shoter than 80 characters.
Oh, God, that's horrific. Did you find that your code is wrong?
num_immovable_region will be reset each time.
Thanks,
dou.
>
>>
>>> + 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
>
> ditto...
> Why do you care this little variable so much.
>
>>
>>> + }
>>> + num_immovable_region = i;
>>
>> Just remove it.
>
> ditto.
>
>>
>>> +}
>>> +#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.
>
> There is existing and useful code, so it's better to reuse but not
> re-write.
>
>>
>> BTW, Using movable_node to store the memory of immovable node is
>> strange and make me confuse. How about adding a new command option.
>>
>
> I have added the document, if you think there are problems, please let
> me know.
>
> Thanks,
> Chao Fan
>
>> Thanks,
>> dou.
>>> } else if (!strcmp(param, "mem")) {
>>> char *p = val;
>>>
>>>
>
Powered by blists - more mailing lists