Re: [PATCH] powerpc/mm: Fix lockup on kernel exec fault

[Nicholas Piggin]

Excerpts from Christophe Leroy's message of July 1, 2021 9:17 pm:
> The powerpc kernel is not prepared to handle exec faults from kernel.
> Especially, the function is_exec_fault() will return 'false' when an
> exec fault is taken by kernel, because the check is based on reading
> current->thread.regs->trap which contains the trap from user.
> 
> For instance, when provoking a LKDTM EXEC_USERSPACE test,
> current->thread.regs->trap is set to SYSCALL trap (0xc00), and
> the fault taken by the kernel is not seen as an exec fault by
> set_access_flags_filter().
> 
> Commit d7df2443cd5f ("powerpc/mm: Fix spurrious segfaults on radix
> with autonuma") made it clear and handled it properly. But later on
> commit d3ca587404b3 ("powerpc/mm: Fix reporting of kernel execute
> faults") removed that handling, introducing test based on error_code.
> And here is the problem, because on the 603 all upper bits of SRR1
> get cleared when the TLB instruction miss handler bails out to ISI.

So the problem is 603 doesn't see the DSISR_NOEXEC_OR_G bit?

I don't see the problem with this for 64s, I don't think anything sane
can be done for any 0x400 interrupt in the kernel so it's probably
good to catch all here just in case. For 64s,

Acked-by: Nicholas Piggin <npiggin@xxxxxxxxx>

Why is 32s clearing those top bits? And it seems to be setting DSISR
that AFAIKS it does not use. Seems like it would be good to add a
NOEXEC_OR_G bit into SRR1.

Thanks,
Nick


> Until commit cbd7e6ca0210 ("powerpc/fault: Avoid heavy
> search_exception_tables() verification"), an exec fault from kernel
> at a userspace address was indirectly caught by the lack of entry for
> that address in the exception tables. But after that commit the
> kernel mainly rely on KUAP or on core mm handling to catch wrong
> user accesses. Here the access is not wrong, so mm handles it.
> It is a minor fault because PAGE_EXEC is not set,
> set_access_flags_filter() should set PAGE_EXEC and voila.
> But as is_exec_fault() returns false as explained in the begining,
> set_access_flags_filter() bails out without setting PAGE_EXEC flag,
> which leads to a forever minor exec fault.
> 
> As the kernel is not prepared to handle such exec faults, the thing
> to do is to fire in bad_kernel_fault() for any exec fault taken by
> the kernel, as it was prior to commit d3ca587404b3.
> 
> Fixes: d3ca587404b3 ("powerpc/mm: Fix reporting of kernel execute faults")
> Cc: stable@xxxxxxxxxxxxxxx
> Signed-off-by: Christophe Leroy <christophe.leroy@xxxxxxxxxx>
> ---
>  arch/powerpc/mm/fault.c | 4 +---
>  1 file changed, 1 insertion(+), 3 deletions(-)
> 
> diff --git a/arch/powerpc/mm/fault.c b/arch/powerpc/mm/fault.c
> index 34f641d4a2fe..a8d0ce85d39a 100644
> --- a/arch/powerpc/mm/fault.c
> +++ b/arch/powerpc/mm/fault.c
> @@ -199,9 +199,7 @@ static bool bad_kernel_fault(struct pt_regs *regs, unsigned long error_code,
>  {
>  	int is_exec = TRAP(regs) == INTERRUPT_INST_STORAGE;
>  
> -	/* NX faults set DSISR_PROTFAULT on the 8xx, DSISR_NOEXEC_OR_G on others */
> -	if (is_exec && (error_code & (DSISR_NOEXEC_OR_G | DSISR_KEYFAULT |
> -				      DSISR_PROTFAULT))) {
> +	if (is_exec) {
>  		pr_crit_ratelimited("kernel tried to execute %s page (%lx) - exploit attempt? (uid: %d)\n",
>  				    address >= TASK_SIZE ? "exec-protected" : "user",
>  				    address,
> -- 
> 2.25.0
> 
>