Re: [PATCH 12/24] powerpc/mm: Fix reporting of kernel execute faults

[Christophe LEROY <christophe.leroy@c-s.fr>]

Le 30/11/2018 à 06:50, Aneesh Kumar K.V a écrit :
> Christophe LEROY <christophe.leroy@c-s.fr> writes:
> 
>> Hi Ben,
>>
>> I have an issue on the 8xx with this change
>>
>> Le 19/07/2017 à 06:49, Benjamin Herrenschmidt a écrit :
>>> We currently test for is_exec and DSISR_PROTFAULT but that doesn't
>>> make sense as this is the wrong error bit to test for an execute
>>> permission failure.
>>
>> On the 8xx, on an exec permission failure, this is the correct BIT, see
>> below extract from reference manual:
>>
>> Note that only one of bits 1, 3, and 4 will be set.
>> 1 1 if the translation of an attempted access is not in the translation
>> tables. Otherwise 0
>> 3 1 if the fetch access was to guarded memory when MSR[IR] = 1. Otherwise 0
>> 4 1 if the access is not permitted by the protection mechanism; otherwise 0.
>>
>> So on the 8xx, bit 3 is not DSISR_NOEXEC_OR_G but only DSISR_G.
>> When the PPP bits are set to No-Execute, we really get bit 4 that is
>> DSISR_PROTFAULT.
> 
> 
> Do you have an url for the document? I am wondering whether we can get
> Documentation/powerpc/cpu_families.txt updated with these urls?

https://www.nxp.com/docs/en/reference-manual/MPC885RM.pdf

Christophe

> 
>>
>>>
>>> In fact, we had code that would return early if we had an exec
>>> fault in kernel mode so I think that was just dead code anyway.
>>>
>>> Finally the location of that test is awkward and prevents further
>>> simplifications.
>>>
>>> So instead move that test into a helper along with the existing
>>> early test for kernel exec faults and out of range accesses,
>>> and put it all in a "bad_kernel_fault()" helper. While at it
>>> test the correct error bits.
>>>
>>> Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
>>> ---
>>>    arch/powerpc/mm/fault.c | 21 +++++++++++++++------
>>>    1 file changed, 15 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/arch/powerpc/mm/fault.c b/arch/powerpc/mm/fault.c
>>> index e8d6acc888c5..aead07cf8a5b 100644
>>> --- a/arch/powerpc/mm/fault.c
>>> +++ b/arch/powerpc/mm/fault.c
>>> @@ -180,6 +180,20 @@ static int mm_fault_error(struct pt_regs *regs, unsigned long addr, int fault)
>>>    	return MM_FAULT_CONTINUE;
>>>    }
>>>    
>>> +/* Is this a bad kernel fault ? */
>>> +static bool bad_kernel_fault(bool is_exec, unsigned long error_code,
>>> +			     unsigned long address)
>>> +{
>>> +	if (is_exec && (error_code & (DSISR_NOEXEC_OR_G | DSISR_KEYFAULT))) {
>>
>> Do you mind if we had DSISR_PROTFAULT here as well ?
>>
>> Christophe
>>
>>> +		printk_ratelimited(KERN_CRIT "kernel tried to execute"
>>> +				   " exec-protected page (%lx) -"
>>> +				   "exploit attempt? (uid: %d)\n",
>>> +				   address, from_kuid(&init_user_ns,
>>> +						      current_uid()));
>>> +	}
>>> +	return is_exec || (address >= TASK_SIZE);
>>> +}
>>> +
>>>    /*
>>>     * Define the correct "is_write" bit in error_code based
>>>     * on the processor family
>>> @@ -252,7 +266,7 @@ static int __do_page_fault(struct pt_regs *regs, unsigned long address,
>>>    	 * The kernel should never take an execute fault nor should it
>>>    	 * take a page fault to a kernel address.
>>>    	 */
>>> -	if (!is_user && (is_exec || (address >= TASK_SIZE)))
>>> +	if (unlikely(!is_user && bad_kernel_fault(is_exec, error_code, address)))
>>>    		return SIGSEGV;
>>>    
>>>    	/* We restore the interrupt state now */
>>> @@ -491,11 +505,6 @@ static int __do_page_fault(struct pt_regs *regs, unsigned long address,
>>>    		return 0;
>>>    	}
>>>    
>>> -	if (is_exec && (error_code & DSISR_PROTFAULT))
>>> -		printk_ratelimited(KERN_CRIT "kernel tried to execute NX-protected"
>>> -				   " page (%lx) - exploit attempt? (uid: %d)\n",
>>> -				   address, from_kuid(&init_user_ns, current_uid()));
>>> -
>>>    	return SIGSEGV;
>>>    }
>>>    NOKPROBE_SYMBOL(__do_page_fault);
>>>