linux-kernel - Re: [PATCH] arm64: Allow vmalloc regions to be set with set_memory

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: <56A9FFBD.8030303@huawei.com>
Date:	Thu, 28 Jan 2016 19:47:09 +0800
From:	Xishi Qiu <qiuxishi@...wei.com>
To:	Mark Rutland <mark.rutland@....com>
CC:	Ard Biesheuvel <ard.biesheuvel@...aro.org>,
	Laura Abbott <labbott@...oraproject.org>,
	Catalin Marinas <catalin.marinas@....com>,
	Will Deacon <will.deacon@....com>,
	Kees Cook <keescook@...omium.com>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"linux-arm-kernel@...ts.infradead.org" 
	<linux-arm-kernel@...ts.infradead.org>,
	zhong jiang <zhongjiang@...wei.com>, <steve.capper@....com>
Subject: Re: [PATCH] arm64: Allow vmalloc regions to be set with set_memory_*

On 2016/1/28 18:51, Mark Rutland wrote:

> On Thu, Jan 28, 2016 at 09:47:20AM +0800, Xishi Qiu wrote:
>> On 2016/1/18 19:56, Mark Rutland wrote:
>>> On Wed, Jan 13, 2016 at 05:10:31PM +0100, Ard Biesheuvel wrote:
>>>> Something along these lines, perhaps?
>>>>
>>>> diff --git a/arch/arm64/mm/pageattr.c b/arch/arm64/mm/pageattr.c
>>>> index 3571c7309c5e..bda0a776c58e 100644
>>>> --- a/arch/arm64/mm/pageattr.c
>>>> +++ b/arch/arm64/mm/pageattr.c
>>>> @@ -13,6 +13,7 @@
>>>>  #include <linux/kernel.h>
>>>>  #include <linux/mm.h>
>>>>  #include <linux/module.h>
>>>> +#include <linux/vmalloc.h>
>>>>  #include <linux/sched.h>
>>>>
>>>>  #include <asm/pgtable.h>
>>>> @@ -44,6 +45,7 @@ static int change_memory_common(unsigned long addr
>>>>         unsigned long end = start + size;
>>>>         int ret;
>>>>         struct page_change_data data;
>>>> +       struct vm_struct *area;
>>>>
>>>>         if (!PAGE_ALIGNED(addr)) {
>>>>                 start &= PAGE_MASK;
>>>> @@ -51,10 +53,14 @@ static int change_memory_common(unsigned long addr,
>>>>                 WARN_ON_ONCE(1);
>>>>         }
>>>>
>>>> -       if (start < MODULES_VADDR || start >= MODULES_END)
>>>> -               return -EINVAL;
>>>> -
>>>> -       if (end < MODULES_VADDR || end >= MODULES_END)
>>>> +       /*
>>>> +        * Check whether the [addr, addr + size) interval is entirely
>>>> +        * covered by precisely one VM area that has the VM_ALLOC flag set
>>>> +        */
>>>> +       area = find_vm_area((void *)addr);
>>>> +       if (!area ||
>>>> +           end > (unsigned long)area->addr + area->size ||
>>>> +           !(area->flags & VM_ALLOC))
>>>>                 return -EINVAL;
>>>>
>>>>         data.set_mask = set_mask;
>>>
>>> Neat. That fixes the fencepost bug too.
>>>
>>> Looks good to me, though as Laura suggested we should have a comment as
>>> to why we limit changes to such regions. Fancy taking her wording below
>>> and spinning this as a patch?
>>>
>>>>>> +       /*
>>>>>> +        * This check explicitly excludes most kernel memory. Most kernel
>>>>>> +        * memory is mapped with a larger page size and breaking down the
>>>>>> +        * larger page size without causing TLB conflicts is very difficult.
>>>>>> +        *
>>>>>> +        * If you need to call set_memory_* on a range, the recommendation is
>>>>>> +        * to use vmalloc since that range is mapped with pages.
>>>>>> +        */
>>>
>>> Thanks,
>>> Mark.
>>>
>>
>> Hi Mark,
>>
>> After change the flag, it calls only flush_tlb_kernel_range(), so why not use 
>> cpu_replace_ttbr1(swapper_pg_dir)? 
> 
> We cannot use cpu_replace_ttbr1 here. Other CPUs may be online, and we
> have no mechanism to place them in a safe set of page tables while
> swapping TTBR1, we'd have to perform a deep copy of tables, and this
> would be horrendously expensive.
> 
> Using flush_tlb_kernel_range() is sufficient. As modules don't share a
> page or section mapping with other users, we can follow a
> Break-Before-Make approach. Additionally, they're mapped at page
> granularity so we never split or fuse sections anyway. We only modify
> the permission bits.
> 

Hi Mark,

Is it safe in the following path?

alloc the whole linear map section
cpu A write something on it
cpu B write something on it
cpu C set read only flag and call flush_tlb_kernel_range()

Thanks,
Xishi Qiu

>> One more question, does TLB conflict only affect kernel page talbe?
> 
> It's harder to solve for the text/linear map as we can't do
> Break-Before-Make without potentially unmapping something in active use
> (e.g. the code used to implement Break-Before-Make).
> 
>> There is no problem when spliting the transparent hugepage, right?
> 
> There was a potential problem with huge pages causing TLB conflicts,
> which didn't always seem to follow a Break-Before-Make approach.
> 
> I believe that Kirill Shutemov's recent THP rework means that
> section->table and table->section conversions always go via an invalid
> entry, with appropriate TLB invalidation, making that safe. I have not
> yet had the chance to verify that yet, however.
> 
> Thanks,
> Mark.
> 
> .
>