Re: [RFC PATCH 03/13] powerpc/dexcr: Handle hashchk exception

[Benjamin Gray]

On Tue, 2022-11-29 at 20:39 +1000, Nicholas Piggin wrote:
> On Mon Nov 28, 2022 at 12:44 PM AEST, Benjamin Gray wrote:
> > Recognise and pass the appropriate signal to the user program when
> > a
> > hashchk instruction triggers. This is independent of allowing
> > configuration of DEXCR[NPHIE], as a hypervisor can enforce this
> > aspect
> > regardless of the kernel.
> > 
> > Signed-off-by: Benjamin Gray <bgray@xxxxxxxxxxxxx>
> > ---
> >  arch/powerpc/include/asm/ppc-opcode.h |  1 +
> >  arch/powerpc/include/asm/processor.h  |  6 ++++++
> >  arch/powerpc/kernel/dexcr.c           | 22 ++++++++++++++++++++++
> >  arch/powerpc/kernel/traps.c           |  6 ++++++
> >  4 files changed, 35 insertions(+)
> > 
> > diff --git a/arch/powerpc/include/asm/ppc-opcode.h
> > b/arch/powerpc/include/asm/ppc-opcode.h
> > index 21e33e46f4b8..89b316466ed1 100644
> > --- a/arch/powerpc/include/asm/ppc-opcode.h
> > +++ b/arch/powerpc/include/asm/ppc-opcode.h
> > @@ -215,6 +215,7 @@
> >  #define OP_31_XOP_STFSX            663
> >  #define OP_31_XOP_STFSUX    695
> >  #define OP_31_XOP_STFDX     727
> > +#define OP_31_XOP_HASHCHK   754
> >  #define OP_31_XOP_STFDUX    759
> >  #define OP_31_XOP_LHBRX     790
> >  #define OP_31_XOP_LFIWAX    855
> > diff --git a/arch/powerpc/include/asm/processor.h
> > b/arch/powerpc/include/asm/processor.h
> > index 0a8a793b8b8b..c17ec1e44c86 100644
> > --- a/arch/powerpc/include/asm/processor.h
> > +++ b/arch/powerpc/include/asm/processor.h
> > @@ -448,10 +448,16 @@ void *exit_vmx_ops(void *dest);
> >  
> >  #ifdef CONFIG_PPC_BOOK3S_64
> >  
> > +bool is_hashchk_trap(struct pt_regs const *regs);
> >  unsigned long get_thread_dexcr(struct thread_struct const *t);
> >  
> >  #else
> >  
> > +static inline bool is_hashchk_trap(struct pt_regs const *regs)
> > +{
> > +       return false;
> > +}
> > +
> >  static inline unsigned long get_thread_dexcr(struct thread_struct
> > const *t)
> >  {
> >         return 0;
> > diff --git a/arch/powerpc/kernel/dexcr.c
> > b/arch/powerpc/kernel/dexcr.c
> > index 32a0a69ff638..11515e67afac 100644
> > --- a/arch/powerpc/kernel/dexcr.c
> > +++ b/arch/powerpc/kernel/dexcr.c
> > @@ -3,6 +3,9 @@
> >  
> >  #include <asm/cpu_has_feature.h>
> >  #include <asm/cputable.h>
> > +#include <asm/disassemble.h>
> > +#include <asm/inst.h>
> > +#include <asm/ppc-opcode.h>
> >  #include <asm/processor.h>
> >  #include <asm/reg.h>
> >  
> > @@ -19,6 +22,25 @@ static int __init dexcr_init(void)
> >  }
> >  early_initcall(dexcr_init);
> >  
> > +bool is_hashchk_trap(struct pt_regs const *regs)
> > +{
> > +       ppc_inst_t insn;
> > +
> > +       if (!cpu_has_feature(CPU_FTR_DEXCR_NPHIE))
> > +               return false;
> > +
> > +       if (get_user_instr(insn, (void __user *)regs->nip)) {
> > +               WARN_ON(1);
> > +               return false;
> > +       }
> 
> Nice series, just starting to have a look at it.
> 
> You probably don't want a WARN_ON() here because it's user
> triggerable
> and isn't necessarily even indiciating a problem or attack if the app
> is doing code unmapping in order to get faults.
> 
> Check some of the other instruction emulation for what to do in case
> of
> an EFAULT.

Alright, I'll take a look

> > +
> > +       if (ppc_inst_primary_opcode(insn) == 31 &&
> > +           get_xop(ppc_inst_val(insn)) == OP_31_XOP_HASHCHK)
> > +               return true;
> > +
> > +       return false;
> > +}
> > +
> >  unsigned long get_thread_dexcr(struct thread_struct const *t)
> >  {
> >         return DEFAULT_DEXCR;
> > diff --git a/arch/powerpc/kernel/traps.c
> > b/arch/powerpc/kernel/traps.c
> > index 9bdd79aa51cf..b83f5b382f24 100644
> > --- a/arch/powerpc/kernel/traps.c
> > +++ b/arch/powerpc/kernel/traps.c
> > @@ -1516,6 +1516,12 @@ static void do_program_check(struct pt_regs
> > *regs)
> >                                 return;
> >                         }
> >                 }
> > +
> > +               if (user_mode(regs) && is_hashchk_trap(regs)) {
> > +                       _exception(SIGILL, regs, ILL_ILLOPN, regs-
> > >nip);
> > +                       return;
> > +               }
> 
> I guess ILLOPN makes sense. Do you know if any other archs do
> similar?

Ah sorry, when refactoring Chris' patches I forgot to put back in the
commit message that this is how ARM reports their similar check
failure. For example, their FPAC handler in
arch/arm64/kernel/traps.c:518 does this.