[PATCH] powerpc/dexcr: Move HASHCHK trap handler

[PATCH] powerpc/dexcr: Move HASHCHK trap handler

[Benjamin Gray]

To determine if a trap was caused by a HASHCHK instruction, we inspect
the user instruction that triggered the trap. However this may sleep
if the page needs to be faulted in.

Move the HASHCHK handler logic to after we allow IRQs, which is fine
because we are only interested in HASHCHK if it's a user space trap.

Fixes: 5bcba4e6c13f ("powerpc/dexcr: Handle hashchk exception")
Signed-off-by: Benjamin Gray <bgray@linux.ibm.com>
---
 arch/powerpc/kernel/traps.c | 56 ++++++++++++++++++++++++-------------
 1 file changed, 36 insertions(+), 20 deletions(-)

diff --git a/arch/powerpc/kernel/traps.c b/arch/powerpc/kernel/traps.c
index f5ce282dc4b8..32b5e841ea97 100644
--- a/arch/powerpc/kernel/traps.c
+++ b/arch/powerpc/kernel/traps.c
@@ -1512,23 +1512,11 @@ static void do_program_check(struct pt_regs *regs)
 			return;
 		}
 
-		if (cpu_has_feature(CPU_FTR_DEXCR_NPHIE) && user_mode(regs)) {
-			ppc_inst_t insn;
-
-			if (get_user_instr(insn, (void __user *)regs->nip)) {
-				_exception(SIGSEGV, regs, SEGV_MAPERR, regs->nip);
-				return;
-			}
-
-			if (ppc_inst_primary_opcode(insn) == 31 &&
-			    get_xop(ppc_inst_val(insn)) == OP_31_XOP_HASHCHK) {
-				_exception(SIGILL, regs, ILL_ILLOPN, regs->nip);
-				return;
-			}
+		/* User mode considers other cases after enabling IRQs */
+		if (!user_mode(regs)) {
+			_exception(SIGTRAP, regs, TRAP_BRKPT, regs->nip);
+			return;
 		}
-
-		_exception(SIGTRAP, regs, TRAP_BRKPT, regs->nip);
-		return;
 	}
 #ifdef CONFIG_PPC_TRANSACTIONAL_MEM
 	if (reason & REASON_TM) {
@@ -1561,16 +1549,44 @@ static void do_program_check(struct pt_regs *regs)
 
 	/*
 	 * If we took the program check in the kernel skip down to sending a
-	 * SIGILL. The subsequent cases all relate to emulating instructions
-	 * which we should only do for userspace. We also do not want to enable
-	 * interrupts for kernel faults because that might lead to further
-	 * faults, and loose the context of the original exception.
+	 * SIGILL. The subsequent cases all relate to user space, such as
+	 * emulating instructions which we should only do for user space. We
+	 * also do not want to enable interrupts for kernel faults because that
+	 * might lead to further faults, and loose the context of the original
+	 * exception.
 	 */
 	if (!user_mode(regs))
 		goto sigill;
 
 	interrupt_cond_local_irq_enable(regs);
 
+	/*
+	 * (reason & REASON_TRAP) is mostly handled before enabling IRQs,
+	 * except get_user_instr() can sleep so we cannot reliably inspect the
+	 * current instruction in that context. Now that we know we are
+	 * handling a user space trap and can sleep, we can check if the trap
+	 * was a hashchk failure.
+	 */
+	if (reason & REASON_TRAP) {
+		if (cpu_has_feature(CPU_FTR_DEXCR_NPHIE)) {
+			ppc_inst_t insn;
+
+			if (get_user_instr(insn, (void __user *)regs->nip)) {
+				_exception(SIGSEGV, regs, SEGV_MAPERR, regs->nip);
+				return;
+			}
+
+			if (ppc_inst_primary_opcode(insn) == 31 &&
+			    get_xop(ppc_inst_val(insn)) == OP_31_XOP_HASHCHK) {
+				_exception(SIGILL, regs, ILL_ILLOPN, regs->nip);
+				return;
+			}
+		}
+
+		_exception(SIGTRAP, regs, TRAP_BRKPT, regs->nip);
+		return;
+	}
+
 	/* (reason & REASON_ILLEGAL) would be the obvious thing here,
 	 * but there seems to be a hardware bug on the 405GP (RevD)
 	 * that means ESR is sometimes set incorrectly - either to
-- 
2.41.0

Re: [PATCH] powerpc/dexcr: Move HASHCHK trap handler

[Michael Ellerman]

Benjamin Gray <bgray@linux.ibm.com> writes:
> To determine if a trap was caused by a HASHCHK instruction, we inspect
> the user instruction that triggered the trap. However this may sleep
> if the page needs to be faulted in.

It would be good to include the output from the might_sleep() check that
failed.

And I think this was found by syzkaller? If so we should say so.

cheers

> Move the HASHCHK handler logic to after we allow IRQs, which is fine
> because we are only interested in HASHCHK if it's a user space trap.
>
> Fixes: 5bcba4e6c13f ("powerpc/dexcr: Handle hashchk exception")
> Signed-off-by: Benjamin Gray <bgray@linux.ibm.com>
> ---
>  arch/powerpc/kernel/traps.c | 56 ++++++++++++++++++++++++-------------
>  1 file changed, 36 insertions(+), 20 deletions(-)
>
> diff --git a/arch/powerpc/kernel/traps.c b/arch/powerpc/kernel/traps.c
> index f5ce282dc4b8..32b5e841ea97 100644
> --- a/arch/powerpc/kernel/traps.c
> +++ b/arch/powerpc/kernel/traps.c
> @@ -1512,23 +1512,11 @@ static void do_program_check(struct pt_regs *regs)
>  			return;
>  		}
>  
> -		if (cpu_has_feature(CPU_FTR_DEXCR_NPHIE) && user_mode(regs)) {
> -			ppc_inst_t insn;
> -
> -			if (get_user_instr(insn, (void __user *)regs->nip)) {
> -				_exception(SIGSEGV, regs, SEGV_MAPERR, regs->nip);
> -				return;
> -			}
> -
> -			if (ppc_inst_primary_opcode(insn) == 31 &&
> -			    get_xop(ppc_inst_val(insn)) == OP_31_XOP_HASHCHK) {
> -				_exception(SIGILL, regs, ILL_ILLOPN, regs->nip);
> -				return;
> -			}
> +		/* User mode considers other cases after enabling IRQs */
> +		if (!user_mode(regs)) {
> +			_exception(SIGTRAP, regs, TRAP_BRKPT, regs->nip);
> +			return;
>  		}
> -
> -		_exception(SIGTRAP, regs, TRAP_BRKPT, regs->nip);
> -		return;
>  	}
>  #ifdef CONFIG_PPC_TRANSACTIONAL_MEM
>  	if (reason & REASON_TM) {
> @@ -1561,16 +1549,44 @@ static void do_program_check(struct pt_regs *regs)
>  
>  	/*
>  	 * If we took the program check in the kernel skip down to sending a
> -	 * SIGILL. The subsequent cases all relate to emulating instructions
> -	 * which we should only do for userspace. We also do not want to enable
> -	 * interrupts for kernel faults because that might lead to further
> -	 * faults, and loose the context of the original exception.
> +	 * SIGILL. The subsequent cases all relate to user space, such as
> +	 * emulating instructions which we should only do for user space. We
> +	 * also do not want to enable interrupts for kernel faults because that
> +	 * might lead to further faults, and loose the context of the original
> +	 * exception.
>  	 */
>  	if (!user_mode(regs))
>  		goto sigill;
>  
>  	interrupt_cond_local_irq_enable(regs);
>  
> +	/*
> +	 * (reason & REASON_TRAP) is mostly handled before enabling IRQs,
> +	 * except get_user_instr() can sleep so we cannot reliably inspect the
> +	 * current instruction in that context. Now that we know we are
> +	 * handling a user space trap and can sleep, we can check if the trap
> +	 * was a hashchk failure.
> +	 */
> +	if (reason & REASON_TRAP) {
> +		if (cpu_has_feature(CPU_FTR_DEXCR_NPHIE)) {
> +			ppc_inst_t insn;
> +
> +			if (get_user_instr(insn, (void __user *)regs->nip)) {
> +				_exception(SIGSEGV, regs, SEGV_MAPERR, regs->nip);
> +				return;
> +			}
> +
> +			if (ppc_inst_primary_opcode(insn) == 31 &&
> +			    get_xop(ppc_inst_val(insn)) == OP_31_XOP_HASHCHK) {
> +				_exception(SIGILL, regs, ILL_ILLOPN, regs->nip);
> +				return;
> +			}
> +		}
> +
> +		_exception(SIGTRAP, regs, TRAP_BRKPT, regs->nip);
> +		return;
> +	}
> +
>  	/* (reason & REASON_ILLEGAL) would be the obvious thing here,
>  	 * but there seems to be a hardware bug on the 405GP (RevD)
>  	 * that means ESR is sometimes set incorrectly - either to
> -- 
> 2.41.0

Re: [PATCH] powerpc/dexcr: Move HASHCHK trap handler

[Michael Ellerman]

On Fri, 25 Aug 2023 11:49:10 +1000, Benjamin Gray wrote:
> To determine if a trap was caused by a HASHCHK instruction, we inspect
> the user instruction that triggered the trap. However this may sleep
> if the page needs to be faulted in.
> 
> Move the HASHCHK handler logic to after we allow IRQs, which is fine
> because we are only interested in HASHCHK if it's a user space trap.
> 
> [...]

Applied to powerpc/fixes.

[1/1] powerpc/dexcr: Move HASHCHK trap handler
      https://git.kernel.org/powerpc/c/c3f4309693758b13fbb34b3741c2e2801ad28769

cheers