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

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

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.

> 
> 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);
>