On Thu, 19 Dec 2019 01:11:33 +1100 Daniel Axtens <d...@axtens.net> wrote:
> Jordan Niethe <jniet...@gmail.com> writes: > > > A user-mode access to an address a long way below the stack pointer is > > only valid if the instruction is one that would update the stack pointer > > to the address accessed. This is checked by directly looking at the > > instructions op-code. As a result is does not take into account prefixed > > instructions. Instead of looking at the instruction our self, use > > analyse_instr() determine if this a store instruction that will update > > the stack pointer. > > > > Something to note is that there currently are not any store with update > > prefixed instructions. Actually there is no plan for prefixed > > update-form loads and stores. So this patch is probably not needed but > > it might be preferable to use analyse_instr() rather than open coding > > the test anyway. > > Yes please. I was looking through this code recently and was > horrified. This improves things a lot and I think is justification > enough as-is. > Except it doesn't work... I'm now experiencing a systematic crash of systemd at boot in my fedora31 guest: [ 3.322912] systemd[1]: segfault (11) at 7ffff3eaf550 nip 7ce4d42f8d78 lr 9d82c098fc0 code 1 in libsystemd-shared-243.so[7ce4d4150000+2e0000] [ 3.323112] systemd[1]: code: 00000480 60420000 3c4c001e 3842edb0 7c0802a6 3d81fff0 fb81ffe0 fba1ffe8 [ 3.323244] systemd[1]: code: fbc1fff0 fbe1fff8 f8010010 7c200b78 <f801f001> 7c216000 4082fff8 f801ff71 f801f001 is 0x1a8d78 <serialize_item_format+40>: stdu r0,-4096(r1) which analyse_instr() is supposed to decode as a STORE that updates r1 so we should be good... Unfortunately analyse_instr() forbids partial register sets, since it might return op->val based on some register content depending on the instruction: /* Following cases refer to regs->gpr[], so we need all regs */ if (!FULL_REGS(regs)) return -1; analyse_instr() was introduced with instruction emulation in mind, which goes far beyond the need we have in store_updates_sp(). Especially the fault path doesn't care for the register content at all... Not sure how to cope with that correctly (refactor analyse_instr() ? ) but until someone comes up with a solution, please don't merge this patch. Cheers, -- Greg > Regards, > Daniel > > > > Signed-off-by: Jordan Niethe <jniet...@gmail.com> > > --- > > arch/powerpc/mm/fault.c | 39 +++++++++++---------------------------- > > 1 file changed, 11 insertions(+), 28 deletions(-) > > > > diff --git a/arch/powerpc/mm/fault.c b/arch/powerpc/mm/fault.c > > index b5047f9b5dec..cb78b3ca1800 100644 > > --- a/arch/powerpc/mm/fault.c > > +++ b/arch/powerpc/mm/fault.c > > @@ -41,37 +41,17 @@ > > #include <asm/siginfo.h> > > #include <asm/debug.h> > > #include <asm/kup.h> > > +#include <asm/sstep.h> > > > > /* > > * Check whether the instruction inst is a store using > > * an update addressing form which will update r1. > > */ > > -static bool store_updates_sp(unsigned int inst) > > +static bool store_updates_sp(struct instruction_op *op) > > { > > - /* check for 1 in the rA field */ > > - if (((inst >> 16) & 0x1f) != 1) > > - return false; > > - /* check major opcode */ > > - switch (inst >> 26) { > > - case OP_STWU: > > - case OP_STBU: > > - case OP_STHU: > > - case OP_STFSU: > > - case OP_STFDU: > > - return true; > > - case OP_STD: /* std or stdu */ > > - return (inst & 3) == 1; > > - case OP_31: > > - /* check minor opcode */ > > - switch ((inst >> 1) & 0x3ff) { > > - case OP_31_XOP_STDUX: > > - case OP_31_XOP_STWUX: > > - case OP_31_XOP_STBUX: > > - case OP_31_XOP_STHUX: > > - case OP_31_XOP_STFSUX: > > - case OP_31_XOP_STFDUX: > > + if (GETTYPE(op->type) == STORE) { > > + if ((op->type & UPDATE) && (op->update_reg == 1)) > > return true; > > - } > > } > > return false; > > } > > @@ -278,14 +258,17 @@ static bool bad_stack_expansion(struct pt_regs *regs, > > unsigned long address, > > > > if ((flags & FAULT_FLAG_WRITE) && (flags & FAULT_FLAG_USER) && > > access_ok(nip, sizeof(*nip))) { > > - unsigned int inst; > > + unsigned int inst, sufx; > > + struct instruction_op op; > > int res; > > > > pagefault_disable(); > > - res = __get_user_inatomic(inst, nip); > > + res = __get_user_instr_inatomic(inst, sufx, nip); > > pagefault_enable(); > > - if (!res) > > - return !store_updates_sp(inst); > > + if (!res) { > > + analyse_instr(&op, uregs, inst, sufx); > > + return !store_updates_sp(&op); > > + } > > *must_retry = true; > > } > > return true; > > -- > > 2.20.1