lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
Open Source and information security mailing list archives
| ||
|
Date: Tue, 25 Aug 2020 11:37:11 +0200 From: Christophe Leroy <christophe.leroy@...roup.eu> To: Ravi Bangoria <ravi.bangoria@...ux.ibm.com>, mpe@...erman.id.au, christophe.leroy@....fr Cc: mikey@...ling.org, paulus@...ba.org, naveen.n.rao@...ux.vnet.ibm.com, pedromfc@...ux.ibm.com, rogealve@...ux.ibm.com, jniethe5@...il.com, linuxppc-dev@...ts.ozlabs.org, linux-kernel@...r.kernel.org Subject: Re: [PATCH v5 5/8] powerpc/watchpoint: Fix exception handling for CONFIG_HAVE_HW_BREAKPOINT=N Le 25/08/2020 à 06:36, Ravi Bangoria a écrit : > On powerpc, ptrace watchpoint works in one-shot mode. i.e. kernel > disables event every time it fires and user has to re-enable it. > Also, in case of ptrace watchpoint, kernel notifies ptrace user > before executing instruction. > > With CONFIG_HAVE_HW_BREAKPOINT=N, kernel is missing to disable > ptrace event and thus it's causing infinite loop of exceptions. > This is especially harmful when user watches on a data which is > also read/written by kernel, eg syscall parameters. In such case, > infinite exceptions happens in kernel mode which causes soft-lockup. > > Fixes: 9422de3e953d ("powerpc: Hardware breakpoints rewrite to handle non DABR breakpoint registers") > Reported-by: Pedro Miraglia Franco de Carvalho <pedromfc@...ux.ibm.com> > Signed-off-by: Ravi Bangoria <ravi.bangoria@...ux.ibm.com> > --- > arch/powerpc/include/asm/hw_breakpoint.h | 3 ++ > arch/powerpc/kernel/process.c | 48 +++++++++++++++++++++++ > arch/powerpc/kernel/ptrace/ptrace-noadv.c | 5 +++ > 3 files changed, 56 insertions(+) > > diff --git a/arch/powerpc/include/asm/hw_breakpoint.h b/arch/powerpc/include/asm/hw_breakpoint.h > index 2eca3dd54b55..c72263214d3f 100644 > --- a/arch/powerpc/include/asm/hw_breakpoint.h > +++ b/arch/powerpc/include/asm/hw_breakpoint.h > @@ -18,6 +18,7 @@ struct arch_hw_breakpoint { > u16 type; > u16 len; /* length of the target data symbol */ > u16 hw_len; /* length programmed in hw */ > + u8 flags; > }; > > /* Note: Don't change the first 6 bits below as they are in the same order > @@ -37,6 +38,8 @@ struct arch_hw_breakpoint { > #define HW_BRK_TYPE_PRIV_ALL (HW_BRK_TYPE_USER | HW_BRK_TYPE_KERNEL | \ > HW_BRK_TYPE_HYP) > > +#define HW_BRK_FLAG_DISABLED 0x1 > + > /* Minimum granularity */ > #ifdef CONFIG_PPC_8xx > #define HW_BREAKPOINT_SIZE 0x4 > diff --git a/arch/powerpc/kernel/process.c b/arch/powerpc/kernel/process.c > index 016bd831908e..160fbbf41d40 100644 > --- a/arch/powerpc/kernel/process.c > +++ b/arch/powerpc/kernel/process.c > @@ -636,6 +636,44 @@ void do_send_trap(struct pt_regs *regs, unsigned long address, > (void __user *)address); > } > #else /* !CONFIG_PPC_ADV_DEBUG_REGS */ > + > +static void do_break_handler(struct pt_regs *regs) > +{ > + struct arch_hw_breakpoint null_brk = {0}; > + struct arch_hw_breakpoint *info; > + struct ppc_inst instr = ppc_inst(0); > + int type = 0; > + int size = 0; > + unsigned long ea; > + int i; > + > + /* > + * If underneath hw supports only one watchpoint, we know it > + * caused exception. 8xx also falls into this category. > + */ > + if (nr_wp_slots() == 1) { > + __set_breakpoint(0, &null_brk); > + current->thread.hw_brk[0] = null_brk; > + current->thread.hw_brk[0].flags |= HW_BRK_FLAG_DISABLED; > + return; > + } > + > + /* Otherwise findout which DAWR caused exception and disable it. */ > + wp_get_instr_detail(regs, &instr, &type, &size, &ea); > + > + for (i = 0; i < nr_wp_slots(); i++) { > + info = ¤t->thread.hw_brk[i]; > + if (!info->address) > + continue; > + > + if (wp_check_constraints(regs, instr, ea, type, size, info)) { > + __set_breakpoint(i, &null_brk); > + current->thread.hw_brk[i] = null_brk; > + current->thread.hw_brk[i].flags |= HW_BRK_FLAG_DISABLED; > + } > + } > +} > + > void do_break (struct pt_regs *regs, unsigned long address, > unsigned long error_code) > { > @@ -647,6 +685,16 @@ void do_break (struct pt_regs *regs, unsigned long address, > if (debugger_break_match(regs)) > return; > > + /* > + * We reach here only when watchpoint exception is generated by ptrace > + * event (or hw is buggy!). Now if CONFIG_HAVE_HW_BREAKPOINT is set, > + * watchpoint is already handled by hw_breakpoint_handler() so we don't > + * have to do anything. But when CONFIG_HAVE_HW_BREAKPOINT is not set, > + * we need to manually handle the watchpoint here. > + */ > + if (!IS_ENABLED(CONFIG_HAVE_HW_BREAKPOINT)) > + do_break_handler(regs); > + > /* Deliver the signal to userspace */ > force_sig_fault(SIGTRAP, TRAP_HWBKPT, (void __user *)address); > } > diff --git a/arch/powerpc/kernel/ptrace/ptrace-noadv.c b/arch/powerpc/kernel/ptrace/ptrace-noadv.c > index 57a0ab822334..866597b407bc 100644 > --- a/arch/powerpc/kernel/ptrace/ptrace-noadv.c > +++ b/arch/powerpc/kernel/ptrace/ptrace-noadv.c > @@ -286,11 +286,16 @@ long ppc_del_hwdebug(struct task_struct *child, long data) > } > return ret; > #else /* CONFIG_HAVE_HW_BREAKPOINT */ > + if (child->thread.hw_brk[data - 1].flags & HW_BRK_FLAG_DISABLED) I think child->thread.hw_brk[data - 1].flags & HW_BRK_FLAG_DISABLED should go around additionnal () > + goto del; > + > if (child->thread.hw_brk[data - 1].address == 0) > return -ENOENT; What about replacing the above if by: if (!(child->thread.hw_brk[data - 1].flags) & HW_BRK_FLAG_DISABLED) && child->thread.hw_brk[data - 1].address == 0) return -ENOENT; That would avoid the goto and the label. > > +del: > child->thread.hw_brk[data - 1].address = 0; > child->thread.hw_brk[data - 1].type = 0; > + child->thread.hw_brk[data - 1].flags = 0; > #endif /* CONFIG_HAVE_HW_BREAKPOINT */ > > return 0; > Christophe
Powered by blists - more mailing lists