[PATCH] arm64: mm: Fix false positives in W+X checking

[Jeffrey Hugo]

On 4/25/2018 9:03 AM, Will Deacon wrote:
> Hi Jeffrey,
> 
> Thanks for the patch. Comment below.
> 
> On Wed, Apr 25, 2018 at 08:13:31AM -0600, Jeffrey Hugo wrote:
>> load_module() creates W+X mappings via __vmalloc_node_range() (from
>> layout_and_allocate()->move_module()->module_alloc()) by using
>> PAGE_KERNEL_EXEC.  These mappings are later cleaned up via
>> "call_rcu_sched(&freeinit->rcu, do_free_init)" from do_init_module().
>>
>> This is a problem because call_rcu_sched() queues work, which can be run
>> after debug_checkwx() is run, resulting in a race condition.  If hit, the
>> race results in a nasty splat about insecure W+X mappings, which results
>> in a poor user experience as these are not the mappings that
>> debug_checkwx() is intended to catch.
>>
>> Address the race by flushing the queued work before running
>> debug_checkwx().
>>
>> Reported-by: Timur Tabi <timur at codeaurora.org>
>> Reported-by: Jan Glauber <jan.glauber at caviumnetworks.com>
>> Fixes: 1404d6f13e47 ("arm64: dump: Add checking for writable and exectuable pages")
>> Signed-off-by: Jeffrey Hugo <jhugo at codeaurora.org>
>> ---
>>   arch/arm64/mm/mmu.c | 6 ++++++
>>   1 file changed, 6 insertions(+)
>>
>> diff --git a/arch/arm64/mm/mmu.c b/arch/arm64/mm/mmu.c
>> index 2dbb2c9..40d45fd 100644
>> --- a/arch/arm64/mm/mmu.c
>> +++ b/arch/arm64/mm/mmu.c
>> @@ -503,6 +503,12 @@ void mark_rodata_ro(void)
>>   	update_mapping_prot(__pa_symbol(__start_rodata), (unsigned long)__start_rodata,
>>   			    section_size, PAGE_KERNEL_RO);
>>   
>> +	/*
>> +	 * load_module() results in W+X mappings, which are cleaned up with
>> +	 * call_rcu_sched().  Let's make sure that queued work is flushed so
>> +	 * that we don't hit false positives.
>> +	 */
>> +	rcu_barrier_sched();
>>   	debug_checkwx();
>>   }
> 
> Whilst this looks correct to me, it looks to me like other architectures
> (e.g. x86) would be affected by this problem as well. Perhaps it would be
> better to solve it in the core code before invoking mark_rodata_ro, or is
> there some reason that we only run into this on arm64?
> 

Thanks for the review.

I was actually pondering pushing this into ptdump_check_wx() so that the 
"barrier" hit is only observed with CONFIG_DEBUG_WX.

I agree, in principal this is not an arm64 specific issue.  I do not 
have sufficient equipment to validate other architectures.  On QDF2400, 
the reproduction rate is very low, roughly 1-3 instances in 2000-3000 
reboots.  I do have a system simulator which happens to repro the issue 
100% of the time, which is what I used to debug and test this fix, 
before applying it to QDF2400.

I'm waffling.  I see the benefit of fixing this in common code, but the 
"core" functionality of mark_rodata_ro doesn't need this barrier...

I suppose I can push it up to core code, and see what the rest of the 
community says.  Is that what you recommend?

-- 
Jeffrey Hugo
Qualcomm Datacenter Technologies as an affiliate of Qualcomm 
Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.