Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <53d5c0793e3eb2176a19b4fe75d23cc3bc675329.camel@linux.ibm.com>
Subject: Re: [RFC PATCH 03/13] powerpc/dexcr: Handle hashchk exception
From:   Benjamin Gray <bgray@linux.ibm.com>
To:     Nicholas Piggin <npiggin@gmail.com>, linuxppc-dev@lists.ozlabs.org
Cc:     ajd@linux.ibm.com, linux-kernel@vger.kernel.org,
        linux-hardening@vger.kernel.org, cmr@bluescreens.de
Date:   Wed, 30 Nov 2022 09:04:02 +1100
In-Reply-To: <COOPRF72WR3V.1WPE5NI7M8V7J@bobo>
References: <20221128024458.46121-1-bgray@linux.ibm.com>
         <20221128024458.46121-4-bgray@linux.ibm.com>
         <COOPRF72WR3V.1WPE5NI7M8V7J@bobo>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
User-Agent: Evolution 3.46.1 (3.46.1-1.fc37) 
MIME-Version: 1.0
Precedence: bulk

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.
> >=20
> > Signed-off-by: Benjamin Gray <bgray@linux.ibm.com>
> > ---
> > =C2=A0arch/powerpc/include/asm/ppc-opcode.h |=C2=A0 1 +
> > =C2=A0arch/powerpc/include/asm/processor.h=C2=A0 |=C2=A0 6 ++++++
> > =C2=A0arch/powerpc/kernel/dexcr.c=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=
=C2=A0=C2=A0=C2=A0=C2=A0 | 22 ++++++++++++++++++++++
> > =C2=A0arch/powerpc/kernel/traps.c=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=
=C2=A0=C2=A0=C2=A0=C2=A0 |=C2=A0 6 ++++++
> > =C2=A04 files changed, 35 insertions(+)
> >=20
> > 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 @@
> > =C2=A0#define OP_31_XOP_STFSX=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=
=C2=A0=C2=A0=C2=A0=C2=A0 663
> > =C2=A0#define OP_31_XOP_STFSUX=C2=A0=C2=A0=C2=A0 695
> > =C2=A0#define OP_31_XOP_STFDX=C2=A0=C2=A0=C2=A0=C2=A0 727
> > +#define OP_31_XOP_HASHCHK=C2=A0=C2=A0 754
> > =C2=A0#define OP_31_XOP_STFDUX=C2=A0=C2=A0=C2=A0 759
> > =C2=A0#define OP_31_XOP_LHBRX=C2=A0=C2=A0=C2=A0=C2=A0 790
> > =C2=A0#define OP_31_XOP_LFIWAX=C2=A0=C2=A0=C2=A0 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);
> > =C2=A0
> > =C2=A0#ifdef CONFIG_PPC_BOOK3S_64
> > =C2=A0
> > +bool is_hashchk_trap(struct pt_regs const *regs);
> > =C2=A0unsigned long get_thread_dexcr(struct thread_struct const *t);
> > =C2=A0
> > =C2=A0#else
> > =C2=A0
> > +static inline bool is_hashchk_trap(struct pt_regs const *regs)
> > +{
> > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0return false;
> > +}
> > +
> > =C2=A0static inline unsigned long get_thread_dexcr(struct thread_struct
> > const *t)
> > =C2=A0{
> > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0return 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 @@
> > =C2=A0
> > =C2=A0#include <asm/cpu_has_feature.h>
> > =C2=A0#include <asm/cputable.h>
> > +#include <asm/disassemble.h>
> > +#include <asm/inst.h>
> > +#include <asm/ppc-opcode.h>
> > =C2=A0#include <asm/processor.h>
> > =C2=A0#include <asm/reg.h>
> > =C2=A0
> > @@ -19,6 +22,25 @@ static int __init dexcr_init(void)
> > =C2=A0}
> > =C2=A0early_initcall(dexcr_init);
> > =C2=A0
> > +bool is_hashchk_trap(struct pt_regs const *regs)
> > +{
> > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0ppc_inst_t insn;
> > +
> > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0if (!cpu_has_feature(CPU_FTR=
_DEXCR_NPHIE))
> > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=
=A0=C2=A0=C2=A0=C2=A0return false;
> > +
> > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0if (get_user_instr(insn, (vo=
id __user *)regs->nip)) {
> > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=
=A0=C2=A0=C2=A0=C2=A0WARN_ON(1);
> > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=
=A0=C2=A0=C2=A0=C2=A0return false;
> > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0}
>=20
> Nice series, just starting to have a look at it.
>=20
> 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.
>=20
> Check some of the other instruction emulation for what to do in case
> of
> an EFAULT.

Alright, I'll take a look

> > +
> > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0if (ppc_inst_primary_opcode(=
insn) =3D=3D 31 &&
> > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 get_xop(p=
pc_inst_val(insn)) =3D=3D OP_31_XOP_HASHCHK)
> > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=
=A0=C2=A0=C2=A0=C2=A0return true;
> > +
> > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0return false;
> > +}
> > +
> > =C2=A0unsigned long get_thread_dexcr(struct thread_struct const *t)
> > =C2=A0{
> > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0return 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)
> > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=
=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=
=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0return;
> > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=
=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=
}
> > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=
=A0=C2=A0=C2=A0=C2=A0=C2=A0}
> > +
> > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=
=A0=C2=A0=C2=A0=C2=A0if (user_mode(regs) && is_hashchk_trap(regs)) {
> > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=
=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0_excep=
tion(SIGILL, regs, ILL_ILLOPN, regs-
> > >nip);
> > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=
=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0return=
;
> > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=
=A0=C2=A0=C2=A0=C2=A0}
>=20
> 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.