linux-kernel - Re: [PATCH 3/4] arm64/kprobes: set VM_FLUSH_RESET

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 for Android: free password hash cracker in your pocket

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

Message-ID: <7c5e1fea-c93e-fc5b-8c77-c9bd9ec41fb3@arm.com>
Date:   Tue, 28 May 2019 10:23:14 +0200
From:   Ard Biesheuvel <ard.biesheuvel@....com>
To:     Anshuman Khandual <anshuman.khandual@....com>,
        linux-arm-kernel@...ts.infradead.org
Cc:     mark.rutland@....com, marc.zyngier@....com,
        Will Deacon <will.deacon@....com>,
        linux-kernel@...r.kernel.org,
        Peter Zijlstra <peterz@...radead.org>,
        Nadav Amit <namit@...are.com>,
        Masami Hiramatsu <mhiramat@...nel.org>,
        James Morse <james.morse@....com>,
        Andrew Morton <akpm@...ux-foundation.org>,
        Rick Edgecombe <rick.p.edgecombe@...el.com>
Subject: Re: [PATCH 3/4] arm64/kprobes: set VM_FLUSH_RESET_PERMS on kprobe
 instruction pages

On 5/28/19 10:20 AM, Anshuman Khandual wrote:
> 
> 
> On 05/23/2019 03:52 PM, Ard Biesheuvel wrote:
>> In order to avoid transient inconsistencies where freed code pages
>> are remapped writable while stale TLB entries still exist on other
>> cores, mark the kprobes text pages with the VM_FLUSH_RESET_PERMS
>> attribute. This instructs the core vmalloc code not to defer the
>> TLB flush when this region is unmapped and returned to the page
>> allocator.
> 
> Makes sense.
> 
>>
>> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@....com>
>> ---
>>   arch/arm64/kernel/probes/kprobes.c | 4 +++-
>>   1 file changed, 3 insertions(+), 1 deletion(-)
>>
>> diff --git a/arch/arm64/kernel/probes/kprobes.c b/arch/arm64/kernel/probes/kprobes.c
>> index 2509fcb6d404..036cfbf9682a 100644
>> --- a/arch/arm64/kernel/probes/kprobes.c
>> +++ b/arch/arm64/kernel/probes/kprobes.c
>> @@ -131,8 +131,10 @@ void *alloc_insn_page(void)
>>   	void *page;
>>   
>>   	page = vmalloc_exec(PAGE_SIZE);
>> -	if (page)
>> +	if (page) {
>>   		set_memory_ro((unsigned long)page, 1);
>> +		set_vm_flush_reset_perms(page);
>> +	}
> 
> Looks good. It seems there might be more users who would like to set
> VM_FLUSH_RESET_PERMS right after their allocation for the same reason.
> Hence would not it help to have a variant like vmalloc_exec_reset() or
> such which will tag vm_struct->flags with VM_FLUSH_RESET_PERMS right
> after it's allocation without requiring the caller to do the same.
> 

If there is a sufficient number of similar users, then of course, it 
makes sense to factor this out. However, the kprobes code is slightly 
unusual in the sense that it allocates memory and immediately remaps it 
read-only, and relies on code patching to populate this allocation. I am 
not expecting to see this pattern in other places.